diff mbox

[C] Warn if inline attributes conflict (PR c/18079)

Message ID 20140320144423.GN6523@redhat.com
State New
Headers show

Commit Message

Marek Polacek March 20, 2014, 2:44 p.m. UTC 
On Thu, Mar 20, 2014 at 11:57:57AM +0000, Joseph S. Myers wrote:
> This version is OK (for after 4.9 branches).

Thanks.
 
> What happens if there are two declarations of the function, one with each 
> attribute?  The testcase doesn't cover that.

Nothing :(.  I had to add some code into diagnose_mismatched_decls to
detect such conflicting decls (plus for cold/hot; there may be others).

Tested x86_64-linux.  Is this version ok?

2014-03-20  Marek Polacek  <polacek@redhat.com>

	PR c/18079
c/
	* c-decl.c (diagnose_mismatched_decls): Warn for mismatched
	always_inline/noinline and hot/cold attributes.
c-family/
	* c-common.c (handle_noinline_attribute): Warn if the attribute
	conflicts with always_inline attribute.
	(handle_always_inline_attribute): Warn if the attribute conflicts
	with noinline attribute.
testsuite/
	* gcc.dg/pr18079.c: New test.
	* gcc.dg/pr18079-2.c: New test.


	Marek

Comments

Joseph Myers March 20, 2014, 3:24 p.m. UTC | #1 
On Thu, 20 Mar 2014, Marek Polacek wrote:

> On Thu, Mar 20, 2014 at 11:57:57AM +0000, Joseph S. Myers wrote:
> > This version is OK (for after 4.9 branches).
> 
> Thanks.
>  
> > What happens if there are two declarations of the function, one with each 
> > attribute?  The testcase doesn't cover that.
> 
> Nothing :(.  I had to add some code into diagnose_mismatched_decls to
> detect such conflicting decls (plus for cold/hot; there may be others).
> 
> Tested x86_64-linux.  Is this version ok?

Yes.
Richard Sandiford March 30, 2014, 7:25 p.m. UTC | #2 
Marek Polacek <polacek@redhat.com> writes:
> @@ -6666,7 +6666,16 @@ handle_noinline_attribute (tree *node, tree name,
>  			   int ARG_UNUSED (flags), bool *no_add_attrs)
>  {
>    if (TREE_CODE (*node) == FUNCTION_DECL)
> -    DECL_UNINLINABLE (*node) = 1;
> +    {
> +      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (*node)))
> +	{
> +	  warning (OPT_Wattributes, "%qE attribute ignored due to conflict "
> +		   "with %<always_inline%> attribute", name);
> +	  *no_add_attrs = true;
> +	}
> +      else
> +	DECL_UNINLINABLE (*node) = 1;
> +    }
>    else
>      {
>        warning (OPT_Wattributes, "%qE attribute ignored", name);
> @@ -6704,9 +6713,16 @@ handle_always_inline_attribute (tree *node, tree name,
>  {
>    if (TREE_CODE (*node) == FUNCTION_DECL)
>      {
> -      /* Set the attribute and mark it for disregarding inline
> -	 limits.  */
> -      DECL_DISREGARD_INLINE_LIMITS (*node) = 1;
> +      if (lookup_attribute ("noinline", DECL_ATTRIBUTES (*node)))
> +	{
> +	  warning (OPT_Wattributes, "%qE attribute ignored due to conflict "
> +		   "with %<noinline%> attribute", name);

Sorry for the nit, but maybe it'd be better to use %qs for the second
attributes, so that we can use the same translation string for all conflicts.
Same with the later messages (where the quotes that come with %qs are
probably needed too).

Thanks,
Richard
Richard Biener March 31, 2014, 8:06 a.m. UTC | #3 
On Sun, Mar 30, 2014 at 9:25 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Marek Polacek <polacek@redhat.com> writes:
>> @@ -6666,7 +6666,16 @@ handle_noinline_attribute (tree *node, tree name,
>>                          int ARG_UNUSED (flags), bool *no_add_attrs)
>>  {
>>    if (TREE_CODE (*node) == FUNCTION_DECL)
>> -    DECL_UNINLINABLE (*node) = 1;
>> +    {
>> +      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (*node)))
>> +     {
>> +       warning (OPT_Wattributes, "%qE attribute ignored due to conflict "
>> +                "with %<always_inline%> attribute", name);
>> +       *no_add_attrs = true;
>> +     }
>> +      else
>> +     DECL_UNINLINABLE (*node) = 1;
>> +    }
>>    else
>>      {
>>        warning (OPT_Wattributes, "%qE attribute ignored", name);
>> @@ -6704,9 +6713,16 @@ handle_always_inline_attribute (tree *node, tree name,
>>  {
>>    if (TREE_CODE (*node) == FUNCTION_DECL)
>>      {
>> -      /* Set the attribute and mark it for disregarding inline
>> -      limits.  */
>> -      DECL_DISREGARD_INLINE_LIMITS (*node) = 1;
>> +      if (lookup_attribute ("noinline", DECL_ATTRIBUTES (*node)))
>> +     {
>> +       warning (OPT_Wattributes, "%qE attribute ignored due to conflict "
>> +                "with %<noinline%> attribute", name);
>
> Sorry for the nit, but maybe it'd be better to use %qs for the second
> attributes, so that we can use the same translation string for all conflicts.
> Same with the later messages (where the quotes that come with %qs are
> probably needed too).

Good point, patch is pre-approved (also adjusting the two other conflict
warnings to spell out that the attribute is ignored).

Thanks,
Richard.

> Thanks,
> Richard
diff mbox

Patch

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index abd96fb..5258e52 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -6666,7 +6666,16 @@  handle_noinline_attribute (tree *node, tree name,
 			   int ARG_UNUSED (flags), bool *no_add_attrs)
 {
   if (TREE_CODE (*node) == FUNCTION_DECL)
-    DECL_UNINLINABLE (*node) = 1;
+    {
+      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (*node)))
+	{
+	  warning (OPT_Wattributes, "%qE attribute ignored due to conflict "
+		   "with %<always_inline%> attribute", name);
+	  *no_add_attrs = true;
+	}
+      else
+	DECL_UNINLINABLE (*node) = 1;
+    }
   else
     {
       warning (OPT_Wattributes, "%qE attribute ignored", name);
@@ -6704,9 +6713,16 @@  handle_always_inline_attribute (tree *node, tree name,
 {
   if (TREE_CODE (*node) == FUNCTION_DECL)
     {
-      /* Set the attribute and mark it for disregarding inline
-	 limits.  */
-      DECL_DISREGARD_INLINE_LIMITS (*node) = 1;
+      if (lookup_attribute ("noinline", DECL_ATTRIBUTES (*node)))
+	{
+	  warning (OPT_Wattributes, "%qE attribute ignored due to conflict "
+		   "with %<noinline%> attribute", name);
+	  *no_add_attrs = true;
+	}
+      else
+	/* Set the attribute and mark it for disregarding inline
+	   limits.  */
+	DECL_DISREGARD_INLINE_LIMITS (*node) = 1;
     }
   else
     {
diff --git gcc/c/c-decl.c gcc/c/c-decl.c
index 2c41bf2..f96cc5e 100644
--- gcc/c/c-decl.c
+++ gcc/c/c-decl.c
@@ -2099,18 +2099,36 @@  diagnose_mismatched_decls (tree newdecl, tree olddecl,
       /* Diagnose inline __attribute__ ((noinline)) which is silly.  */
       if (DECL_DECLARED_INLINE_P (newdecl)
 	  && lookup_attribute ("noinline", DECL_ATTRIBUTES (olddecl)))
-	{
-	  warned |= warning (OPT_Wattributes,
-			     "inline declaration of %qD follows "
-			     "declaration with attribute noinline", newdecl);
-	}
+	warned |= warning (OPT_Wattributes,
+			   "inline declaration of %qD follows "
+			   "declaration with attribute noinline", newdecl);
       else if (DECL_DECLARED_INLINE_P (olddecl)
 	       && lookup_attribute ("noinline", DECL_ATTRIBUTES (newdecl)))
-	{
-	  warned |= warning (OPT_Wattributes,
-			     "declaration of %q+D with attribute "
-			     "noinline follows inline declaration ", newdecl);
-	}
+	warned |= warning (OPT_Wattributes,
+			   "declaration of %q+D with attribute "
+			   "noinline follows inline declaration ", newdecl);
+      else if (lookup_attribute ("noinline", DECL_ATTRIBUTES (newdecl))
+	       && lookup_attribute ("always_inline", DECL_ATTRIBUTES (olddecl)))
+	warned |= warning (OPT_Wattributes,
+			   "declaration of %q+D with attribute "
+			   "noinline follows declaration with attribute "
+			   "always_inline", newdecl);
+      else if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (newdecl))
+	       && lookup_attribute ("noinline", DECL_ATTRIBUTES (olddecl)))
+	warned |= warning (OPT_Wattributes,
+			   "declaration of %q+D with attribute "
+			   "always_inline follows declaration with attribute "
+			   "noinline", newdecl);
+      else if (lookup_attribute ("cold", DECL_ATTRIBUTES (newdecl))
+	       && lookup_attribute ("hot", DECL_ATTRIBUTES (olddecl)))
+	warned |= warning (OPT_Wattributes,
+			   "declaration of %q+D with attribute cold follows "
+			   "declaration with attribute hot", newdecl);
+      else if (lookup_attribute ("hot", DECL_ATTRIBUTES (newdecl))
+	       && lookup_attribute ("cold", DECL_ATTRIBUTES (olddecl)))
+	warned |= warning (OPT_Wattributes,
+			   "declaration of %q+D with attribute hot follows "
+			   "declaration with attribute cold", newdecl);
     }
   else /* PARM_DECL, VAR_DECL */
     {
diff --git gcc/testsuite/gcc.dg/pr18079-2.c gcc/testsuite/gcc.dg/pr18079-2.c
index e69de29..5091dd41b 100644
--- gcc/testsuite/gcc.dg/pr18079-2.c
+++ gcc/testsuite/gcc.dg/pr18079-2.c
@@ -0,0 +1,16 @@ 
+/* PR c/18079 */
+/* { dg-do compile } */
+/* { dg-options "-Wall" } */
+
+__attribute__ ((always_inline)) void fndecl1 (void);
+__attribute__ ((noinline)) void fndecl1 (void); /* { dg-warning "attribute noinline follows declaration with attribute always_inline" } */
+
+__attribute__ ((noinline)) void fndecl2 (void);
+__attribute__ ((always_inline)) void fndecl2 (void); /* { dg-warning "attribute always_inline follows declaration with attribute noinline" } */
+
+
+__attribute__ ((hot)) void fndecl3 (void);
+__attribute__ ((cold)) void fndecl3 (void); /* { dg-warning "attribute cold follows declaration with attribute hot" } */
+
+__attribute__ ((cold)) void fndecl4 (void);
+__attribute__ ((hot)) void fndecl4 (void); /* { dg-warning "attribute hot follows declaration with attribute cold" } */
diff --git gcc/testsuite/gcc.dg/pr18079.c gcc/testsuite/gcc.dg/pr18079.c
index e69de29..b84cdeb 100644
--- gcc/testsuite/gcc.dg/pr18079.c
+++ gcc/testsuite/gcc.dg/pr18079.c
@@ -0,0 +1,33 @@ 
+/* PR c/18079 */
+/* { dg-do compile } */
+/* { dg-options "-Wall" } */
+
+__attribute__ ((noinline))
+__attribute__ ((always_inline))
+int
+fn1 (int r)
+{ /* { dg-warning "attribute ignored due to conflict" } */
+  return r & 4;
+}
+
+__attribute__ ((noinline, always_inline))
+int
+fn2 (int r)
+{ /* { dg-warning "attribute ignored due to conflict" } */
+  return r & 4;
+}
+
+__attribute__ ((always_inline))
+__attribute__ ((noinline))
+inline int
+fn3 (int r)
+{ /* { dg-warning "attribute ignored due to conflict" } */
+  return r & 8;
+}
+
+__attribute__ ((always_inline, noinline))
+inline int
+fn4 (int r)
+{ /* { dg-warning "attribute ignored due to conflict" } */
+  return r & 8;
+}