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.
Yea.  That does seem wrong.  I wonder if we should immediately carve out
a patch to fix that independent of everything else and see if there's
any undesirable fallout.


>>
>> 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?
Well, I think the core issue here is what does the attribute mean and
that's brought up elsewhere in the discussion.   We've got the attribute
attached to the DECL node, but then we want to query an SSA_NAME.  But
the value in an SSA_NAME may have no real relationship with the DECL.

> 
>> 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(?).
This is a good example of the SSA_NAME vs DECL question above (there's
others of course).  The value held by the SSA_NAME that gets passed to
strlen here has nothing to do with the underlying DECL for p.

As long as we're using the attribute on the DECL rather than tracking
when it applies to the SSA_NAME, then we have problems like above.


> 
> 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.
Put that aside for now.  It's an implementation detail.

In an ideal world, should we warn for the above code?  I'd lean towards
warning on the assignment rather than the strlen call.  The assignment
effectively drops the nonstring attribute.  It feels a lot like the case
where an assignment drops a const qualifier.

Jeff