diff mbox series

reject conflicting attributes before calling handlers (PR 86453)

Message ID 4c30a386-0be0-f796-a651-757d86ee16b4@gmail.com
State New
Headers show
Series reject conflicting attributes before calling handlers (PR 86453) | expand

Commit Message

Martin Sebor July 11, 2018, 10:04 p.m. UTC 
The attached change set adjusts the attribute exclusion code
to detect and reject incompatible attributes before attribute
handlers are called to have a chance to make changes despite
the exclusions.  The handlers are not run when a conflict is
found.

Tested on x86_64-linux.  I expected the fallout to be bigger
but only a handful of tests needed adjusting and the changes
all look like clear improvements.  I.e., conflicting attributes
that diagnosed as being ignored really are being ignored as one
would expect.

Martin

Comments

Richard Biener July 12, 2018, 8:46 a.m. UTC | #1 
On Wed, 11 Jul 2018, Martin Sebor wrote:

> The attached change set adjusts the attribute exclusion code
> to detect and reject incompatible attributes before attribute
> handlers are called to have a chance to make changes despite
> the exclusions.  The handlers are not run when a conflict is
> found.
> 
> Tested on x86_64-linux.  I expected the fallout to be bigger
> but only a handful of tests needed adjusting and the changes
> all look like clear improvements.  I.e., conflicting attributes
> that diagnosed as being ignored really are being ignored as one
> would expect.

OK.

Note the LTO testcase is now redundant after my commit so you can
omit it.

Thanks,
Richard.
Christophe Lyon July 13, 2018, 8:53 a.m. UTC | #2 
Hi,

On Thu, 12 Jul 2018 at 00:04, Martin Sebor <msebor@gmail.com> wrote:
>
> The attached change set adjusts the attribute exclusion code
> to detect and reject incompatible attributes before attribute
> handlers are called to have a chance to make changes despite
> the exclusions.  The handlers are not run when a conflict is
> found.
>
> Tested on x86_64-linux.  I expected the fallout to be bigger
> but only a handful of tests needed adjusting and the changes
> all look like clear improvements.  I.e., conflicting attributes
> that diagnosed as being ignored really are being ignored as one
> would expect.
>

Since you committed this patch (r262596), I've noticed regressions on
aarch64/arm:
    g++.dg/warn/pr86453.C  -std=c++11  (test for warnings, line 4)
    g++.dg/warn/pr86453.C  -std=c++11 (test for excess errors)
    g++.dg/warn/pr86453.C  -std=c++14  (test for warnings, line 4)
    g++.dg/warn/pr86453.C  -std=c++14 (test for excess errors)
    g++.dg/warn/pr86453.C  -std=c++98  (test for warnings, line 4)
    g++.dg/warn/pr86453.C  -std=c++98 (test for excess errors)

The log says:
Excess errors:
/gcc/testsuite/g++.dg/warn/pr86453.C:4:44: warning: ignoring attribute
'packed' because it conflicts with attribute 'aligned' [-Wattributes]

Isn't there the same message on x86_64?


> Martin
Martin Sebor July 13, 2018, 3:10 p.m. UTC | #3 
On 07/13/2018 02:53 AM, Christophe Lyon wrote:
> Hi,
>
> On Thu, 12 Jul 2018 at 00:04, Martin Sebor <msebor@gmail.com> wrote:
>>
>> The attached change set adjusts the attribute exclusion code
>> to detect and reject incompatible attributes before attribute
>> handlers are called to have a chance to make changes despite
>> the exclusions.  The handlers are not run when a conflict is
>> found.
>>
>> Tested on x86_64-linux.  I expected the fallout to be bigger
>> but only a handful of tests needed adjusting and the changes
>> all look like clear improvements.  I.e., conflicting attributes
>> that diagnosed as being ignored really are being ignored as one
>> would expect.
>>
>
> Since you committed this patch (r262596), I've noticed regressions on
> aarch64/arm:
>     g++.dg/warn/pr86453.C  -std=c++11  (test for warnings, line 4)
>     g++.dg/warn/pr86453.C  -std=c++11 (test for excess errors)
>     g++.dg/warn/pr86453.C  -std=c++14  (test for warnings, line 4)
>     g++.dg/warn/pr86453.C  -std=c++14 (test for excess errors)
>     g++.dg/warn/pr86453.C  -std=c++98  (test for warnings, line 4)
>     g++.dg/warn/pr86453.C  -std=c++98 (test for excess errors)
>
> The log says:
> Excess errors:
> /gcc/testsuite/g++.dg/warn/pr86453.C:4:44: warning: ignoring attribute
> 'packed' because it conflicts with attribute 'aligned' [-Wattributes]
>
> Isn't there the same message on x86_64?

There was.  The test above was added between the time I tested
my patch and the time I committed it.  I adjusted it yesterday
via r262609 so the failure should be gone.

Martin
Christophe Lyon July 13, 2018, 3:45 p.m. UTC | #4 
On Fri, 13 Jul 2018 at 17:10, Martin Sebor <msebor@gmail.com> wrote:
>
> On 07/13/2018 02:53 AM, Christophe Lyon wrote:
> > Hi,
> >
> > On Thu, 12 Jul 2018 at 00:04, Martin Sebor <msebor@gmail.com> wrote:
> >>
> >> The attached change set adjusts the attribute exclusion code
> >> to detect and reject incompatible attributes before attribute
> >> handlers are called to have a chance to make changes despite
> >> the exclusions.  The handlers are not run when a conflict is
> >> found.
> >>
> >> Tested on x86_64-linux.  I expected the fallout to be bigger
> >> but only a handful of tests needed adjusting and the changes
> >> all look like clear improvements.  I.e., conflicting attributes
> >> that diagnosed as being ignored really are being ignored as one
> >> would expect.
> >>
> >
> > Since you committed this patch (r262596), I've noticed regressions on
> > aarch64/arm:
> >     g++.dg/warn/pr86453.C  -std=c++11  (test for warnings, line 4)
> >     g++.dg/warn/pr86453.C  -std=c++11 (test for excess errors)
> >     g++.dg/warn/pr86453.C  -std=c++14  (test for warnings, line 4)
> >     g++.dg/warn/pr86453.C  -std=c++14 (test for excess errors)
> >     g++.dg/warn/pr86453.C  -std=c++98  (test for warnings, line 4)
> >     g++.dg/warn/pr86453.C  -std=c++98 (test for excess errors)
> >
> > The log says:
> > Excess errors:
> > /gcc/testsuite/g++.dg/warn/pr86453.C:4:44: warning: ignoring attribute
> > 'packed' because it conflicts with attribute 'aligned' [-Wattributes]
> >
> > Isn't there the same message on x86_64?
>
> There was.  The test above was added between the time I tested
> my patch and the time I committed it.  I adjusted it yesterday
> via r262609 so the failure should be gone.
>

Indeed, thanks!
I reported the regression because I didn't see any comment about it on
gcc-patches.

Christophe

> Martin
diff mbox series

Patch

PR c/86453 - error: type variant differs by TYPE_PACKED in free_lang_data since r255469

gcc/ChangeLog:

	PR c/86453
	* attribs.c (decl_attributes): Reject conflicting attributes before
	calling attribute handlers.

gcc/testsuite/ChangeLog:

	PR c/86453
	* c-c++-common/Wattributes.c: Adjust.
	* gcc.dg/Wattributes-10.c: New test.
	* g++.dg/Wattributes-3.C: Adjust.
	* g++.dg/lto/pr86453_0.C: New test.
	* gcc.dg/Wattributes-6.c: Adjust.
	* gcc.dg/pr18079.c: Adjust.
	* gcc.dg/torture/pr42363.c: Adjust.

Index: gcc/attribs.c
===================================================================
--- gcc/attribs.c	(revision 262542)
+++ gcc/attribs.c	(working copy)
@@ -672,6 +672,35 @@  decl_attributes (tree *node, tree attributes, int
 
       bool no_add_attrs = false;
 
+      /* Check for exclusions with other attributes on the current
+	 declation as well as the last declaration of the same
+	 symbol already processed (if one exists).  Detect and
+	 reject incompatible attributes.  */
+      bool built_in = flags & ATTR_FLAG_BUILT_IN;
+      if (spec->exclude
+	  && (flag_checking || !built_in))
+	{
+	  /* Always check attributes on user-defined functions.
+	     Check them on built-ins only when -fchecking is set.
+	     Ignore __builtin_unreachable -- it's both const and
+	     noreturn.  */
+
+	  if (!built_in
+	      || !DECL_P (*anode)
+	      || (DECL_FUNCTION_CODE (*anode) != BUILT_IN_UNREACHABLE
+		  && (DECL_FUNCTION_CODE (*anode)
+		      != BUILT_IN_UBSAN_HANDLE_BUILTIN_UNREACHABLE)))
+	    {
+	      bool no_add = diag_attr_exclusions (last_decl, *anode, name, spec);
+	      if (!no_add && anode != node)
+		no_add = diag_attr_exclusions (last_decl, *node, name, spec);
+	      no_add_attrs |= no_add;
+	    }
+	}
+
+      if (no_add_attrs)
+	continue;
+
       if (spec->handler != NULL)
 	{
 	  int cxx11_flag =
@@ -695,33 +724,6 @@  decl_attributes (tree *node, tree attributes, int
 	    returned_attrs = chainon (ret, returned_attrs);
 	}
 
-      /* If the attribute was successfully handled on its own and is
-	 about to be added check for exclusions with other attributes
-	 on the current declation as well as the last declaration of
-	 the same symbol already processed (if one exists).  */
-      bool built_in = flags & ATTR_FLAG_BUILT_IN;
-      if (spec->exclude
-	  && !no_add_attrs
-	  && (flag_checking || !built_in))
-	{
-	  /* Always check attributes on user-defined functions.
-	     Check them on built-ins only when -fchecking is set.
-	     Ignore __builtin_unreachable -- it's both const and
-	     noreturn.  */
-
-	  if (!built_in
-	      || !DECL_P (*anode)
-	      || (DECL_FUNCTION_CODE (*anode) != BUILT_IN_UNREACHABLE
-		  && (DECL_FUNCTION_CODE (*anode)
-		      != BUILT_IN_UBSAN_HANDLE_BUILTIN_UNREACHABLE)))
-	    {
-	      bool no_add = diag_attr_exclusions (last_decl, *anode, name, spec);
-	      if (!no_add && anode != node)
-		no_add = diag_attr_exclusions (last_decl, *node, name, spec);
-	      no_add_attrs |= no_add;
-	    }
-	}
-
       /* Layout the decl in case anything changed.  */
       if (spec->type_required && DECL_P (*node)
 	  && (VAR_P (*node)
Index: gcc/testsuite/c-c++-common/Wattributes.c
===================================================================
--- gcc/testsuite/c-c++-common/Wattributes.c	(revision 262542)
+++ gcc/testsuite/c-c++-common/Wattributes.c	(working copy)
@@ -39,13 +39,13 @@  PackedPacked { int i; };
    aligned and packed on a function declaration.  */
 
 void ATTR ((aligned (8), packed))
-faligned8_1 (void);           /* { dg-warning ".packed. attribute ignored" } */
+faligned8_1 (void);           /* { dg-warning "ignoring attribute .packed. because it conflicts with attribute .aligned." } */
 
 void ATTR ((aligned (8)))
-faligned8_2 (void);           /* { dg-message "previous declaration here" "" { xfail *-*-* } } */
+faligned8_2 (void);           /* { dg-message "previous declaration here" } */
 
 void ATTR ((packed))
-faligned8_2 (void);           /* { dg-warning ".packed. attribute ignored" } */
+faligned8_2 (void);           /* { dg-warning "ignoring attribute .packed. because it conflicts with attribute .aligned." } */
 
 /* Exercise the handling of the mutually exclusive attributes
    always_inline and noinline (in that order).  */
Index: gcc/testsuite/g++.dg/Wattributes-3.C
===================================================================
--- gcc/testsuite/g++.dg/Wattributes-3.C	(revision 262542)
+++ gcc/testsuite/g++.dg/Wattributes-3.C	(working copy)
@@ -26,6 +26,8 @@  B::operator char () const { return 0; }
 
 ATTR ((__noinline__))
 B::operator int () const      // { dg-warning "ignoring attribute .noinline. because it conflicts with attribute .always_inline." }
+// { dg-warning "function might not be inlinable" "" { target *-*-* } .-1 }
+
 {
   return 0;
 }
@@ -43,6 +45,7 @@  C::operator char () { return 0; }
 
 ATTR ((__noinline__))
 C::operator short ()           // { dg-warning "ignoring attribute .noinline. because it conflicts with attribute .always_inline." }
+// { dg-warning "function might not be inlinable" "" { target *-*-* } .-1 }
 { return 0; }
 
 inline ATTR ((__noinline__))
Index: gcc/testsuite/g++.dg/lto/pr86453_0.C
===================================================================
--- gcc/testsuite/g++.dg/lto/pr86453_0.C	(nonexistent)
+++ gcc/testsuite/g++.dg/lto/pr86453_0.C	(working copy)
@@ -0,0 +1,11 @@ 
+/* PR 86453/middle-end - error: type variant differs by TYPE_PACKED
+   in free_lang_data
+   { dg-lto-do link }
+   { dg-require-effective-target shared }
+   { dg-require-effective-target fpic }
+   { dg-lto-options {{-fPIC -shared -flto -w}} } */
+
+struct
+{
+  int *__attribute__((aligned, packed)) a;
+} b;
Index: gcc/testsuite/gcc.dg/Wattributes-10.c
===================================================================
--- gcc/testsuite/gcc.dg/Wattributes-10.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/Wattributes-10.c	(working copy)
@@ -0,0 +1,26 @@ 
+/* PR middle-end/86453 - error: type variant differs by TYPE_PACKED in
+   free_lang_data since r255469
+   { dg-do compile }
+   { dg-options "-Wall" } */
+
+#define A(expr) do { int a[1 - 2 * !(expr)]; (void)&a; } while (0)
+
+struct S
+{
+  int* __attribute__ ((aligned (16))) paligned;
+  int* __attribute__ ((packed)) ppacked;
+
+  int* __attribute__ ((aligned (16), packed)) qaligned;   /* { dg-warning "ignoring attribute .packed. because it conflicts with attribute .aligned." } */
+  int* __attribute__ ((packed, aligned (16))) qpacked;    /* { dg-warning "ignoring attribute .aligned. because it conflicts with attribute .packed." } */
+} s;
+
+void test (void)
+{
+  /* Verify that attributes reported ignored really are ignored
+     and not applied.  */
+
+  A (__alignof__ (s.paligned) == 16);
+  A (__alignof__ (s.ppacked) < 16);
+  A (__alignof__ (s.qaligned) == 16);
+  A (__alignof__ (s.qpacked) == __alignof__ (s.ppacked));
+}
Index: gcc/testsuite/gcc.dg/Wattributes-6.c
===================================================================
--- gcc/testsuite/gcc.dg/Wattributes-6.c	(revision 262542)
+++ gcc/testsuite/gcc.dg/Wattributes-6.c	(working copy)
@@ -39,13 +39,13 @@  PackedPacked { int i; };
    aligned and packed on a function declaration.  */
 
 void ATTR ((aligned (8), packed))
-faligned8_1 (void);           /* { dg-warning ".packed. attribute ignored" } */
+faligned8_1 (void);           /* { dg-warning "ignoring attribute .packed. because it conflicts with attribute .aligned." } */
 
 void ATTR ((aligned (8)))
-faligned8_2 (void);           /* { dg-message "previous declaration here" "" { xfail *-*-* } } */
+faligned8_2 (void);           /* { dg-message "previous declaration here" } */
 
 void ATTR ((packed))
-faligned8_2 (void);           /* { dg-warning ".packed. attribute ignored" } */
+faligned8_2 (void);           /* { dg-warning "ignoring attribute .packed. because it conflicts with attribute .aligned." } */
 
 /* Exercise the handling of the mutually exclusive attributes
    always_inline and noinline (in that order).  */
Index: gcc/testsuite/gcc.dg/pr18079.c
===================================================================
--- gcc/testsuite/gcc.dg/pr18079.c	(revision 262542)
+++ gcc/testsuite/gcc.dg/pr18079.c	(working copy)
@@ -6,7 +6,7 @@  __attribute__ ((noinline))
 __attribute__ ((always_inline))
 int
 fn1 (int r)
-{ /* { dg-warning "attribute ignored due to conflict" } */
+{ /* { dg-warning "ignoring attribute .always_inline. because it conflicts with attribute .noinline." } */
   return r & 4;
 }
 
@@ -13,7 +13,7 @@  fn1 (int r)
 __attribute__ ((noinline, always_inline))
 int
 fn2 (int r)
-{ /* { dg-warning "attribute ignored due to conflict" } */
+{ /* { dg-warning "ignoring attribute .always_inline. because it conflicts with attribute .noinline." } */
   return r & 4;
 }
 
@@ -21,7 +21,7 @@  __attribute__ ((always_inline))
 __attribute__ ((noinline))
 inline int
 fn3 (int r)
-{ /* { dg-warning "attribute ignored due to conflict" } */
+{ /* { dg-warning "ignoring attribute .noinline. because it conflicts with attribute .always_inline." } */
   return r & 8;
 }
 
@@ -28,6 +28,6 @@  fn3 (int r)
 __attribute__ ((always_inline, noinline))
 inline int
 fn4 (int r)
-{ /* { dg-warning "attribute ignored due to conflict" } */
+{ /* { dg-warning "ignoring attribute .noinline. because it conflicts with attribute .always_inline." } */
   return r & 8;
 }
Index: gcc/testsuite/gcc.dg/torture/pr42363.c
===================================================================
--- gcc/testsuite/gcc.dg/torture/pr42363.c	(revision 262542)
+++ gcc/testsuite/gcc.dg/torture/pr42363.c	(working copy)
@@ -54,8 +54,12 @@  static int __attribute__ ((pure, const, noreturn))
 barf (void) {
   /* { dg-warning "ignoring attribute .const." "const" { target *-*-* } .-1 } */
   /* { dg-warning "ignoring attribute .noreturn." "noreturn" { target *-*-* } .-2 } */
-} /* { dg-warning "does return" } */
 
+  /* The noreturn attribute is ignored so verify there is no warning
+     for returning from the function:
+     { dg-bogus "does return" } */
+}
+
 static int __attribute__ ((pure, const))
 bark (void) {   /* { dg-warning "ignoring attribute .const." } */
   barf ();