[C PATCH] Warn for optimize attribute on decl after definition (PR c/70255)

[C PATCH] Warn for optimize attribute on decl after definition (PR c/70255)

[Marek Polacek]

In this PR, Richi pointed out that we don't warn for the case when a
declaration with attribute optimize follows the definition which is lacking
that attribute.  This patch adds such a warning.  Though the question is
whether this shouldn't apply to more attributes than just "optimize".  And,
as can be seen in the testcase, we'll warn for even for the case when the
definition has
  optimize ("no-associative-math,O2")
and the declaration
  optimize ("O2,no-associative-math")
Not sure if we have something better than attribute_value_equal, though.

(The C++ FE lacks these kind of warnings; I opened PR71024 for that.)

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-05-09  Marek Polacek  <polacek@redhat.com>

	PR c/70255
	* c-decl.c (diagnose_mismatched_decls): Warn for optimize attribute
	on a declaration following the definition.

	* gcc.dg/attr-opt-1.c: New test.

diff --git gcc/c/c-decl.c gcc/c/c-decl.c
index 7094efc..6f97ed9 100644
--- gcc/c/c-decl.c
+++ gcc/c/c-decl.c
@@ -2228,6 +2228,18 @@ diagnose_mismatched_decls (tree newdecl, tree olddecl,
 
   if (TREE_CODE (newdecl) == FUNCTION_DECL)
     {
+      tree a1 = lookup_attribute ("optimize", DECL_ATTRIBUTES (olddecl));
+      tree a2 = lookup_attribute ("optimize", DECL_ATTRIBUTES (newdecl));
+      /* An optimization attribute applied on a declaration after the
+	 definition is likely not what the user wanted.  */
+      if (a2 != NULL_TREE
+	  && DECL_SAVED_TREE (olddecl) != NULL_TREE
+	  && (a1 == NULL_TREE || !attribute_value_equal (a1, a2)))
+	warned |= warning (OPT_Wattributes,
+			   "optimization attribute on %qD follows "
+			   "definition but the attribute doesn%'t match",
+			   newdecl);
+
       /* Diagnose inline __attribute__ ((noinline)) which is silly.  */
       if (DECL_DECLARED_INLINE_P (newdecl)
 	  && lookup_attribute ("noinline", DECL_ATTRIBUTES (olddecl)))
diff --git gcc/testsuite/gcc.dg/attr-opt-1.c gcc/testsuite/gcc.dg/attr-opt-1.c
index e69de29..07fa4db 100644
--- gcc/testsuite/gcc.dg/attr-opt-1.c
+++ gcc/testsuite/gcc.dg/attr-opt-1.c
@@ -0,0 +1,30 @@
+/* PR c/70255 */
+/* { dg-do compile } */
+
+double
+fn1 (double h, double l) /* { dg-message "previous definition" } */
+{
+  return h + l;
+}
+double fn1 (double, double) __attribute__ ((optimize ("no-associative-math"))); /* { dg-warning "optimization attribute on .fn1. follows definition" } */
+
+__attribute__ ((optimize ("no-associative-math"))) double
+fn2 (double h, double l)
+{
+  return h + l;
+}
+double fn2 (double, double) __attribute__ ((optimize ("no-associative-math")));
+
+__attribute__ ((optimize ("no-associative-math"))) double
+fn3 (double h, double l) /* { dg-message "previous definition" } */
+{
+  return h + l;
+}
+double fn3 (double, double) __attribute__ ((optimize ("O2,no-associative-math"))); /* { dg-warning "optimization attribute on .fn3. follows definition" } */
+
+__attribute__ ((optimize ("no-associative-math,O2"))) double
+fn4 (double h, double l) /* { dg-message "previous definition" } */
+{
+  return h + l;
+}
+double fn4 (double, double) __attribute__ ((optimize ("O2,no-associative-math"))); /* { dg-warning "optimization attribute on .fn4. follows definition" } */

	Marek

Re: [C PATCH] Warn for optimize attribute on decl after definition (PR c/70255)

[Joseph Myers]

On Mon, 9 May 2016, Marek Polacek wrote:

> In this PR, Richi pointed out that we don't warn for the case when a
> declaration with attribute optimize follows the definition which is lacking
> that attribute.  This patch adds such a warning.  Though the question is
> whether this shouldn't apply to more attributes than just "optimize".  And,
> as can be seen in the testcase, we'll warn for even for the case when the
> definition has
>   optimize ("no-associative-math,O2")
> and the declaration
>   optimize ("O2,no-associative-math")
> Not sure if we have something better than attribute_value_equal, though.
> 
> (The C++ FE lacks these kind of warnings; I opened PR71024 for that.)
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?

OK.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [C PATCH] Warn for optimize attribute on decl after definition (PR c/70255)

[Martin Sebor]

On 05/09/2016 08:45 AM, Marek Polacek wrote:
> In this PR, Richi pointed out that we don't warn for the case when a
> declaration with attribute optimize follows the definition which is lacking
> that attribute.  This patch adds such a warning.  Though the question is
> whether this shouldn't apply to more attributes than just "optimize".  And,
> as can be seen in the testcase, we'll warn for even for the case when the
> definition has
>    optimize ("no-associative-math,O2")
> and the declaration
>    optimize ("O2,no-associative-math")
> Not sure if we have something better than attribute_value_equal, though.

There is attribute_list_equal which seems more appropriate given
that there could be more than one attribute optimize associated
with a function, and the order of the attributes shouldn't matter.
attribute_value_equal only returns true if all attributes are
the same and in the same order.  I would not expect GCC to warn
on the following, for example:

   int attribute__ ((optimize ("no-reciprocal-math"),
                     optimize ("no-associative-math")))
   f () { return 0; }

   int __attribute__ ((optimize ("no-associative-math"),
                       optimize ("no-reciprocal-math")))
   f ();

Martin

Re: [C PATCH] Warn for optimize attribute on decl after definition (PR c/70255)

[Marek Polacek]

On Mon, May 09, 2016 at 09:52:41PM -0600, Martin Sebor wrote:
> On 05/09/2016 08:45 AM, Marek Polacek wrote:
> > In this PR, Richi pointed out that we don't warn for the case when a
> > declaration with attribute optimize follows the definition which is lacking
> > that attribute.  This patch adds such a warning.  Though the question is
> > whether this shouldn't apply to more attributes than just "optimize".  And,
> > as can be seen in the testcase, we'll warn for even for the case when the
> > definition has
> >    optimize ("no-associative-math,O2")
> > and the declaration
> >    optimize ("O2,no-associative-math")
> > Not sure if we have something better than attribute_value_equal, though.
> 
> There is attribute_list_equal which seems more appropriate given
> that there could be more than one attribute optimize associated
> with a function, and the order of the attributes shouldn't matter.

Ah, yea, I knew there was something else...  That's better so here is
a revised patch...

> attribute_value_equal only returns true if all attributes are
> the same and in the same order.  I would not expect GCC to warn
> on the following, for example:
> 
>   int attribute__ ((optimize ("no-reciprocal-math"),
>                     optimize ("no-associative-math")))
>   f () { return 0; }
> 
>   int __attribute__ ((optimize ("no-associative-math"),
>                       optimize ("no-reciprocal-math")))
>   f ();

... with this test included.  Thanks.

Bootstrapped/regtested on x86_64-linux, applying to trunk.

2016-05-10  Marek Polacek  <polacek@redhat.com>

	PR c/70255
	* c-decl.c (diagnose_mismatched_decls): Warn for optimize attribute
	on a declaration following the definition.

	* gcc.dg/attr-opt-1.c: New test.

diff --git gcc/c/c-decl.c gcc/c/c-decl.c
index 7094efc..9c09536 100644
--- gcc/c/c-decl.c
+++ gcc/c/c-decl.c
@@ -2228,6 +2228,18 @@ diagnose_mismatched_decls (tree newdecl, tree olddecl,
 
   if (TREE_CODE (newdecl) == FUNCTION_DECL)
     {
+      tree a1 = lookup_attribute ("optimize", DECL_ATTRIBUTES (olddecl));
+      tree a2 = lookup_attribute ("optimize", DECL_ATTRIBUTES (newdecl));
+      /* An optimization attribute applied on a declaration after the
+	 definition is likely not what the user wanted.  */
+      if (a2 != NULL_TREE
+	  && DECL_SAVED_TREE (olddecl) != NULL_TREE
+	  && (a1 == NULL_TREE || !attribute_list_equal (a1, a2)))
+	warned |= warning (OPT_Wattributes,
+			   "optimization attribute on %qD follows "
+			   "definition but the attribute doesn%'t match",
+			   newdecl);
+
       /* Diagnose inline __attribute__ ((noinline)) which is silly.  */
       if (DECL_DECLARED_INLINE_P (newdecl)
 	  && lookup_attribute ("noinline", DECL_ATTRIBUTES (olddecl)))
diff --git gcc/testsuite/gcc.dg/attr-opt-1.c gcc/testsuite/gcc.dg/attr-opt-1.c
index e69de29..4140fda 100644
--- gcc/testsuite/gcc.dg/attr-opt-1.c
+++ gcc/testsuite/gcc.dg/attr-opt-1.c
@@ -0,0 +1,37 @@
+/* PR c/70255 */
+/* { dg-do compile } */
+
+double
+fn1 (double h, double l) /* { dg-message "previous definition" } */
+{
+  return h + l;
+}
+double fn1 (double, double) __attribute__ ((optimize ("no-associative-math"))); /* { dg-warning "optimization attribute on .fn1. follows definition" } */
+
+__attribute__ ((optimize ("no-associative-math"))) double
+fn2 (double h, double l)
+{
+  return h + l;
+}
+double fn2 (double, double) __attribute__ ((optimize ("no-associative-math")));
+
+__attribute__ ((optimize ("no-associative-math"))) double
+fn3 (double h, double l) /* { dg-message "previous definition" } */
+{
+  return h + l;
+}
+double fn3 (double, double) __attribute__ ((optimize ("O2,no-associative-math"))); /* { dg-warning "optimization attribute on .fn3. follows definition" } */
+
+__attribute__ ((optimize ("no-associative-math,O2"))) double
+fn4 (double h, double l) /* { dg-message "previous definition" } */
+{
+  return h + l;
+}
+double fn4 (double, double) __attribute__ ((optimize ("O2,no-associative-math"))); /* { dg-warning "optimization attribute on .fn4. follows definition" } */
+
+__attribute__ ((optimize ("no-reciprocal-math"), optimize ("no-associative-math"))) int
+fn5 (void)
+{
+  return 0;
+}
+int __attribute__ ((optimize ("no-associative-math"), optimize ("no-reciprocal-math"))) fn5 (void);

	Marek