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.
>
>
>>
>>>
>>> /* 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.
We may be doing too much here in some cases, but the general idea is to
avoid duplicated warnings on one or more targets. IIRC it depends on
how they handle the various cases where you lower from one builtin to
another at expansion time. Hence my comment that we'd need to go back
and look at that discussion before ripping out so much nowarning stuff.
It's there for a reason (though I'm not convinced we're using it
consistently).
>
>>
>> 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
Yes, I'm aware of it.
jeff
