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?
> 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.
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.
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.
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.
Jakub
