[PATCH] fix ice in attribute copy (PR 89685)

[PATCH] fix ice in attribute copy (PR 89685)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 477 bytes --]

To copy type attributes from struct A, the copy attribute (new
in GCC 9) accepts a pointer argument such as (struct A*)0, but
it isn't prepared for anything much more complicated than that.
So for example when it's passed something like (struct A*)(0, 1)
as the test case in PR 89685 does (a P1 regression), it fails
with an ICE.

The attached patch makes this handling more robust by letting
it accept all forms of type and member references.

Tested on x86_64-linux.

Martin

[-- Attachment #2: gcc-89685.diff --]
[-- Type: text/x-patch, Size: 10777 bytes --]

PR c/89685 - ICE on attribute copy with a compound expression

gcc/c-family/ChangeLog:

	PR c/89685
	* c-attribs.c (handle_copy_attribute): Handle references and
	non-constant expressions.

gcc/testsuite/ChangeLog:

	PR c/89685
	* gcc.dg/attr-copy-8.c: New test.
	* g++.dg/ext/attr-copy-2.C: New test.

Index: gcc/c-family/c-attribs.c
===================================================================
--- gcc/c-family/c-attribs.c	(revision 269657)
+++ gcc/c-family/c-attribs.c	(working copy)
@@ -2413,15 +2413,30 @@ handle_copy_attribute (tree *node, tree name, tree
     }
 
   /* Consider address-of expressions in the attribute argument
-     as requests to copy from the referenced entity.  For constant
-     expressions, consider those to be requests to copy from their
-     type, such as in:
+     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));
+
+  /* For pointer expressions, consider those to be requests to copy
+     from their type, such as in:
        struct __attribute__ (copy ((struct T *)0)) U { ... };
      which copies type attributes from struct T to the declaration
      of struct U.  */
-  if (TREE_CODE (ref) == ADDR_EXPR)
-    ref = TREE_OPERAND (ref, 0);
-  else if (CONSTANT_CLASS_P (ref))
+  if ((CONSTANT_CLASS_P (ref) || EXPR_P (ref))
+      && POINTER_TYPE_P (TREE_TYPE (ref)))
     ref = TREE_TYPE (ref);
 
   if (DECL_P (decl))
Index: gcc/testsuite/gcc.dg/attr-copy-8.c
===================================================================
--- gcc/testsuite/gcc.dg/attr-copy-8.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/attr-copy-8.c	(working copy)
@@ -0,0 +1,97 @@
+/* PR c/89685 - ICE on attribute copy with a compound expression
+   { dg-do compile }
+   { dg-options "-Wall -Wno-unused-value -Wno-int-to-pointer-cast" } */
+
+#define ATTR(...) __attribute__ ((__VA_ARGS__))
+
+typedef struct ATTR (packed) A { ATTR (packed) unsigned bf: 1; } A;
+
+typedef struct B
+{
+  struct A a;
+  struct A *pa;
+} B;
+
+extern struct A a;
+extern struct A *pa;
+extern B b;
+extern B ab[1];
+extern B *pb;
+
+typedef struct C
+{
+  ATTR (copy ((struct A *)0)) short m_pa_0;
+  ATTR (copy ((struct A *)(1, 0))) int m_pa_1_0;
+  ATTR (copy ((struct A *)(0, 1))) long m_pa_0_1;
+
+  ATTR (copy (*(struct A *)0)) short m_xpa_0;
+  ATTR (copy (*(struct A *)(1, 0))) int m_xpa_1_0;
+  ATTR (copy (*(struct A *)(0, 1))) long m_xpa_0_1;
+
+  ATTR (copy (((struct A *)0)[0])) short m_arpa_0;
+  ATTR (copy (((struct A *)(1, 0))[0])) int m_arpa_1_0;
+  ATTR (copy (((struct A *)(0, 1))[0])) long m_arpa_0_1;
+
+  ATTR (copy (a)) short m_ra;
+  ATTR (copy (b.a)) int m_rb_a;
+  ATTR (copy (b.pa)) long m_rb_pa;
+
+  ATTR (copy (&a)) short m_ara;
+  ATTR (copy (&b.a)) int m_arb_a;
+  ATTR (copy (*b.pa)) long m_xb_pa;
+  ATTR (copy (b.pa[0])) long m_arb_pa;
+  
+  ATTR (copy (*pa)) short m_xpa;
+  ATTR (copy (pa[0])) short m_arpa;
+
+  ATTR (copy (ab[0].a)) int m_arab_a;
+  ATTR (copy (ab[1].pa)) long m_arab_pa;
+  ATTR (copy (*ab[2].pa)) int m_xarab_pa;
+  ATTR (copy (ab[3].pa->bf)) unsigned int m_arab_pa_bf: 1;
+
+  ATTR (copy (pb->a)) int m_pb_a;
+  ATTR (copy (pb->pa)) long m_pb_pa;
+  ATTR (copy (*pb->pa)) int m_xpb_pa;
+  ATTR (copy (pb->pa->bf)) unsigned int m_pb_pa_bf: 1;
+
+  ATTR (aligned (4), copy ((struct A *)(0))) short m_a4_pa_0;
+} C;
+
+
+_Static_assert (__builtin_has_attribute (((C*)0)->m_pa_0, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_pa_1_0, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_pa_0_1, packed));
+
+_Static_assert (__builtin_has_attribute (((C*)0)->m_xpa_0, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_xpa_1_0, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_xpa_0_1, packed));
+
+_Static_assert (__builtin_has_attribute (((C*)0)->m_arpa_0, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_arpa_1_0, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_arpa_0_1, packed));
+
+_Static_assert (__builtin_has_attribute (((C*)0)->m_ra, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_rb_a, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_rb_pa, packed));
+
+_Static_assert (__builtin_has_attribute (((C*)0)->m_ara, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_arb_a, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_xb_pa, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_arb_pa, packed));
+
+_Static_assert (__builtin_has_attribute (((C*)0)->m_xpa, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_arpa, packed));
+
+_Static_assert (__builtin_has_attribute (((C*)0)->m_arab_a, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_arab_pa, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_xarab_pa, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_arab_pa_bf, packed));
+
+_Static_assert (__builtin_has_attribute (((C*)0)->m_pb_a, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_pb_pa, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_xpb_pa, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_pb_pa_bf, packed));
+
+_Static_assert (__builtin_has_attribute (((C*)0)->m_a4_pa_0, packed));
+_Static_assert (__builtin_has_attribute (((C*)0)->m_a4_pa_0, aligned));
+_Static_assert (__alignof__ (((C*)0)->m_a4_pa_0) == 4);
Index: gcc/testsuite/g++.dg/ext/attr-copy-2.C
===================================================================
--- gcc/testsuite/g++.dg/ext/attr-copy-2.C	(nonexistent)
+++ gcc/testsuite/g++.dg/ext/attr-copy-2.C	(working copy)
@@ -0,0 +1,124 @@
+/* PR c/89685 - ICE on attribute copy with a compound expression
+   { dg-do compile }
+   { dg-options "-Wall -Wno-unused-value -Wno-int-to-pointer-cast" } */
+
+#if __cplusplus <= 199711L
+#  define static_assert(expr) typedef char Assert [1 - !(expr) * 2]
+#endif
+
+#define ATTR(...) __attribute__ ((__VA_ARGS__))
+
+typedef struct ATTR (packed) A { ATTR (packed) unsigned bf: 1; } A;
+
+typedef struct B
+{
+  struct A a;
+  struct A *pa;
+  struct A &ra;
+} B;
+
+extern struct A a;
+extern struct A *pa;
+extern struct A &ra;
+extern B b;
+extern B ab[1];
+extern B *pb;
+extern B &rb;
+
+typedef struct C
+{
+  ATTR (copy ((struct A *)0)) short m_pa_0;
+  ATTR (copy ((struct A *)(1, 0))) int m_pa_1_0;
+  ATTR (copy ((struct A *)(0, 1))) long m_pa_0_1;
+
+  ATTR (copy (*(struct A *)0)) short m_xpa_0;
+  ATTR (copy (*(struct A *)(1, 0))) int m_xpa_1_0;
+  ATTR (copy (*(struct A *)(0, 1))) long m_xpa_0_1;
+
+  ATTR (copy (((struct A *)0)[0])) short m_arpa_0;
+  ATTR (copy (((struct A *)(1, 0))[0])) int m_arpa_1_0;
+  ATTR (copy (((struct A *)(0, 1))[0])) long m_arpa_0_1;
+
+  ATTR (copy (a)) short m_a;
+  ATTR (copy (b.a)) int m_b_a;
+  ATTR (copy (b.pa)) long m_b_pa;
+  ATTR (copy (b.ra)) long m_b_ra;
+
+  ATTR (copy (&a)) short m_ara;
+  ATTR (copy (&b.a)) int m_arb_a;
+  ATTR (copy (*b.pa)) long m_xb_pa;
+  ATTR (copy (b.pa[0])) long m_arb_pa;
+
+  ATTR (copy (*pa)) short m_xpa;
+  ATTR (copy (pa[0])) short m_arpa;
+
+  ATTR (copy (ra)) short m_ra;
+
+  ATTR (copy (ab[0].a)) int m_arab_a;
+  ATTR (copy (ab[1].pa)) long m_arab_pa;
+  ATTR (copy (*ab[2].pa)) int m_xarab_pa;
+  ATTR (copy (ab[3].pa->bf)) unsigned int m_arab_pa_bf: 1;
+  ATTR (copy (ab[4].ra.bf)) unsigned int m_arab_ra_bf: 1;
+
+  ATTR (copy (pb->a)) int m_pb_a;
+  ATTR (copy (pb->pa)) long m_pb_pa;
+  ATTR (copy (*pb->pa)) int m_xpb_pa;
+  ATTR (copy (pb->pa->bf)) unsigned int m_pb_pa_bf: 1;
+  ATTR (copy (pb->ra.bf)) unsigned int m_pb_ra_bf: 1;
+
+  ATTR (copy (rb.a)) int m_rb_a;
+  ATTR (copy (rb.pa)) long m_rb_pa;
+  ATTR (copy (*rb.pa)) int m_xrb_pa;
+  ATTR (copy (rb.pa->bf)) unsigned int m_rb_pa_bf: 1;
+
+  ATTR (aligned (4), copy ((struct A *)(0))) short m_a4_pa_0;
+} C;
+
+
+static_assert (__builtin_has_attribute (((C*)0)->m_pa_0, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_pa_1_0, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_pa_0_1, packed));
+
+static_assert (__builtin_has_attribute (((C*)0)->m_xpa_0, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_xpa_1_0, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_xpa_0_1, packed));
+
+static_assert (__builtin_has_attribute (((C*)0)->m_ra, packed));
+
+static_assert (__builtin_has_attribute (((C*)0)->m_arpa_0, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_arpa_1_0, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_arpa_0_1, packed));
+
+static_assert (__builtin_has_attribute (((C*)0)->m_a, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_b_a, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_b_pa, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_b_ra, packed));
+
+static_assert (__builtin_has_attribute (((C*)0)->m_ara, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_arb_a, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_xb_pa, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_arb_pa, packed));
+
+static_assert (__builtin_has_attribute (((C*)0)->m_xpa, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_arpa, packed));
+
+static_assert (__builtin_has_attribute (((C*)0)->m_arab_a, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_arab_pa, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_xarab_pa, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_arab_pa_bf, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_arab_ra_bf, packed));
+
+static_assert (__builtin_has_attribute (((C*)0)->m_pb_a, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_pb_pa, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_pb_pa, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_pb_pa_bf, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_pb_ra_bf, packed));
+
+static_assert (__builtin_has_attribute (((C*)0)->m_rb_a, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_rb_pa, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_xrb_pa, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_rb_pa_bf, packed));
+
+static_assert (__builtin_has_attribute (((C*)0)->m_a4_pa_0, packed));
+static_assert (__builtin_has_attribute (((C*)0)->m_a4_pa_0, aligned));
+static_assert (__alignof__ (((C*)0)->m_a4_pa_0) == 4);

Re: [PATCH] fix ice in attribute copy (PR 89685)

[Jeff Law]

On 3/14/19 7:47 PM, Martin Sebor wrote:
> To copy type attributes from struct A, the copy attribute (new
> in GCC 9) accepts a pointer argument such as (struct A*)0, but
> it isn't prepared for anything much more complicated than that.
> So for example when it's passed something like (struct A*)(0, 1)
> as the test case in PR 89685 does (a P1 regression), it fails
> with an ICE.
> 
> The attached patch makes this handling more robust by letting
> it accept all forms of type and member references.
> 
> Tested on x86_64-linux.
> 
> Martin
> 
> gcc-89685.diff
> 
> PR c/89685 - ICE on attribute copy with a compound expression
> 
> gcc/c-family/ChangeLog:
> 
> 	PR c/89685
> 	* c-attribs.c (handle_copy_attribute): Handle references and
> 	non-constant expressions.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c/89685
> 	* gcc.dg/attr-copy-8.c: New test.
> 	* g++.dg/ext/attr-copy-2.C: New test.I think this is in the same state as the __builtin_has_attribute bits --
you're trying to support attributes on expressions.  We should reject
those as syntax errors right now.

jeff
> 

Re: [PATCH] fix ice in attribute copy (PR 89685)

[Martin Sebor]

On 3/19/19 12:42 PM, Jeff Law wrote:
> On 3/14/19 7:47 PM, Martin Sebor wrote:
>> To copy type attributes from struct A, the copy attribute (new
>> in GCC 9) accepts a pointer argument such as (struct A*)0, but
>> it isn't prepared for anything much more complicated than that.
>> So for example when it's passed something like (struct A*)(0, 1)
>> as the test case in PR 89685 does (a P1 regression), it fails
>> with an ICE.
>>
>> The attached patch makes this handling more robust by letting
>> it accept all forms of type and member references.
>>
>> Tested on x86_64-linux.
>>
>> Martin
>>
>> gcc-89685.diff
>>
>> PR c/89685 - ICE on attribute copy with a compound expression
>>
>> gcc/c-family/ChangeLog:
>>
>> 	PR c/89685
>> 	* c-attribs.c (handle_copy_attribute): Handle references and
>> 	non-constant expressions.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR c/89685
>> 	* gcc.dg/attr-copy-8.c: New test.
>> 	* g++.dg/ext/attr-copy-2.C: New test.I think this is in the same state as the __builtin_has_attribute bits --
> you're trying to support attributes on expressions.  We should reject
> those as syntax errors right now.

There is no way for attribute copy to refer to a type but by
mentioning an expression:

   struct __attribute__ ((aligned (8))) A { ... };
   struct __attribute__ ((copy ((struct S*)0))) struct B { };

Copying type attributes is one third of the feature's documented
purpose:

   copy
   copy (expression)

     The copy attribute applies the set of attributes with which
     the type of the expression has been declared to the declaration
     of the type to which the attribute is applied.

Why are you so determined to break these features?

Martin

Re: [PATCH] fix ice in attribute copy (PR 89685)

[Jeff Law]

On 3/14/19 7:47 PM, Martin Sebor wrote:
> To copy type attributes from struct A, the copy attribute (new
> in GCC 9) accepts a pointer argument such as (struct A*)0, but
> it isn't prepared for anything much more complicated than that.
> So for example when it's passed something like (struct A*)(0, 1)
> as the test case in PR 89685 does (a P1 regression), it fails
> with an ICE.
> 
> The attached patch makes this handling more robust by letting
> it accept all forms of type and member references.
> 
> Tested on x86_64-linux.
> 
> Martin
> 
> gcc-89685.diff
> 
> PR c/89685 - ICE on attribute copy with a compound expression
> 
> gcc/c-family/ChangeLog:
> 
> 	PR c/89685
> 	* c-attribs.c (handle_copy_attribute): Handle references and
> 	non-constant expressions.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c/89685
> 	* gcc.dg/attr-copy-8.c: New test.
> 	* g++.dg/ext/attr-copy-2.C: New test.
So after further review of the patch, the associated documentation and
discussions, I'm going to ACK this for the trunk.

The documentation on this is reasonably clear in specifying how it
applies to expressions (by digging through the expression's type).  It
doesn't in any way imply the attribute is associated with the expression.

Things like front-end folding of constant expressions and potentially
array decaying could result in inconsistencies.  I think we can
reasonably address these if they turn out to be an issue in practice.

Note I think the decision here impacts the decision around
builtin_has_attribute.  Martin has an update for that on the immediate
horizon -- my inclination is to go forward with that, perhaps after some
documentation clarifications.

Jeff