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.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.
> 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?
> 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.
It is possible that there are cases where this expansion produces a warning
where previous folding steps did not catch the issue, however they
are probably rare, and come at a cost, which means duplicated warnings.
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?
Bernd.
