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

login
register
mail settings
Submitter Marek Polacek
Date March 20, 2014, 11:45 a.m.
Message ID <20140320114552.GM6523@redhat.com>
Download mbox | patch
Permalink /patch/332094/
State New
Headers show

Comments

Marek Polacek - March 20, 2014, 11:45 a.m.
On Thu, Mar 20, 2014 at 12:10:35PM +0100, Richard Biener wrote:
> On Thu, Mar 20, 2014 at 12:07 PM, Marek Polacek <polacek@redhat.com> wrote:
> > We should warn if someone wants to use both always_inline and noinline
> > attributes.
> >
> > Regtested/bootstrapped on x86_64-linux.  This is hardly 4.9 material,
> > so ok for 5.0?
> 
> Shouldn't the warning state that the attribute will be ignored?  That is,
> the common warning is
> 
>       warning (OPT_Wattributes, "%qE attribute ignored", name);
> 
> which could be amended with " due to conflict with ....".

Dunno.  I did what we do wrt conflicting cold/hot attributes.  But
here's a patch with what you suggest (with some Extra Quotes).

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

	PR c/18079
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.


	Marek
Richard Guenther - March 20, 2014, 11:50 a.m.
On Thu, Mar 20, 2014 at 12:45 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Thu, Mar 20, 2014 at 12:10:35PM +0100, Richard Biener wrote:
>> On Thu, Mar 20, 2014 at 12:07 PM, Marek Polacek <polacek@redhat.com> wrote:
>> > We should warn if someone wants to use both always_inline and noinline
>> > attributes.
>> >
>> > Regtested/bootstrapped on x86_64-linux.  This is hardly 4.9 material,
>> > so ok for 5.0?
>>
>> Shouldn't the warning state that the attribute will be ignored?  That is,
>> the common warning is
>>
>>       warning (OPT_Wattributes, "%qE attribute ignored", name);
>>
>> which could be amended with " due to conflict with ....".
>
> Dunno.  I did what we do wrt conflicting cold/hot attributes.  But
> here's a patch with what you suggest (with some Extra Quotes).

That works for me, please let Joseph some time to comment.

The "%qE attribute conflicts with attribute %s" way leaves me out in
the dark on what attribute will be in effect for the decl - it could be
either or none as far as I read it.  Or even both, with unspecified
behavior.

Richard.

> 2014-03-20  Marek Polacek  <polacek@redhat.com>
>
>         PR c/18079
> 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.
>
> 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/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;
> +}
>
>         Marek
Joseph S. Myers - March 20, 2014, 11:57 a.m.
On Thu, 20 Mar 2014, Marek Polacek wrote:

> Dunno.  I did what we do wrt conflicting cold/hot attributes.  But
> here's a patch with what you suggest (with some Extra Quotes).
> 
> 2014-03-20  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c/18079
> 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.

This version is OK (for after 4.9 branches).

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

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/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;
+}