diff mbox

[3/3] diagnose attribute aligned conflicts (PR 81566)

Message ID 184eaaaa-bea6-482f-1579-c3e926d130fb@gmail.com
State New
Headers show

Commit Message

Martin Sebor Aug. 8, 2017, 4:13 p.m. UTC 
Patch 3 in the series restores the diagnostics for conflicting
attribute aligned on the same function (this regressed in r192199
in GCC 4.9), and also makes use of the enhanced infrastructure to
enhance the detection of the same conflicts on distinct declarations
of the same function.

Rather than issuing an error for the confluct as was done in GCC 4.8
I made it a -Wattributes warning.  That seems more in line with how
otherwise syntactically valid attributes are handled elsewhere, and
it also lets code developed since the regression was introduced to
temporarily suppress the warning until the problem is conflict is
corrected.

Martin

Comments

Jeff Law Oct. 2, 2017, 10:27 p.m. UTC | #1 
On 08/08/2017 10:13 AM, Martin Sebor wrote:
> Patch 3 in the series restores the diagnostics for conflicting
> attribute aligned on the same function (this regressed in r192199
> in GCC 4.9), and also makes use of the enhanced infrastructure to
> enhance the detection of the same conflicts on distinct declarations
> of the same function.
> 
> Rather than issuing an error for the confluct as was done in GCC 4.8
> I made it a -Wattributes warning.  That seems more in line with how
> otherwise syntactically valid attributes are handled elsewhere, and
> it also lets code developed since the regression was introduced to
> temporarily suppress the warning until the problem is conflict is
> corrected.
> 
> Martin
> 
> gcc-81544-3.diff
> 
> 
> PR c/81566 - invalid attribute aligned accepted on functions
> 
> gcc/c-family/ChangeLog:
> 
> 	PR c/81566
> 	c-attribs.c (handle_aligned_attribute): Diagnose conflicting
> 	attribute specifications.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c/8166
> 	* c-c++-common/Wattributes-2.c: New test.
OK.
jeff
Andreas Schwab Dec. 9, 2017, 11:40 a.m. UTC | #2 
That requires updates to gcc.dg/pr53037-4.c and g++.dg/pr53037-4.C.

FAIL: gcc.dg/pr53037-4.c (test for excess errors)                               
Excess errors:                                                                  
/usr/local/gcc/gcc-20171208/gcc/testsuite/gcc.dg/pr53037-4.c:9:1: error: alignment for 'foo2' must be at least 16

FAIL: g++.dg/pr53037-4.C  -std=gnu++98 (test for excess errors)
Excess errors:
/usr/local/gcc/gcc-20171208/gcc/testsuite/g++.dg/pr53037-4.C:9:1: error: alignment for 'void foo2()' must be at least 16

Andreas.
Martin Sebor Dec. 11, 2017, 6:06 p.m. UTC | #3 
On 12/09/2017 04:40 AM, Andreas Schwab wrote:
> That requires updates to gcc.dg/pr53037-4.c and g++.dg/pr53037-4.C.

I don't see these failures in my own test result or in those
reported for common targets.

Would you mind providing some details about where this output came
from?

Martin

> FAIL: gcc.dg/pr53037-4.c (test for excess errors)
> Excess errors:
> /usr/local/gcc/gcc-20171208/gcc/testsuite/gcc.dg/pr53037-4.c:9:1: error: alignment for 'foo2' must be at least 16
>
> FAIL: g++.dg/pr53037-4.C  -std=gnu++98 (test for excess errors)
> Excess errors:
> /usr/local/gcc/gcc-20171208/gcc/testsuite/g++.dg/pr53037-4.C:9:1: error: alignment for 'void foo2()' must be at least 16
>
> Andreas.
>
Andreas Schwab Dec. 11, 2017, 6:28 p.m. UTC | #4 
http://gcc.gnu.org/ml/gcc-testresults/2017-12/msg00672.html

Andreas.
diff mbox

Patch

PR c/81566 - invalid attribute aligned accepted on functions

gcc/c-family/ChangeLog:

	PR c/81566
	c-attribs.c (handle_aligned_attribute): Diagnose conflicting
	attribute specifications.

gcc/testsuite/ChangeLog:

	PR c/8166
	* c-c++-common/Wattributes-2.c: New test.

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index ac704bc..5a37dc2 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -1769,14 +1769,18 @@  check_cxx_fundamental_alignment_constraints (tree node,
    struct attribute_spec.handler.  */
 
 static tree
-handle_aligned_attribute (tree *node, tree ARG_UNUSED (name), tree args,
+handle_aligned_attribute (tree *node, tree name, tree args,
 			  int flags, bool *no_add_attrs)
 {
   tree decl = NULL_TREE;
   tree *type = NULL;
-  int is_type = 0;
+  bool is_type = false;
   tree align_expr;
-  int i;
+
+  /* The last (already pushed) declaration with all validated attributes
+     merged in or the current about-to-be-pushed one if one hassn't been
+     yet.  */
+  tree last_decl = node[1] ? node[1] : *node;
 
   if (args)
     {
@@ -1795,10 +1799,21 @@  handle_aligned_attribute (tree *node, tree ARG_UNUSED (name), tree args,
       is_type = TREE_CODE (*node) == TYPE_DECL;
     }
   else if (TYPE_P (*node))
-    type = node, is_type = 1;
+    type = node, is_type = true;
+
+  /* Log2 of specified alignment.  */
+  int pow2align = check_user_alignment (align_expr, true);
+
+  /* The alignment in bits corresponding to the specified alignment.  */
+  unsigned bitalign = (1U << pow2align) * BITS_PER_UNIT;
 
-  if ((i = check_user_alignment (align_expr, true)) == -1
-      || !check_cxx_fundamental_alignment_constraints (*node, i, flags))
+  /* The alignment of the current declaration and that of the last
+     pushed declaration, determined on demand below.  */
+  unsigned curalign = 0;
+  unsigned lastalign = 0;
+
+  if (pow2align == -1
+      || !check_cxx_fundamental_alignment_constraints (*node, pow2align, flags))
     *no_add_attrs = true;
   else if (is_type)
     {
@@ -1819,7 +1834,7 @@  handle_aligned_attribute (tree *node, tree ARG_UNUSED (name), tree args,
       else
 	*type = build_variant_type_copy (*type);
 
-      SET_TYPE_ALIGN (*type, (1U << i) * BITS_PER_UNIT);
+      SET_TYPE_ALIGN (*type, bitalign);
       TYPE_USER_ALIGN (*type) = 1;
     }
   else if (! VAR_OR_FUNCTION_DECL_P (decl)
@@ -1828,8 +1843,34 @@  handle_aligned_attribute (tree *node, tree ARG_UNUSED (name), tree args,
       error ("alignment may not be specified for %q+D", decl);
       *no_add_attrs = true;
     }
+  else if (TREE_CODE (decl) == FUNCTION_DECL
+	   && ((curalign = DECL_ALIGN (decl)) > bitalign
+	       || ((lastalign = DECL_ALIGN (last_decl)) > bitalign)))
+    {
+      /* Either a prior attribute on the same declaration or one
+	 on a prior declaration of the same function specifies
+	 stricter alignment than this attribute.  */
+      bool note = lastalign != 0;
+      if (lastalign)
+	curalign = lastalign;
+
+      curalign /= BITS_PER_UNIT;
+      bitalign /= BITS_PER_UNIT;
+
+      if (DECL_USER_ALIGN (decl) || DECL_USER_ALIGN (last_decl))
+	warning (OPT_Wattributes,
+		 "ignoring attribute %<%E (%u)%> because it conflicts with "
+		 "attribute %<%E (%u)%>", name, bitalign, name, curalign);
+      else
+	error ("alignment for %q+D must be at least %d", decl, curalign);
+
+      if (note)
+	inform (DECL_SOURCE_LOCATION (last_decl), "previous declaration here");
+
+      *no_add_attrs = true;
+    }
   else if (DECL_USER_ALIGN (decl)
-	   && DECL_ALIGN (decl) > (1U << i) * BITS_PER_UNIT)
+	   && DECL_ALIGN (decl) > bitalign)
     /* C++-11 [dcl.align/4]:
 
 	   When multiple alignment-specifiers are specified for an
@@ -1839,21 +1880,9 @@  handle_aligned_attribute (tree *node, tree ARG_UNUSED (name), tree args,
       This formally comes from the c++11 specification but we are
       doing it for the GNU attribute syntax as well.  */
     *no_add_attrs = true;
-  else if (TREE_CODE (decl) == FUNCTION_DECL
-	   && DECL_ALIGN (decl) > (1U << i) * BITS_PER_UNIT)
-    {
-      if (DECL_USER_ALIGN (decl))
-	error ("alignment for %q+D was previously specified as %d "
-	       "and may not be decreased", decl,
-	       DECL_ALIGN (decl) / BITS_PER_UNIT);
-      else
-	error ("alignment for %q+D must be at least %d", decl,
-	       DECL_ALIGN (decl) / BITS_PER_UNIT);
-      *no_add_attrs = true;
-    }
   else
     {
-      SET_DECL_ALIGN (decl, (1U << i) * BITS_PER_UNIT);
+      SET_DECL_ALIGN (decl, bitalign);
       DECL_USER_ALIGN (decl) = 1;
     }
 
diff --git a/gcc/testsuite/c-c++-common/Wattributes-2.c b/gcc/testsuite/c-c++-common/Wattributes-2.c
new file mode 100644
index 0000000..0904742
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wattributes-2.c
@@ -0,0 +1,74 @@ 
+/* PR c/81566 - invalid attribute aligned accepted on functions
+   { dg-do compile }
+   { dg-options "-Wall -Wattributes -ftrack-macro-expansion=0" } */
+
+#define ATTR(list) __attribute__ (list)
+#define ALIGN(n)   ATTR ((aligned (n)))
+
+/* It's okay to increase the alignment of a function.  */
+
+void ALIGN (16) ALIGN (32)
+falign32_1 (void);
+
+void ALIGN (16) falign32_2 (void);
+void ALIGN (32) falign32_2 (void);
+
+void falign32_2 (void) { }
+
+void ALIGN (32) falign32_2 (void);
+
+/* It's not okay to decrease it.  */
+
+void ALIGN (32) ALIGN (16)
+falign64_3 (void);            /* { dg-warning "ignoring attribute .aligned \\(16\\). because it conflicts with attribute .aligned \\(32\\)." } */
+
+void ALIGN (32)
+falign64_3 (void);
+
+void falign64_3 (void);
+
+void falign64_3 (void) { }
+
+
+void ALIGN (32)
+falign64_4 (void);            /* { dg-message "previous declaration here" } */
+
+void ALIGN (16)
+falign64_4 (void);            /* { dg-warning "ignoring attribute .aligned \\(16\\). because it conflicts with attribute .aligned \\(32\\)." } */
+
+void ALIGN (32)
+falign64_4 (void);            /* { dg-message "previous declaration here" } */
+
+void ALIGN (16)
+falign64_4 (void);            /* { dg-warning "ignoring attribute .aligned \\(16\\). because it conflicts with attribute .aligned \\(32\\)." } */
+
+void ALIGN (64)
+falign64_4 (void);
+
+void ALIGN (32)
+falign64_4 (void);            /* { dg-warning "ignoring attribute .aligned \\(32\\). because it conflicts with attribute .aligned \\(64\\)." } */
+
+void falign64_4 (void);
+
+void ALIGN (64)
+falign64_4 (void) { }
+
+void falign64_4 (void);
+
+void ALIGN (64)
+falign64_4 (void);
+
+
+void ATTR ((aligned (16), aligned (32)))
+falign64_5 (void);
+
+void ATTR ((aligned (32), aligned (64)))
+falign64_5 (void);
+
+void ATTR ((aligned (16), aligned (32), aligned (64)))
+falign64_5 (void);            /* { dg-warning "ignoring attribute .aligned \\(16\\). because it conflicts with attribute .aligned \\(64\\)." } */
+                              /* { dg-warning "ignoring attribute .aligned \\(32\\). because it conflicts with attribute .aligned \\(64\\)." "" { target *-*-* } .-1 } */
+
+
+void ATTR ((aligned (16), aligned (32), aligned (16)))
+falign64_6 (void);            /* { dg-warning "ignoring attribute .aligned \\(16\\). because it conflicts with attribute .aligned \\(32\\)." } */