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
