* [PATCH] avoid passing expressions to decl_attributes [PR94346]
@ 2020-03-27 1:55 Martin Sebor
2020-03-27 9:55 ` Jakub Jelinek
0 siblings, 1 reply; 3+ messages in thread
From: Martin Sebor @ 2020-03-27 1:55 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 649 bytes --]
Attribute copy can be invoked with an expression argument to copy from
the expression's type. However, it must avoid passing the expression
as the last (optional) argument to decl_attributes because the function
is only prepared to deal with DECLs and types.
The attached patch passes null as the last argument in these situations
instead. In addition, it makes more robust the handling of expressions
of pointers to function type (a subtle bug here was exposed by testing
of additional cases beyond the one in the report).
Tested on x86_64-linux. I plan to commit the patch in the next day or
so unless there are concerns/suggestions.
Martin
[-- Attachment #2: gcc-94346.diff --]
[-- Type: text/x-patch, Size: 4031 bytes --]
PR c++/94346 - [9/10 Regression] ICE due to handle_copy_attribute since r9-3982
gcc/c-family/ChangeLog:
PR c++/94346
* c-attribs.c (handle_copy_attribute): Avoid passing expressions
to decl_attributes. Make handling of different kinds of entities
more robust.
gcc/c-c++-common/ChangeLog:
PR c++/94346
* c-c++-common/attr-copy.c: New test.
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index f30158a258b..93e740853a9 100644
--- 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;
+
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);
@@ -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;
}
diff --git a/gcc/testsuite/c-c++-common/attr-copy.c b/gcc/testsuite/c-c++-common/attr-copy.c
new file mode 100644
index 00000000000..284088a8b97
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/attr-copy.c
@@ -0,0 +1,43 @@
+/* PR c++/94346 - ICE due to handle_copy_attribute
+ { dg-do compile }
+ { dg-options "-Wall" } */
+
+#define ATTR(...) __attribute__ ((__VA_ARGS__))
+
+#if __cplusplus > 199711L
+# define SA(expr) static_assert (expr, #expr)
+#elif __cplusplus
+# define SA(expr) \
+ typedef __attribute__ ((unused)) char Assert[!(expr) ? -1 : 1]
+#else
+# define SA(expr) _Static_assert (expr, #expr)
+#endif
+
+typedef struct ATTR (packed) A { ATTR (packed) unsigned bf: 1; } A;
+
+int bar (void);
+
+struct C
+{
+ char c;
+ ATTR (copy ((bar (), ((struct A *)(0))[0]))) int i;
+};
+
+/* Verify the attribute has been copied. */
+SA (__builtin_offsetof (struct C, i) == 1);
+
+
+
+/* Verify attribute copy can copy from the type a comma expression. */
+ATTR (alloc_size (1)) void* alloc1 (int);
+
+ATTR (copy ((bar (), alloc1))) void* alloc2 (int, int);
+
+ATTR (copy ((bar (), alloc1))) void alloc3 (int); /* { dg-warning "'alloc_size' attribute ignored on a function returning 'void'" } */
+
+
+typedef ATTR (alloc_size (1)) void* F (int);
+
+ATTR (copy ((bar (), (F*)0))) void* alloc4 (int, int);
+
+ATTR (copy ((bar (), (F*)0))) void alloc5 (int, int); /* { dg-warning "'alloc_size' attribute ignored on a function returning 'void'" } */
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] avoid passing expressions to decl_attributes [PR94346]
2020-03-27 1:55 [PATCH] avoid passing expressions to decl_attributes [PR94346] Martin Sebor
@ 2020-03-27 9:55 ` Jakub Jelinek
2020-03-27 20:41 ` Martin Sebor
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Jelinek @ 2020-03-27 9:55 UTC (permalink / raw)
To: Martin Sebor; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] avoid passing expressions to decl_attributes [PR94346]
2020-03-27 9:55 ` Jakub Jelinek
@ 2020-03-27 20:41 ` Martin Sebor
0 siblings, 0 replies; 3+ messages in thread
From: Martin Sebor @ 2020-03-27 20:41 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-03-27 20:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-27 1:55 [PATCH] avoid passing expressions to decl_attributes [PR94346] Martin Sebor
2020-03-27 9:55 ` Jakub Jelinek
2020-03-27 20:41 ` Martin Sebor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).