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

Martin Sebor msebor@gmail.com
Wed Sep 12 15:50:00 GMT 2018


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

There have been follow up comments in this thread suggesting
alternate designs for the nonstr attribute but (AFAICT) no
objections to the bug fix.  I don't expect to have the time
to redesign and reimplement the attribute for GCC 9 in terms
of the alias oracle as was suggested but I would like to avoid
the warning in the report.

Is the final patch okay to commit?

Martin

On 08/28/2018 06: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



More information about the Gcc-patches mailing list