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

[Jeff Law <law at redhat dot com>]

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.



> 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.

Jeff