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
