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

[Martin Sebor <msebor@gmail.com>]

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