On 09/17/18 20:32, Jeff Law wrote:
> On 9/17/18 12:20 PM, Bernd Edlinger wrote:
>> On 09/17/18 19:33, Jeff Law wrote:
>>> On 9/16/18 1:58 PM, Bernd Edlinger wrote:
>>>> Hi,
>>>>
>>>> this is a cleanup of the recently added strlen/strcpy/stpcpy
>>>> no nul warning code.
>>>>
>>>> Most importantly it moves the SSA_NAME handling from
>>>> unterminated_array to string_constant, thereby fixing
>>>> another round of xfails in the strlen and stpcpy test cases.
>>>>
>>>> I need to say that the fix for bug 86622 is relying in
>>>> type info on the pointer which is probably not safe in
>>>> GIMPLE in the light of the recent discussion.
>>>>
>>>> I had to add two further exceptions, which should
>>>> be safe in general: that is a pointer arithmentic on a string
>>>> literal is okay, and arithmetic on a string constant
>>>> that is exactly the size of the whole DECL, cannot
>>>> access an adjacent member.
>>>>
>>>>
>>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>>>> Is it OK for trunk?
>>>>
>>>>
>>>> Thanks
>>>> Bernd.
>>>>
>>>>
>>>> patch-cleanup-no-nul.diff
>>>>
>>>> gcc:
>>>> 2018-09-16 Bernd Edlinger <bernd.edlin...@hotmail.de>
>>>>
>>>> * builtins.h (unterminated_array): Remove prototype.
>>>> * builtins.c (unterminated_array): Simplify. Make static.
>>>> (expand_builtin_strcpy_args): Don't use TREE_NO_WARNING here.
>>>> (expand_builtin_stpcpy_1): Remove warn_string_no_nul without effect.
>>>> * expr.c (string_constant): Handle SSA_NAME. Add more exceptions
>>>> where pointer arithmetic is safe.
>>>> * gimple-fold.c (get_range_strlen): Handle nonstr like in c_strlen.
>>>> (get_max_strlen): Remove the unnecessary mynonstr handling.
>>>> (gimple_fold_builtin_strcpy): Simplify.
>>>> (gimple_fold_builtin_stpcpy): Simplify.
>>>> (gimple_fold_builtin_sprintf): Remove NO_WARNING propagation
>>>> without effect.
>>>> (gimple_fold_builtin_strlen): Simplify.
>>>>
>>>> testsuite:
>>>> 2018-09-16 Bernd Edlinger <bernd.edlin...@hotmail.de>
>>>>
>>>> * gcc.dg/warn-stpcpy-no-nul.c: Remove xfails.
>>>> * gcc.dg/warn-strlen-no-nul.c: Remove xfails.
>>>>
>>>> Index: gcc/builtins.c
>>>> ===================================================================
>>>> --- gcc/builtins.c (revision 264342)
>>>> +++ gcc/builtins.c (working copy)
>>>> @@ -567,31 +567,12 @@ warn_string_no_nul (location_t loc, const char *fn
>>>> the declaration of the object of which the array is a member or
>>>> element. Otherwise return null. */
>>>>
>>>> -tree
>>>> +static tree
>>>> unterminated_array (tree exp)
>>>> {
>>>> - if (TREE_CODE (exp) == SSA_NAME)
>>>> - {
>>>> - gimple *stmt = SSA_NAME_DEF_STMT (exp);
>>>> - if (!is_gimple_assign (stmt))
>>>> - return NULL_TREE;
>>>> -
>>>> - tree rhs1 = gimple_assign_rhs1 (stmt);
>>>> - tree_code code = gimple_assign_rhs_code (stmt);
>>>> - if (code == ADDR_EXPR
>>>> - && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
>>>> - rhs1 = rhs1;
>>>> - else if (code != POINTER_PLUS_EXPR)
>>>> - return NULL_TREE;
>>>> -
>>>> - exp = rhs1;
>>>> - }
>>>> -
>>>> tree nonstr = NULL;
>>>> - if (c_strlen (exp, 1, &nonstr, 1) == NULL && nonstr)
>>>> - return nonstr;
>>>> -
>>>> - return NULL_TREE;
>>>> + c_strlen (exp, 1, &nonstr);
>>>> + return nonstr;
>>>> }
>>> Sigh. This is going to conflict with patch #6 from Martin's kit.
>>>
>>
>> Sorry, it just feels wrong to do this folding here instead of in
>> string_constant.
>>
>> I think the handling of POINTER_PLUS_EXPR above, is faulty,
>> because it is ignoring the offset parameter, which typically
>> contains the offset part, may add an offset to a different
>> structure member or another array index. That is what happened
>> in PR 86622.
>>
>> So you are likely looking at the wrong index, or even the wrong
>> structure member.
> I'm not disagreeing that something's wrong here -- the whole concept
> that we extract the rhs and totally ignore the offset seems wrong. I've
> stumbled over it working through issues with either patch #4 or #6 from
> Martin. But I felt I needed to go back and reevaluate any assumptions I
> had about how the code was supposed to be used before I called it out.
>
>
> Your expr.c changes may be worth pushing through independently of the
> rest. AFAICT they're really just exposing more cases where we can
> determine that we're working with a stirng constant.
>
I think that moves just the folding from here to expr.c,
I don't see how the change in part #6 on unterminated_array
is supposed to work, I quote it here and add some comments:
> tree
> -unterminated_array (tree exp)
> +unterminated_array (tree exp, tree *size /* = NULL */, bool *exact /* = NULL
> */)
> {
> + /* Offset from the beginning of the array or null. */
> + tree off = NULL_TREE;
> +
> if (TREE_CODE (exp) == SSA_NAME)
> {
> gimple *stmt = SSA_NAME_DEF_STMT (exp);
> @@ -595,18 +600,43 @@ unterminated_array (tree exp)
>
> tree rhs1 = gimple_assign_rhs1 (stmt);
> tree_code code = gimple_assign_rhs_code (stmt);
> - if (code == ADDR_EXPR
> - && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
> - rhs1 = rhs1;
> - else if (code != POINTER_PLUS_EXPR)
> + if ((code == ADDR_EXPR
> + && TREE_CODE (TREE_OPERAND (rhs1, 0)) == ARRAY_REF)
> + || code == POINTER_PLUS_EXPR)
> + {
> + /* Store the index or offset. */
> + off = gimple_assign_rhs2 (stmt);
ADDR_EXPR does not have rhs2, right?
> + exp = rhs1;
> + }
> + else
> return NULL_TREE;
> -
> - exp = rhs1;
> }
>
> tree nonstr;
> - if (c_strlen (exp, 1, &nonstr) && nonstr)
> - return nonstr;
> + tree len = c_strlen (exp, 1, &nonstr);
> + if (len && nonstr)
> + {
> + if (size)
> + {
> + if (off)
> + {
> + if (TREE_CODE (off) == INTEGER_CST)
> + {
> + /* Subtract the offset from the size of the array. */
> + *exact = true;
> + off = fold_convert (ssizetype, off);
> + len = fold_build2 (MINUS_EXPR, ssizetype, len, off);
Underflow could happen here, off could even be negative.
Something similar to the check in string_constant should be done to avoid
that off might be moving the access to another array member:
if (POINTER_TYPE_P (TREE_TYPE (rhs1))
&& TREE_CODE (TREE_TYPE (TREE_TYPE (rhs1))) == ARRAY_TYPE
&& !(decl && !*decl)
&& !(decl && tree_fits_uhwi_p (DECL_SIZE_UNIT (*decl))
&& mem_size && tree_fits_uhwi_p (*mem_size)
&& tree_int_cst_equal (*mem_size, DECL_SIZE_UNIT (*decl))))
> + }
> + else
> + *exact = false;
> + }
> + else
> + *exact = true;
> +
> + *size = len;
> + }
> + return nonstr;
> + }
>
> return NULL_TREE;
> }
Yeah, sorry, but this looks like a hack.
I would just use c_strlen if I need the length
and optionally nonstr.
The previous version was a convenience when you
are only interested in nonstr, which may happen.
I suppose the expr.c change will likely work alone, but
it was designed to replace the stuff here.
So I will try to split the expr.c part out, and post it
tomorrow.
Bernd.
