[PATCH] reject conflicting attributes before calling handlers (PR 86453)

[PATCH] reject conflicting attributes before calling handlers (PR 86453)

[Martin Sebor]

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

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

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

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 ();

Re: [PATCH] reject conflicting attributes before calling handlers (PR 86453)

[Richard Biener]

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.

Re: [PATCH] reject conflicting attributes before calling handlers (PR 86453)

[Christophe Lyon]

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

Re: [PATCH] reject conflicting attributes before calling handlers (PR 86453)

[Martin Sebor]

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

Re: [PATCH] reject conflicting attributes before calling handlers (PR 86453)

[Christophe Lyon]

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