This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] avoid warning on constant strncpy until next statement is reachable (PR 87028)
On 08/29/2018 01:29 AM, Richard Biener wrote:
On Wed, Aug 29, 2018 at 2:12 AM Martin Sebor <msebor@gmail.com> 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, the question is what "nonstring" is, semantically. I read it
as sth like __restrinct - a pointer with "nonstring" attribute points
to a non-string. So I suspect your function either computes
"may expr point to a nonstring" or "must expr point to a nonstring"
if it gets a pointer argument. If it gets a (string) object it checks whether
that object is declared "nonstring" (thus, if you'd built a pointer to expr
whether that pointer _must_ point to a nonstring. So I guess the first
one is "must". Clearly looking at SSA_NAME_VAR isn't good here,
it would be semantically correct only for SSA_NAME_IS_DEFAULT_DEF
and SSA_NAME_VAR being a PARM_DECL.
I guess it would be nice to clearly separate the pointer vs. object case
by documentation in the function - all of the quoted parts above seem
to be for the address case so a gcc_assert (POINTER_TYPE_P (TREE_TYPE (decl))
inside the if (TREE_CODE (decl) == SSA_NAME) path should never trigger?
Attribute nonstring on either an array or a pointer decl means
"it need not be a nul-terminated string." I.e., it's just
a sequence of bytes. If it happens to have a nul in it then it
is a string. I don't think of the pointer case as different
from the array.
The get_attr_nonstring_decl() function isn't a predicate telling
us whether or not an expression refers to a string. It returns
true if it refers to an object declared nonstring. Whether what
the object contains/points to is in fact a string is determined
somewhere else.
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 say it shouldn't because I assign "bar" to p and after that p isn't
the original parameter anymore?
I agree with not warning here, but I don't think of p's nonstring
property as changing with an assignment. It's still nonstring,
we just know that what it points to at the moment is a string.
If the code were instead:
extern char a[];
p = a;
return strlen (a) + strlen (p);
a warning would be expected for strlen (p) because p is declared
to point to what need not be a string. A warning would not be
expected for strlen (a) because it is not declared nonstring so
when we don't know, the assumption is that it is a string.
Does that make sense?
Martin
PS Since restrict is a property of a pointer and part of the type
system nonstring a property of what the pointer points and not
part of the type system to I don't think of them as similar. In
my mind, nonstring is analogous to the notions of object constness
and volatility (but not the const and volatile qualifiers). it's
okay to assign the address of a const object to a non-const pointer,
but it's an error to try to modify the object through the pointer.
(It would be nice to add a warning to detect these kinds of errors
as well.)
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