[PATCH] Cleanup strcpy/stpcpy no nul warning code

[Jeff Law]

On 9/24/18 12:18 PM, Bernd Edlinger wrote:
> On 09/24/18 19:48, 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.edlinger@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.
>> So my thinking right now is to go forward with the API change to allow
>> c_strlen to fill in a structure with relevant tidbits about the string.
>>
>> That in turn allows us to use simplify unterminated_array in a manner
>> similar to what you've done in your patch -- while carrying forward the
>> capabilities we need for Martin's nul terminator warnings.  This would
>> be combined with the expr.c chunks from your patch.
>>
> 
> Do you want me to elaborate that idea?
No need.  I've already got those bits here.

> 
>> However, most of the changes to drop NO_WARNING stuff should be handled
>> separately.  I don't think they're safe as-is.  I'm also pretty sure the
>> stpcpy changes in builtins.c aren't correct as-is.
>>
>>
> 
> Well, I think you must be referring to this:
> 
> @@ -3984,14 +3964,10 @@ expand_builtin_stpcpy_1 (tree exp, rtx target, mac
>           compile-time, not an expression containing a string.  This is
>           because the latter will potentially produce pessimized code
>           when used to produce the return value.  */
> -      tree nonstr = NULL_TREE;
>         if (!c_getstr (src, NULL)
> -         || !(len = c_strlen (src, 0, &nonstr, 1)))
> +         || !(len = c_strlen (src, 0)))
>          return expand_movstr (dst, src, target, /*endp=*/2);
> 
> -      if (nonstr && !TREE_NO_WARNING (exp))
> -       warn_string_no_nul (EXPR_LOCATION (exp), "stpcpy", src, nonstr);
> -
>         lenp1 = size_binop_loc (loc, PLUS_EXPR, len, ssize_int (1));
>         ret = expand_builtin_mempcpy_args (dst, src, lenp1,
>                                           target, exp, /*endp=*/2);
> 
> 
> My observation is: If that one is necessary and does not only emit some
> duplicated warnings, then the test case must be incomplete, at least it did not
> regress when this code is removed.
I'm pretty sure the test is incomplete.

> 
> Maybe there could a better way than TREE_NO_WARNING to get rid
> of the duplicated warnings.
> 
> Maybe it will be best to concentrate the warnings on a single pass,
> which means expand will it not be, right?
COnceptually getting all the warnings out of the folder is a good start,
but insufficient.  You'd need to look at the thread for the duplicated
warnings due to how the expanders call into each other.

jeff