On 3/27/20 3:55 AM, Jakub Jelinek wrote:
On Thu, Mar 26, 2020 at 07:55:39PM -0600, Martin Sebor via Gcc-patches wrote:
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -2526,17 +2526,21 @@ handle_copy_attribute (tree *node, tree name, tree args,
&& !FUNCTION_POINTER_TYPE_P (TREE_TYPE (ref)))
ref = TREE_TYPE (ref);
+ tree reftype = DECL_P (ref) || EXPR_P (ref) ? TREE_TYPE (ref) : ref;
Perhaps
tree reftype = TYPE_P (ref) ? ref : TREE_TYPE (ref);
would be shorter and easier to read?
Sure.
if (DECL_P (decl))
{
if ((VAR_P (decl)
&& (TREE_CODE (ref) == FUNCTION_DECL
|| (EXPR_P (ref)
- && POINTER_TYPE_P (TREE_TYPE (ref))
- && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (TREE_TYPE (ref))))))
+ && POINTER_TYPE_P (reftype)
+ && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (reftype)))))
|| (TREE_CODE (decl) == FUNCTION_DECL
&& (VAR_P (ref)
|| (EXPR_P (ref)
- && !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (ref))))))
+ && !FUNC_OR_METHOD_TYPE_P (reftype)
+ && (!POINTER_TYPE_P (reftype)
+ || !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (reftype)))))))
{
/* It makes no sense to try to copy function attributes
to a variable, or variable attributes to a function. */
@@ -2606,15 +2610,11 @@ handle_copy_attribute (tree *node, tree name, tree args,
/* Similarly, a function declared with attribute noreturn has it
attached on to it, but a C11 _Noreturn function does not. */
- tree reftype = ref;
if (DECL_P (ref)
&& TREE_THIS_VOLATILE (ref)
- && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (reftype)))
+ && FUNC_OR_METHOD_TYPE_P (reftype))
TREE_THIS_VOLATILE (decl) = true;
- if (DECL_P (ref) || EXPR_P (ref))
- reftype = TREE_TYPE (ref);
-
if (POINTER_TYPE_P (reftype))
reftype = TREE_TYPE (reftype);
The above changes are certainly a good cleanup.
@@ -2623,9 +2623,10 @@ handle_copy_attribute (tree *node, tree name, tree args,
tree attrs = TYPE_ATTRIBUTES (reftype);
- /* Copy type attributes from REF to DECL. */
+ /* Copy type attributes from REF to DECL. Pass in REF if it's a DECL
+ or a type but not if it's an expression. */
for (tree at = attrs; at; at = TREE_CHAIN (at))
- decl_attributes (node, at, flags, ref);
+ decl_attributes (node, at, flags, EXPR_P (ref) ? NULL_TREE : ref);
return NULL_TREE;
}
This one really depends on what exactly the copy attribute should do.
For most expressions it's meant to copy the attributes from their type.
The reason why expressions are accepted as arguments is to avoid having
to declare things only to copy from types.
The current state where it tries to determine a decl from which to copy
attributes is something that is IMHO very hard to be documented:
>
/* Consider address-of expressions in the attribute argument
as requests to copy from the referenced entity. */
if (TREE_CODE (ref) == ADDR_EXPR)
ref = TREE_OPERAND (ref, 0);
do
{
/* Drill down into references to find the referenced decl. */
tree_code refcode = TREE_CODE (ref);
if (refcode == ARRAY_REF
|| refcode == INDIRECT_REF)
ref = TREE_OPERAND (ref, 0);
else if (refcode == COMPONENT_REF)
ref = TREE_OPERAND (ref, 1);
else
break;
} while (!DECL_P (ref));
but my point in the PR was that if you do it this way, COMPOUND_EXPRs should
be taken into account as well.
A COMPOUND_EXPR is an oddball corner case I used in the tests only to
stress the implementation.
In contrast, I expected ARRAY_REF and especially COMPONENT_REF and
INDIRECT_REF to be far more likely to come up because they support
use cases that aren't possible otherwise. For example, given:
struct A {
...
__attribute__ ((aligned (A) ... )) T a;
};
a natural thing to want to do is to define another struct with a member
that has the same properties as a, including alignment. The only way
to make that possible is by having attribute copy accept expressions
so that it can be used like this:
struct B {
__attribute__ ((copy (((struct A*)0)->a)) T a1;
};
I can think of no similar use case for the comma expression. At
the same time, I also can't think of any problems that extending
the special treatment to COMPOUND_EXPR would cause but I don't see
it as important.
Because, right now you e.g. diagnose
copy (1)
but don't diagnose
copy (bar (), 1)
which appart from some side-effects first is the same thing.
I don't disagree that the handler could be more discerning in what
it accepts and what it diagnoses. So could many other handlers.
I've been improving things here for a while and plan to continue
going forward.
Similarly, if you have
copy (&decl)
it will behave differently from
copy (bar (), &decl)
or
copy (&whatever.field)
from
copy (bar (), &whatever.field)
Yes, the comma expression isn't lvalue in C (but is in C++), but it doesn't
seem the current code would differentiate between what is an lvalue or
rvalue. But it is something that can come either from the user, or e.g.
from sanitizers if they need to evaluate something before evaluating some
expression, or from slight differentiations on how we deal with the
COMPOUND_EXPRs elsewhere in the FE (see the PR94339 change).
From the user POV, it would be better if there was separate syntax in which
case to extract the attributes from the type vs. when extract it from
much more limited set of expressions (basically, only decls or
COMPONENT_REFs) and error on anything else.
Maybe. I don't see it that way but regardless, it's too late to
change. The current interface was introduced in GCC 9 and changing
it could break code that relies on it. Adding a new one, while
possible, doesn't seem necessary given the very limited audience
this is designed for.
If you think some of these misuses are especially important to diagnose
please open bugs for them. I'll be looking into making improvements
in the area of attributes in stage 1 so I can see about improving this
one as well.
Until then, I have pushed just the fix for the ICE alone.
Martin
