Re: [PING] [PATCH] avoid warning on constant strncpy until next statement is reachable (PR 87028)

[Martin Sebor <msebor@gmail.com>]

Ping: https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01818.html

As with the other patch (bug 84561), there may be ways to redesign
the warning, but I don't have the cycles to undertake it before
stage 1 ends.  Unless someone has a simpler suggestion for how
to avoid this false positive now can we please accept this patch
for GCC 9 and consider the more ambitious approaches for GCC 10?

On 10/01/2018 03:24 PM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01818.html
>
> On 09/21/2018 11:13 AM, Martin Sebor wrote:
>> On 09/17/2018 07:30 PM, Jeff Law wrote:
>>> On 8/28/18 6:12 PM, Martin Sebor wrote:
>>>>>> Sadly, dstbase is the PARM_DECL for d.  That's where things are going
>>>>>> "wrong".  Not sure why you're getting the PARM_DECL in that case.
>>>>>> I'd
>>>>>> debug get_addr_base_and_unit_offset to understand what's going on.
>>>>>> Essentially you're getting different results of
>>>>>> get_addr_base_and_unit_offset in a case where they arguably should be
>>>>>> the same.
>>>>>
>>>>> Probably get_attr_nonstring_decl has the same "mistake" and returns
>>>>> the PARM_DECL instead of the SSA name pointer.  So we're comparing
>>>>> apples and oranges here.
>>>>
>>>> Returning the SSA_NAME_VAR from get_attr_nonstring_decl() is
>>>> intentional but the function need not (perhaps should not)
>>>> also set *REF to it.
>>>>
>>>>>
>>>>> Yeah:
>>>>>
>>>>> /* If EXPR refers to a character array or pointer declared attribute
>>>>>    nonstring return a decl for that array or pointer and set *REF to
>>>>>    the referenced enclosing object or pointer.  Otherwise returns
>>>>>    null.  */
>>>>>
>>>>> tree
>>>>> get_attr_nonstring_decl (tree expr, tree *ref)
>>>>> {
>>>>>   tree decl = expr;
>>>>>   if (TREE_CODE (decl) == SSA_NAME)
>>>>>     {
>>>>>       gimple *def = SSA_NAME_DEF_STMT (decl);
>>>>>
>>>>>       if (is_gimple_assign (def))
>>>>>         {
>>>>>           tree_code code = gimple_assign_rhs_code (def);
>>>>>           if (code == ADDR_EXPR
>>>>>               || code == COMPONENT_REF
>>>>>               || code == VAR_DECL)
>>>>>             decl = gimple_assign_rhs1 (def);
>>>>>         }
>>>>>       else if (tree var = SSA_NAME_VAR (decl))
>>>>>         decl = var;
>>>>>     }
>>>>>
>>>>>   if (TREE_CODE (decl) == ADDR_EXPR)
>>>>>     decl = TREE_OPERAND (decl, 0);
>>>>>
>>>>>   if (ref)
>>>>>     *ref = decl;
>>>>>
>>>>> I see a lot of "magic" here again in the attempt to "propagate"
>>>>> a nonstring attribute.
>>>>
>>>> That's the function's purpose: to look for the attribute.  Is
>>>> there a better way to do this?
>>>>
>>>>> Note
>>>>>
>>>>> foo (char *p __attribute__(("nonstring")))
>>>>> {
>>>>>   p = "bar";
>>>>>   strlen (p); // or whatever is necessary to call
>>>>> get_attr_nonstring_decl
>>>>> }
>>>>>
>>>>> is perfectly valid and p as passed to strlen is _not_ nonstring(?).
>>>>
>>>> I don't know if you're saying that it should get a warning or
>>>> shouldn't.  Right now it doesn't because the strlen() call is
>>>> folded before we check for nonstring.
>>>>
>>>> I could see an argument for diagnosing it but I suspect you
>>>> wouldn't like it because it would mean more warning from
>>>> the folder.  I could also see an argument against it because,
>>>> as you said, it's safe.
>>>>
>>>> If you take the assignment to p away then a warning is issued,
>>>> and that's because p is declared with attribute nonstring.
>>>> That's also why get_attr_nonstring_decl looks at SSA_NAME_VAR.
>>>>
>>>>> I think in your code comparing bases you want to look at the
>>>>> _original_
>>>>> argument to the string function rather than what
>>>>> get_attr_nonstring_decl
>>>>> returned as ref.
>>>>
>>>> I've adjusted get_attr_nonstring_decl() to avoid setting *REF
>>>> to SSA_NAME_VAR.  That let me remove the GIMPLE_NOP code from
>>>> the patch.  I've also updated the comment above SSA_NAME_VAR
>>>> to clarify its purpose per Jeff's comments.
>>>>
>>>> Attached is an updated revision with these changes.
>>>>
>>>> Martin
>>>>
>>>> gcc-87028.diff
>>>>
>>>> PR tree-optimization/87028 - false positive -Wstringop-truncation
>>>> strncpy with global variable source string
>>>> gcc/ChangeLog:
>>>>
>>>>     PR tree-optimization/87028
>>>>     * calls.c (get_attr_nonstring_decl): Avoid setting *REF to
>>>>     SSA_NAME_VAR.
>>>>     * gimple-fold.c (gimple_fold_builtin_strncpy): Avoid folding
>>>>     when statement doesn't belong to a basic block.
>>>>     * tree.h (SSA_NAME_VAR): Update comment.
>>>>     * tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Simplify.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>>     PR tree-optimization/87028
>>>>     * c-c++-common/Wstringop-truncation.c: Remove xfails.
>>>>     * gcc.dg/Wstringop-truncation-5.c: New test.
>>>>
>>>
>>>> Index: gcc/calls.c
>>>> ===================================================================
>>>> --- gcc/calls.c    (revision 263928)
>>>> +++ gcc/calls.c    (working copy)
>>>> @@ -1503,6 +1503,7 @@ tree
>>>>  get_attr_nonstring_decl (tree expr, tree *ref)
>>>>  {
>>>>    tree decl = expr;
>>>> +  tree var = NULL_TREE;
>>>>    if (TREE_CODE (decl) == SSA_NAME)
>>>>      {
>>>>        gimple *def = SSA_NAME_DEF_STMT (decl);
>>>> @@ -1515,17 +1516,25 @@ get_attr_nonstring_decl (tree expr, tree *ref)
>>>>            || code == VAR_DECL)
>>>>          decl = gimple_assign_rhs1 (def);
>>>>      }
>>>> -      else if (tree var = SSA_NAME_VAR (decl))
>>>> -    decl = var;
>>>> +      else
>>>> +    var = SSA_NAME_VAR (decl);
>>>>      }
>>>>
>>>>    if (TREE_CODE (decl) == ADDR_EXPR)
>>>>      decl = TREE_OPERAND (decl, 0);
>>>>
>>>> +  /* To simplify calling code, store the referenced DECL regardless of
>>>> +     the attribute determined below, but avoid storing the
>>>> SSA_NAME_VAR
>>>> +     obtained above (it's not useful for dataflow purposes).  */
>>>>    if (ref)
>>>>      *ref = decl;
>>>>
>>>> -  if (TREE_CODE (decl) == ARRAY_REF)
>>>> +  /* Use the SSA_NAME_VAR that was determined above to see if it's
>>>> +     declared nonstring.  Otherwise drill down into the referenced
>>>> +     DECL.  */
>>>> +  if (var)
>>>> +    decl = var;
>>>> +  else if (TREE_CODE (decl) == ARRAY_REF)
>>>>      decl = TREE_OPERAND (decl, 0);
>>>>    else if (TREE_CODE (decl) == COMPONENT_REF)
>>>>      decl = TREE_OPERAND (decl, 1);
>>> The more I look at this the more I think what we really want to be doing
>>> is real propagation of the property either via the alias oracle or a
>>> propagation engine.   You can't even guarantee that if you've got an
>>> SSA_NAME that the value it holds has any relation to its underlying
>>> SSA_NAME_VAR -- the value in the SSA_NAME could well have been copied
>>> from a some other SSA_NAME with a different underlying SSA_NAME_VAR.
>>>
>>> I'm not going to insist on it, but I think if we find ourselves
>>> extending this again in a way that is really working around lack of
>>> propagation of the property then we should go back and fix the
>>> propagation problem.
>>
>> We talked about improving this back in the GCC 8 cycle.  I've
>> been collecting input (and test cases) from Miguel Ojeda from
>> the adoption of the attribute in the Linux kernel.  There are
>> a number of issues I was hoping to get to in stage 1 but that
>> has been derailed by all the strlen back and forth.  I'm still
>> hoping to be able to fix some of the false positives here in
>> stage 3 but, IIUC the constraints, a redesign along the lines
>> you suggest would be considered overly intrusive.  (If not,
>> I'm willing to look into it.)
>>
>> That said, I had the impression from Richard's comments that
>> implementing the propagation in points-to analysis would come
>> at a cost and have its own downsides:
>>
>>   https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01954.html
>>
>> So I wasn't sure it was necessarily an endorsement of
>> the approach as the ideal solution or just a passing thought.
>>
>>>> Index: gcc/gimple-fold.c
>>>> ===================================================================
>>>> --- gcc/gimple-fold.c    (revision 263925)
>>>> +++ gcc/gimple-fold.c    (working copy)
>>>> @@ -1702,6 +1702,11 @@ gimple_fold_builtin_strncpy
>>>> (gimple_stmt_iterator
>>>>    if (tree_int_cst_lt (ssize, len))
>>>>      return false;
>>>>
>>>> +  /* Defer warning (and folding) until the next statement in the basic
>>>> +     block is reachable.  */
>>>> +  if (!gimple_bb (stmt))
>>>> +    return false;
>>>> +
>>>>    /* Diagnose truncation that leaves the copy unterminated.  */
>>>>    maybe_diag_stxncpy_trunc (*gsi, src, len);
>>> I thought Richi wanted the guard earlier (maybe_fold_stmt) -- it wasn't
>>> entirely clear to me if the subsequent comments about needing to fold
>>> early where meant to raise issues with guarding earlier or not.
>>
>> I'm fine with moving it if that's preferable.
>>
>> Moving the test to maybe_fold_stmt() would, IMO, be the right
>> change to make in general, at least for library built-ins.
>> I have been meaning to suggest it independently of this fix
>> but because of its pervasive impact I've been holding off,
>> expecting it to be controversial.  If there is consensus I'm
>> happy to make this change but I would prefer to do it separately
>> since it causes a number of regressions in tests that expect
>> built-ins to be folded very early on (i.e., look for evidence
>> of the folding in the output of -fdump-tree-gimple or
>> -fdump-tree-ccp1).  Some of the regression would go away if
>> maybe_fold_stmt() only avoided folding of library built-in
>> functions.  Resolving the others would require adjusting
>> the tests to either use optimization or look for the evidence
>> of folding in later passes than gimple or ccp1).  I think all
>> that is reasonable and won't impact the efficiency of
>> the emitted object code, but it's obviously a much bigger
>> change than a simple fix for a false positive warning.
>>
>> If that sounds reasonable, is the patch acceptable as is?
>>
>> The latest version is here:
>>
>>   https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01818.html
>>
>> Martin
>