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.
>
>>
>> /* Compute the length of a null-terminated character string or wide
>> @@ -3936,8 +3917,7 @@ expand_builtin_strcpy_args (tree exp, tree dest, t
>> if (tree nonstr = unterminated_array (src))
>> {
>> /* NONSTR refers to the non-nul terminated constant array. */
>> - if (!TREE_NO_WARNING (exp))
>> - warn_string_no_nul (EXPR_LOCATION (exp), "strcpy", src, nonstr);
>> + warn_string_no_nul (EXPR_LOCATION (exp), "strcpy", src, nonstr);
>> return NULL_RTX;
>> }
> There's also some target dependencies to be concerned about. I'll have
> to dig out the thread, but in general removing these TREE_NO_WARNING
> checks is probably a bad idea -- you're likely introducing redundant
> warnings on one or more targets (but not x86).
>
I just wondered why use if (!TREE_NO_WARNING(exp)) when warn_string_no_nul
uses if (!TREE_NO_WARNING(src)), and sets TREE_NO_WARNING(src) on success.
>
> There may be things in there we want to take. But my focus is going to
> be on getting the #4 and #6 patches from Martin's kit resolved (while he
> works on the string length range stuff).
>
Oh, you are aware that I have a proposed patch on the string length range
vs. gimple semantics here:
https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00805.html
Bernd.
