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?
Well, there's a distinction between looking for the attribute (which
will be on the _DECL node) and determining if the current instance (an
SSA_NAME) has that attribute.
What I think Richard is implying is that it might be better to propagate
the state of the attribute to instances rather than going from an
SSA_NAME backwards through the use-def chains or SSA_NAME_VAR to get to
a potentially related _DECL node.
This could be built into the alias oracle, or via a propagation engine.
In either approach you should be able to cut down on false positives as
well as false negatives.
>
>> 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.
Well, this is where propagating the bit would help. The assignment p =
"bar" would clobber the nonstring property because we know "bar" is
properly terminated. Pointer arithmetic, casts and the like would
preserve the property and so on.
If it were done via the aliasing oracle, the instance of P in the strlen
call would be known to point to a proper string and thus the call is safe.
Hope this helps...
Jeff
