[PATH] PR/49139 fix always_inline failures diagnostics

[PATH] PR/49139 fix always_inline failures diagnostics

[Christian Bruel]

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

Hello,

The attached patch fixes a few diagnostic discrepancies for 
always_inline failures.

Illustrated by the fail_always_inline[12].c attached cases, the current 
behavior is one of:

- success (with and without -Winline), silently not honoring always_inline
    gcc fail_always_inline1.c -S -Winline -O0 -fpic
    gcc fail_always_inline1.c -S -O2 -fpic

- error: with -Winline but not without
    gcc fail_always_inline1.c -S -Winline -O2 -fpic

- error: without -Winline
    gcc fail_always_inline2.c -S -fno-early-inlining -O2
    or the original c++ attachment in this defect

note that -Winline never warns, as stated in the documentation

This simple patch consistently emits a warning (changing the sorry 
unimplemented message) whenever the attribute is not honored.

My first will was to generate and error instead of the warning, but 
since it is possible that inlines is only performed at LTO time, an 
error would be inapropriate (Note that today this is not possible with 
-Winline that would abort).

Another alternative I considered would be to emit the warning under 
-Winline rather than unconditionally, but this more a user misuse of the 
attribute, so should always be warned anyway. Or maybe a new 
-Winline-always that would be activated under -Wall ? Other opinion 
welcomed.

Tested with standard bootstrap and regression on x86.

Comments, and/or OK for trunk ?

Many thanks,

Christian

2010-05-25  Christian Bruel  <christian.bruel@st.com>
	
	PR 49139
	* ipa-inline-transform.c (inline_transform):force call to 
optimize_inline_calls error if function is always_inline.
	* tree-inline.c (tree_inlinable_function_p): always warn.
	(expand_call_inline): Likewise.
	
2010-05-25  Christian Bruel  <christian.bruel@st.com>
	
	* gcc.db/always_inline.c: Removed -Winline. Update checks
	* gcc.db/always_inline2.c: Likewise.
	* gcc.db/always_inline3.c: Likewise.
	* gcc.db/fail_always_inline1.c: New test.
	* gcc.db/fail_always_inline2.c: New test.	


	


[-- Attachment #2: fail_always_inline1.c --]
[-- Type: text/plain, Size: 236 bytes --]

/* { dg-do compile { target fpic } } */
/* { dg-options "-fpic" } */

__attribute((always_inline)) void
 bar() { } /* { dg-warning "can be overwriten at linktime" } */

void
f()
{
  bar(); /* { dg-warning "called from here" "" } */
}



[-- Attachment #3: fail_always_inline2.c --]
[-- Type: text/plain, Size: 296 bytes --]

/* { dg-do compile { target fpic } } */
/* { dg-options "-fpic -fno-early-inlining" } */

inline void foo() { foo2(); }

__attribute((always_inline)) void
 bar() { } /* { dg-warning "can be overwriten at linktime" } */

void
f()
{
  foo();
  bar(); /* { dg-warning "called from here" "" } */
}



[-- Attachment #4: always_inline_warn.patch --]
[-- Type: text/plain, Size: 4782 bytes --]

Index: gcc/ipa-inline-transform.c
===================================================================
--- gcc/ipa-inline-transform.c	(revision 174264)
+++ gcc/ipa-inline-transform.c	(working copy)
@@ -302,9 +302,20 @@
 
   for (e = node->callees; e; e = e->next_callee)
     {
-      cgraph_redirect_edge_call_stmt_to_callee (e);
+      gimple call = cgraph_redirect_edge_call_stmt_to_callee (e);
+
+      if (!inline_p)
+	{
       if (!e->inline_failed || warn_inline)
         inline_p = true;
+	  else
+	    {
+	      tree fn = gimple_call_fndecl (call);
+	      
+	      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (fn)))
+		inline_p = true;
+	    }
+	}
     }
 
   if (inline_p)
Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 174264)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,11 @@
+2010-05-25  Christian Bruel  <christian.bruel@st.com>
+	
+	PR 49139
+	* ipa-inline-transform.c (inline_transform): force optimize_inline_calls
+	error checking if always_inline seen.
+	* tree-inline.c (tree_inlinable_function_p): use error instead of sorry.
+	(expand_call_inline): Likewise.
+	
 2011-05-25  Ian Lance Taylor  <iant@google.com>
 
 	* godump.c (go_format_type): Output the first field with a usable
Index: gcc/testsuite/gcc.dg/always_inline.c
===================================================================
--- gcc/testsuite/gcc.dg/always_inline.c	(revision 174264)
+++ gcc/testsuite/gcc.dg/always_inline.c	(working copy)
@@ -1,8 +1,8 @@
 /* { dg-do compile } */
-/* { dg-options "-Winline -O2" } */
+/* { dg-options "-O2" } */
 #include <stdarg.h>
 inline __attribute__ ((always_inline)) void
-e(int t, ...) /* { dg-message "sorry\[^\n\]*variable argument" "" } */
+e(int t, ...) /* { dg-warning "variable argument" } */
 {
   va_list q;
   va_start (q, t);
Index: gcc/testsuite/gcc.dg/always_inline2.c
===================================================================
--- gcc/testsuite/gcc.dg/always_inline2.c	(revision 174264)
+++ gcc/testsuite/gcc.dg/always_inline2.c	(working copy)
@@ -1,8 +1,8 @@
 /* { dg-do compile } */
-/* { dg-options "-Winline -O2" } */
-inline __attribute__ ((always_inline)) void t(void); /* { dg-message "sorry\[^\n\]*body not available" "" } */
+/* { dg-options "-O2" } */
+inline __attribute__ ((always_inline)) void t(void); /* { dg-warning "body not available" } */
 void
 q(void)
 {
-  t(); 				/* { dg-message "sorry\[^\n\]*called from here" "" } */
+  t(); 				/* { dg-warning "called from here" } */
 }
Index: gcc/testsuite/gcc.dg/always_inline3.c
===================================================================
--- gcc/testsuite/gcc.dg/always_inline3.c	(revision 174264)
+++ gcc/testsuite/gcc.dg/always_inline3.c	(working copy)
@@ -1,11 +1,11 @@
 /* { dg-do compile } */
-/* { dg-options "-Winline -O2" } */
+/* { dg-options "-O2" } */
 int do_something_evil (void);
 inline __attribute__ ((always_inline)) void
-q2(void) /* { dg-message "sorry\[^\n\]*recursive" "" } */
+q2(void) /* { dg-warning "recursive inlining" } */
 {
   if (do_something_evil ())
     return;
-  q2(); 			/* { dg-message "sorry\[^\n\]*called from here" "" } */
+  q2(); 			/* { dg-warning "called from here" } */
   q2(); /* With -O2 we don't warn here, it is eliminated by tail recursion.  */
 }
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog	(revision 174264)
+++ gcc/testsuite/ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2010-05-25  Christian Bruel  <christian.bruel@st.com>
+	
+	* gcc.db/always_inline.c: Removed -Winline. Update checks
+	* gcc.db/always_inline2.c: Likewise.
+	* gcc.db/always_inline3.c: Likewise.
+	
 2011-05-26  Fabien Chêne  <fabien@gcc.gnu.org>
 	* g++.dg/init/pr25811-3.C: New.
 	* g++.dg/init/pr25811-4.C: New.
Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c	(revision 174264)
+++ gcc/tree-inline.c	(working copy)
@@ -3192,7 +3192,7 @@
 	 As a bonus we can now give more details about the reason why a
 	 function is not inlinable.  */
       if (always_inline)
-	sorry (inline_forbidden_reason, fn);
+	warning (0, inline_forbidden_reason, fn);
       else if (do_warning)
 	warning (OPT_Winline, inline_forbidden_reason, fn);
 
@@ -3744,9 +3744,9 @@
 	  /* Avoid warnings during early inline pass. */
 	  && cgraph_global_info_ready)
 	{
-	  sorry ("inlining failed in call to %q+F: %s", fn,
-		 _(cgraph_inline_failed_string (reason)));
-	  sorry ("called from here");
+	  warning (0, "inlining failed in call to always_inline %q+F: %s", fn,
+		 cgraph_inline_failed_string (reason));
+	  warning (0, "called from here");
 	}
       else if (warn_inline
 	       && DECL_DECLARED_INLINE_P (fn)

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Richard Guenther]

On Tue, May 31, 2011 at 9:54 AM, Christian Bruel <christian.bruel@st.com> wrote:
> Hello,
>
> The attached patch fixes a few diagnostic discrepancies for always_inline
> failures.
>
> Illustrated by the fail_always_inline[12].c attached cases, the current
> behavior is one of:
>
> - success (with and without -Winline), silently not honoring always_inline
>   gcc fail_always_inline1.c -S -Winline -O0 -fpic
>   gcc fail_always_inline1.c -S -O2 -fpic
>
> - error: with -Winline but not without
>   gcc fail_always_inline1.c -S -Winline -O2 -fpic
>
> - error: without -Winline
>   gcc fail_always_inline2.c -S -fno-early-inlining -O2
>   or the original c++ attachment in this defect
>
> note that -Winline never warns, as stated in the documentation
>
> This simple patch consistently emits a warning (changing the sorry
> unimplemented message) whenever the attribute is not honored.
>
> My first will was to generate and error instead of the warning, but since it
> is possible that inlines is only performed at LTO time, an error would be
> inapropriate (Note that today this is not possible with -Winline that would
> abort).
>
> Another alternative I considered would be to emit the warning under -Winline
> rather than unconditionally, but this more a user misuse of the attribute,
> so should always be warned anyway. Or maybe a new -Winline-always that would
> be activated under -Wall ? Other opinion welcomed.
>
> Tested with standard bootstrap and regression on x86.
>
> Comments, and/or OK for trunk ?

The patch is not ok, we may not fail to inline an always_inline
function.  To make this more consistent I proposed to warn
whenever you take the address of an always_inline function
(because then you can confuse GCC by indirectly calling
such function which we might inline dependent on optimization
setting and which we might discover we didn't inline only
dependent on optimization setting).  Honza proposed to move
the sorry()ing to when we feel the need to output the
always_inline function, thus when it was not optimized away,
but that would require us not preserving the body (do we?)
with -fpreserve-inline-functions.

For fail_always_inline1.c we should diagnose the appearant
misuse of always_inline with a warning, drop the attribute
but keep DECL_DISREGARD_INLINE_LIMITS set.

Same for fail_always_inline2.c.

I agree that sorry()ing for those cases is odd.  EIther we
should reject the declarations upfront
("always-inline function will not be inlinable"), or we should
emit a warning of that kind and make sure to not sorry later.

Richard.

> Many thanks,
>
> Christian
>
> 2010-05-25  Christian Bruel  <christian.bruel@st.com>
>
>        PR 49139
>        * ipa-inline-transform.c (inline_transform):force call to
> optimize_inline_calls error if function is always_inline.
>        * tree-inline.c (tree_inlinable_function_p): always warn.
>        (expand_call_inline): Likewise.
>
> 2010-05-25  Christian Bruel  <christian.bruel@st.com>
>
>        * gcc.db/always_inline.c: Removed -Winline. Update checks
>        * gcc.db/always_inline2.c: Likewise.
>        * gcc.db/always_inline3.c: Likewise.
>        * gcc.db/fail_always_inline1.c: New test.
>        * gcc.db/fail_always_inline2.c: New test.
>
>
>
>
>

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Jakub Jelinek]

On Tue, May 31, 2011 at 11:18:18AM +0200, Richard Guenther wrote:
> The patch is not ok, we may not fail to inline an always_inline
> function.  To make this more consistent I proposed to warn
> whenever you take the address of an always_inline function
> (because then you can confuse GCC by indirectly calling
> such function which we might inline dependent on optimization
> setting and which we might discover we didn't inline only
> dependent on optimization setting).  Honza proposed to move

That would warn on a lot of valid programs.  Even
#include <unistd.h>
void *readptr = read;
would warn, because read is both extern and
extern __inline __attribute__ ((__always_inline__, __artificial__, __gnu_inline__, __warn_unused_result__)) ssize_t
read (int __fd, void *__buf, size_t __nbytes)
{
  ...
}
wrapper.  Similarly dozens of other functions.  glibc relies on
extern inline gnu_inline behavior there, if you take address, the
extern is used instead of the inline.

	Jakub

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Christian Bruel]

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



On 05/31/2011 11:18 AM, Richard Guenther wrote:
> On Tue, May 31, 2011 at 9:54 AM, Christian Bruel<christian.bruel@st.com>  wrote:
>> Hello,
>>
>> The attached patch fixes a few diagnostic discrepancies for always_inline
>> failures.
>>
>> Illustrated by the fail_always_inline[12].c attached cases, the current
>> behavior is one of:
>>
>> - success (with and without -Winline), silently not honoring always_inline
>>    gcc fail_always_inline1.c -S -Winline -O0 -fpic
>>    gcc fail_always_inline1.c -S -O2 -fpic
>>
>> - error: with -Winline but not without
>>    gcc fail_always_inline1.c -S -Winline -O2 -fpic
>>
>> - error: without -Winline
>>    gcc fail_always_inline2.c -S -fno-early-inlining -O2
>>    or the original c++ attachment in this defect
>>
>> note that -Winline never warns, as stated in the documentation
>>
>> This simple patch consistently emits a warning (changing the sorry
>> unimplemented message) whenever the attribute is not honored.
>>
>> My first will was to generate and error instead of the warning, but since it
>> is possible that inlines is only performed at LTO time, an error would be
>> inapropriate (Note that today this is not possible with -Winline that would
>> abort).
>>
>> Another alternative I considered would be to emit the warning under -Winline
>> rather than unconditionally, but this more a user misuse of the attribute,
>> so should always be warned anyway. Or maybe a new -Winline-always that would
>> be activated under -Wall ? Other opinion welcomed.
>>
>> Tested with standard bootstrap and regression on x86.
>>
>> Comments, and/or OK for trunk ?
>
> The patch is not ok, we may not fail to inline an always_inline
> function.

OK, I thought so that would be an error. but I was afraid to abort the 
inline of function with a missing body (provided by another file) by 
LTO, which would be a regression. rethinking about this and considering 
that a valid LTO program should be valid without LTO, and that the scope 
is the translation unit, that would be OK to always reject 
attribute_inline on functions without a body.

To make this more consistent I proposed to warn
> whenever you take the address of an always_inline function
> (because then you can confuse GCC by indirectly calling
> such function which we might inline dependent on optimization
> setting and which we might discover we didn't inline only
> dependent on optimization setting).Honza proposed to move
> the sorry()ing to when we feel the need to output the
> always_inline function, thus when it was not optimized away,
> but that would require us not preserving the body (do we?)
> with -fpreserve-inline-functions.
>

But we don't know if taking the address of the function will result by a 
call to it, or how the pointer will be used. So I think the check should 
be done at the caller site. But I code like:

inline __attribute__((always_inline))  int foo() { return 0; }

static int (*ptr)() = foo;

int
bar()
{
   return ptr();
}

is not be (legitimately) inlined, but without diagnostic, I don't know 
where to look at this that at the moment.

> For fail_always_inline1.c we should diagnose the appearant
> misuse of always_inline with a warning, drop the attribute
> but keep DECL_DISREGARD_INLINE_LIMITS set.
>
 > Same for fail_always_inline2.c.
>
> I agree that sorry()ing for those cases is odd.  EIther we
> should reject the declarations upfront
> ("always-inline function will not be inlinable"), or we should
> emit a warning of that kind and make sure to not sorry later.

In addition to the error at the caller site, I've added the specific 
warn about the missing inline keyword.

Thanks for your comments, here is the new patch that I'm testing, 
(without the handling of indirect calls which can be treated separately).

Cheers

Christian

[-- Attachment #2: always_inline_warn.patch --]
[-- Type: text/plain, Size: 4832 bytes --]

2010-05-25  Christian Bruel  <christian.bruel@st.com>
	
	PR 49139
	* cgraph.c (cgraph_function_body_availability): Check always_inline
	* ipa-inline-transform.c (inline_transform): Force optimize_inline_calls
	error checking if always_inline seen.
	* tree-inline.c (tree_inlinable_function_p): Use error instead of sorry.
	(expand_call_inline): Likewise.

2010-05-25  Christian Bruel  <christian.bruel@st.com>
	
	* gcc.db/always_inline.c: Removed -Winline. Update checks
	* gcc.db/always_inline2.c: Likewise.
	* gcc.db/always_inline3.c: Likewise.
	* gcc.db/fail_always_inline.c: New.


Index: gcc/cgraph.c
===================================================================
--- gcc/cgraph.c	(revision 174264)
+++ gcc/cgraph.c	(working copy)
@@ -2414,7 +2414,14 @@
      bit.  */
 
   else if (decl_replaceable_p (node->decl) && !DECL_EXTERNAL (node->decl))
+    {
+      if (cgraph_global_info_ready)
+	if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (node->decl)))
+	  if (!DECL_DECLARED_INLINE_P (node->decl))
+	    warning (0, "always_inline function might not be inlinable");
+      
     avail = AVAIL_OVERWRITABLE;
+    }
   else avail = AVAIL_AVAILABLE;
 
   return avail;
Index: gcc/ipa-inline-transform.c
===================================================================
--- gcc/ipa-inline-transform.c	(revision 174264)
+++ gcc/ipa-inline-transform.c	(working copy)
@@ -302,9 +302,20 @@
 
   for (e = node->callees; e; e = e->next_callee)
     {
-      cgraph_redirect_edge_call_stmt_to_callee (e);
+      gimple call = cgraph_redirect_edge_call_stmt_to_callee (e);
+
+      if (!inline_p)
+	{
       if (!e->inline_failed || warn_inline)
         inline_p = true;
+	  else
+	    {
+	      tree fn = gimple_call_fndecl (call);
+	      
+	      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (fn)))
+		inline_p = true;
+	    }
+	}
     }
 
   if (inline_p)
Index: gcc/testsuite/gcc.dg/always_inline.c
===================================================================
--- gcc/testsuite/gcc.dg/always_inline.c	(revision 174264)
+++ gcc/testsuite/gcc.dg/always_inline.c	(working copy)
@@ -1,8 +1,8 @@
 /* { dg-do compile } */
-/* { dg-options "-Winline -O2" } */
+/* { dg-options "-O2" } */
 #include <stdarg.h>
 inline __attribute__ ((always_inline)) void
-e(int t, ...) /* { dg-message "sorry\[^\n\]*variable argument" "" } */
+e(int t, ...) /* { dg-error "variable argument" } */
 {
   va_list q;
   va_start (q, t);
Index: gcc/testsuite/gcc.dg/always_inline2.c
===================================================================
--- gcc/testsuite/gcc.dg/always_inline2.c	(revision 174264)
+++ gcc/testsuite/gcc.dg/always_inline2.c	(working copy)
@@ -1,8 +1,8 @@
 /* { dg-do compile } */
-/* { dg-options "-Winline -O2" } */
-inline __attribute__ ((always_inline)) void t(void); /* { dg-message "sorry\[^\n\]*body not available" "" } */
+/* { dg-options "-O2" } */
+inline __attribute__ ((always_inline)) void t(void); /* { dg-error "body not available" } */
 void
 q(void)
 {
-  t(); 				/* { dg-message "sorry\[^\n\]*called from here" "" } */
+  t(); 				/* { dg-error "called from here" } */
 }
Index: gcc/testsuite/gcc.dg/always_inline3.c
===================================================================
--- gcc/testsuite/gcc.dg/always_inline3.c	(revision 174264)
+++ gcc/testsuite/gcc.dg/always_inline3.c	(working copy)
@@ -1,11 +1,11 @@
 /* { dg-do compile } */
-/* { dg-options "-Winline -O2" } */
+/* { dg-options "-O2" } */
 int do_something_evil (void);
 inline __attribute__ ((always_inline)) void
-q2(void) /* { dg-message "sorry\[^\n\]*recursive" "" } */
+q2(void) /* { dg-error "recursive inlining" } */
 {
   if (do_something_evil ())
     return;
-  q2(); 			/* { dg-message "sorry\[^\n\]*called from here" "" } */
+  q2(); 			/* { dg-error "called from here" } */
   q2(); /* With -O2 we don't warn here, it is eliminated by tail recursion.  */
 }
Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c	(revision 174264)
+++ gcc/tree-inline.c	(working copy)
@@ -3192,7 +3192,7 @@
 	 As a bonus we can now give more details about the reason why a
 	 function is not inlinable.  */
       if (always_inline)
-	sorry (inline_forbidden_reason, fn);
+	error (inline_forbidden_reason, fn);
       else if (do_warning)
 	warning (OPT_Winline, inline_forbidden_reason, fn);
 
@@ -3744,9 +3744,9 @@
 	  /* Avoid warnings during early inline pass. */
 	  && cgraph_global_info_ready)
 	{
-	  sorry ("inlining failed in call to %q+F: %s", fn,
-		 _(cgraph_inline_failed_string (reason)));
-	  sorry ("called from here");
+	  error ("inlining failed in call to always_inline %q+F: %s", fn,
+		 cgraph_inline_failed_string (reason));
+	  error ("called from here");
 	}
       else if (warn_inline
 	       && DECL_DECLARED_INLINE_P (fn)

[-- Attachment #3: fail_always_inline.c --]
[-- Type: text/plain, Size: 232 bytes --]

/* { dg-do compile { target fpic } } */
/* { dg-options "-fpic" } */

__attribute((always_inline)) void
 bar() { } /* { dg-error "can be overwriten at linktime" } */

void
f()
{
  bar(); /* { dg-error "called from here" "" } */
}

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Richard Guenther]

On Tue, May 31, 2011 at 6:03 PM, Christian Bruel <christian.bruel@st.com> wrote:
>
>
> On 05/31/2011 11:18 AM, Richard Guenther wrote:
>>
>> On Tue, May 31, 2011 at 9:54 AM, Christian Bruel<christian.bruel@st.com>
>>  wrote:
>>>
>>> Hello,
>>>
>>> The attached patch fixes a few diagnostic discrepancies for always_inline
>>> failures.
>>>
>>> Illustrated by the fail_always_inline[12].c attached cases, the current
>>> behavior is one of:
>>>
>>> - success (with and without -Winline), silently not honoring
>>> always_inline
>>>   gcc fail_always_inline1.c -S -Winline -O0 -fpic
>>>   gcc fail_always_inline1.c -S -O2 -fpic
>>>
>>> - error: with -Winline but not without
>>>   gcc fail_always_inline1.c -S -Winline -O2 -fpic
>>>
>>> - error: without -Winline
>>>   gcc fail_always_inline2.c -S -fno-early-inlining -O2
>>>   or the original c++ attachment in this defect
>>>
>>> note that -Winline never warns, as stated in the documentation
>>>
>>> This simple patch consistently emits a warning (changing the sorry
>>> unimplemented message) whenever the attribute is not honored.
>>>
>>> My first will was to generate and error instead of the warning, but since
>>> it
>>> is possible that inlines is only performed at LTO time, an error would be
>>> inapropriate (Note that today this is not possible with -Winline that
>>> would
>>> abort).
>>>
>>> Another alternative I considered would be to emit the warning under
>>> -Winline
>>> rather than unconditionally, but this more a user misuse of the
>>> attribute,
>>> so should always be warned anyway. Or maybe a new -Winline-always that
>>> would
>>> be activated under -Wall ? Other opinion welcomed.
>>>
>>> Tested with standard bootstrap and regression on x86.
>>>
>>> Comments, and/or OK for trunk ?
>>
>> The patch is not ok, we may not fail to inline an always_inline
>> function.
>
> OK, I thought so that would be an error. but I was afraid to abort the
> inline of function with a missing body (provided by another file) by LTO,
> which would be a regression. rethinking about this and considering that a
> valid LTO program should be valid without LTO, and that the scope is the
> translation unit, that would be OK to always reject attribute_inline on
> functions without a body.
>
> To make this more consistent I proposed to warn
>>
>> whenever you take the address of an always_inline function
>> (because then you can confuse GCC by indirectly calling
>> such function which we might inline dependent on optimization
>> setting and which we might discover we didn't inline only
>> dependent on optimization setting).Honza proposed to move
>> the sorry()ing to when we feel the need to output the
>> always_inline function, thus when it was not optimized away,
>> but that would require us not preserving the body (do we?)
>> with -fpreserve-inline-functions.
>>
>
> But we don't know if taking the address of the function will result by a
> call to it, or how the pointer will be used. So I think the check should be
> done at the caller site. But I code like:
>
> inline __attribute__((always_inline))  int foo() { return 0; }
>
> static int (*ptr)() = foo;
>
> int
> bar()
> {
>  return ptr();
> }
>
> is not be (legitimately) inlined, but without diagnostic, I don't know where
> to look at this that at the moment.

Yeah, the issue is that we only warn if we end up seeing a direct
call to an always_inline function that wasn't inlined.  The idea of
sorrying() for remaining always_inline bodies instead would also
catch the above, but so would

inline __attribute__((always_inline))  int foo() { return 0; }
int (*ptr)() = foo;

(address-taken but not called).

>> For fail_always_inline1.c we should diagnose the appearant
>> misuse of always_inline with a warning, drop the attribute
>> but keep DECL_DISREGARD_INLINE_LIMITS set.
>>
>> Same for fail_always_inline2.c.
>>
>> I agree that sorry()ing for those cases is odd.  EIther we
>> should reject the declarations upfront
>> ("always-inline function will not be inlinable"), or we should
>> emit a warning of that kind and make sure to not sorry later.
>
> In addition to the error at the caller site, I've added the specific warn
> about the missing inline keyword.

But not in a good place.  Please instead check it alongside the
other attribute checks in cgraphunit.c:process_function_and_variable_attributes

> Thanks for your comments, here is the new patch that I'm testing, (without
> the handling of indirect calls which can be treated separately).

Index: gcc/ipa-inline-transform.c
===================================================================
--- gcc/ipa-inline-transform.c	(revision 174264)
+++ gcc/ipa-inline-transform.c	(working copy)
@@ -302,9 +302,20 @@

   for (e = node->callees; e; e = e->next_callee)
     {
-      cgraph_redirect_edge_call_stmt_to_callee (e);
+      gimple call = cgraph_redirect_edge_call_stmt_to_callee (e);
+
+      if (!inline_p)
+	{
       if (!e->inline_failed || warn_inline)
         inline_p = true;
+	  else
+	    {
+	      tree fn = gimple_call_fndecl (call);
+	
+	      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (fn)))
+		inline_p = true;
+	    }
+	}
     }

   if (inline_p)

Honza - this conditional calling of optimize_inline_calls just if
warn_inline is on is extra ugly.  Does it really save that much
time to only conditionally run optimize_inline_calls?  If so
we should re-write that function completely.

Christian, for now I suggest to instead do

Index: ipa-inline-transform.c
===================================================================
--- ipa-inline-transform.c      (revision 174520)
+++ ipa-inline-transform.c      (working copy)
@@ -288,7 +288,6 @@ inline_transform (struct cgraph_node *no
 {
   unsigned int todo = 0;
   struct cgraph_edge *e;
-  bool inline_p = false;

   /* FIXME: Currently the pass manager is adding inline transform more than
      once to some clones.  This needs revisiting after WPA cleanups.  */
@@ -301,18 +300,12 @@ inline_transform (struct cgraph_node *no
     save_inline_function_body (node);

   for (e = node->callees; e; e = e->next_callee)
-    {
-      cgraph_redirect_edge_call_stmt_to_callee (e);
-      if (!e->inline_failed || warn_inline)
-        inline_p = true;
-    }
+    cgraph_redirect_edge_call_stmt_to_callee (e);
+
+  timevar_push (TV_INTEGRATION);
+  todo = optimize_inline_calls (current_function_decl);
+  timevar_pop (TV_INTEGRATION);

-  if (inline_p)
-    {
-      timevar_push (TV_INTEGRATION);
-      todo = optimize_inline_calls (current_function_decl);
-      timevar_pop (TV_INTEGRATION);
-    }
   cfun->always_inline_functions_inlined = true;
   cfun->after_inlining = true;
   return todo | execute_fixup_cfg ();

this also looks like a recently introduced regression.

I'm not sure about changing sorry () to error () (not even sure why
it was sorry in the first place ...).  Any opinion from others?

Thanks,
Richard.

> Cheers
>
> Christian
>

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Christian Bruel]

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



On 06/01/2011 12:02 PM, Richard Guenther wrote:
> On Tue, May 31, 2011 at 6:03 PM, Christian Bruel<christian.bruel@st.com>  wrote:
>>
>>
>> On 05/31/2011 11:18 AM, Richard Guenther wrote:
>>>
>>> On Tue, May 31, 2011 at 9:54 AM, Christian Bruel<christian.bruel@st.com>
>>>   wrote:
>>>>
>>>> Hello,
>>>>
>>>> The attached patch fixes a few diagnostic discrepancies for always_inline
>>>> failures.
>>>>
>>>> Illustrated by the fail_always_inline[12].c attached cases, the current
>>>> behavior is one of:
>>>>
>>>> - success (with and without -Winline), silently not honoring
>>>> always_inline
>>>>    gcc fail_always_inline1.c -S -Winline -O0 -fpic
>>>>    gcc fail_always_inline1.c -S -O2 -fpic
>>>>
>>>> - error: with -Winline but not without
>>>>    gcc fail_always_inline1.c -S -Winline -O2 -fpic
>>>>
>>>> - error: without -Winline
>>>>    gcc fail_always_inline2.c -S -fno-early-inlining -O2
>>>>    or the original c++ attachment in this defect
>>>>
>>>> note that -Winline never warns, as stated in the documentation
>>>>
>>>> This simple patch consistently emits a warning (changing the sorry
>>>> unimplemented message) whenever the attribute is not honored.
>>>>
>>>> My first will was to generate and error instead of the warning, but since
>>>> it
>>>> is possible that inlines is only performed at LTO time, an error would be
>>>> inapropriate (Note that today this is not possible with -Winline that
>>>> would
>>>> abort).
>>>>
>>>> Another alternative I considered would be to emit the warning under
>>>> -Winline
>>>> rather than unconditionally, but this more a user misuse of the
>>>> attribute,
>>>> so should always be warned anyway. Or maybe a new -Winline-always that
>>>> would
>>>> be activated under -Wall ? Other opinion welcomed.
>>>>
>>>> Tested with standard bootstrap and regression on x86.
>>>>
>>>> Comments, and/or OK for trunk ?
>>>
>>> The patch is not ok, we may not fail to inline an always_inline
>>> function.
>>
>> OK, I thought so that would be an error. but I was afraid to abort the
>> inline of function with a missing body (provided by another file) by LTO,
>> which would be a regression. rethinking about this and considering that a
>> valid LTO program should be valid without LTO, and that the scope is the
>> translation unit, that would be OK to always reject attribute_inline on
>> functions without a body.
>>
>> To make this more consistent I proposed to warn
>>>
>>> whenever you take the address of an always_inline function
>>> (because then you can confuse GCC by indirectly calling
>>> such function which we might inline dependent on optimization
>>> setting and which we might discover we didn't inline only
>>> dependent on optimization setting).Honza proposed to move
>>> the sorry()ing to when we feel the need to output the
>>> always_inline function, thus when it was not optimized away,
>>> but that would require us not preserving the body (do we?)
>>> with -fpreserve-inline-functions.
>>>
>>
>> But we don't know if taking the address of the function will result by a
>> call to it, or how the pointer will be used. So I think the check should be
>> done at the caller site. But I code like:
>>
>> inline __attribute__((always_inline))  int foo() { return 0; }
>>
>> static int (*ptr)() = foo;
>>
>> int
>> bar()
>> {
>>   return ptr();
>> }
>>
>> is not be (legitimately) inlined, but without diagnostic, I don't know where
>> to look at this that at the moment.
>
> Yeah, the issue is that we only warn if we end up seeing a direct
> call to an always_inline function that wasn't inlined.  The idea of
> sorrying() for remaining always_inline bodies instead would also
> catch the above, but so would
>
> inline __attribute__((always_inline))  int foo() { return 0; }
> int (*ptr)() = foo;
>
> (address-taken but not called).
>
>>> For fail_always_inline1.c we should diagnose the appearant
>>> misuse of always_inline with a warning, drop the attribute
>>> but keep DECL_DISREGARD_INLINE_LIMITS set.
>>>
>>> Same for fail_always_inline2.c.
>>>
>>> I agree that sorry()ing for those cases is odd.  EIther we
>>> should reject the declarations upfront
>>> ("always-inline function will not be inlinable"), or we should
>>> emit a warning of that kind and make sure to not sorry later.
>>
>> In addition to the error at the caller site, I've added the specific warn
>> about the missing inline keyword.
>
> But not in a good place.  Please instead check it alongside the
> other attribute checks in cgraphunit.c:process_function_and_variable_attributes

OK, the only difference is that we don't have the node analyzed here, so 
externally_visible, etc are not set. With the initial proposal the 
warning was emitted only if the function could not be inlined. Now it 
will be emitted for each  DECL_COMDAT (decl) && !DECL_DECLARED_INLINE_P 
(decl)) even if not preemptible, so conservatively we don't want to 
duplicate the availability check.

see attached new patch for that.


>
>> Thanks for your comments, here is the new patch that I'm testing, (without
>> the handling of indirect calls which can be treated separately).
>
> Index: gcc/ipa-inline-transform.c
> ===================================================================
> --- gcc/ipa-inline-transform.c	(revision 174264)
> +++ gcc/ipa-inline-transform.c	(working copy)
> @@ -302,9 +302,20 @@
>
>     for (e = node->callees; e; e = e->next_callee)
>       {
> -      cgraph_redirect_edge_call_stmt_to_callee (e);
> +      gimple call = cgraph_redirect_edge_call_stmt_to_callee (e);
> +
> +      if (!inline_p)
> +	{
>         if (!e->inline_failed || warn_inline)
>           inline_p = true;
> +	  else
> +	    {
> +	      tree fn = gimple_call_fndecl (call);
> +	
> +	      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (fn)))
> +		inline_p = true;
> +	    }
> +	}
>       }
>
>     if (inline_p)
>
> Honza - this conditional calling of optimize_inline_calls just if
> warn_inline is on is extra ugly.  Does it really save that much
> time to only conditionally run optimize_inline_calls?  If so
> we should re-write that function completely.
>

I fully agree, and this avoids the double check for the 
inline_attribute. However one alternative could be to promote the error 
checking from expand_call_inline in inline_transform only in Winline or 
always_inline.

> Christian, for now I suggest to instead do
>
> Index: ipa-inline-transform.c
> ===================================================================
> --- ipa-inline-transform.c      (revision 174520)
> +++ ipa-inline-transform.c      (working copy)
> @@ -288,7 +288,6 @@ inline_transform (struct cgraph_node *no
>   {
>     unsigned int todo = 0;
>     struct cgraph_edge *e;
> -  bool inline_p = false;
>
>     /* FIXME: Currently the pass manager is adding inline transform more than
>        once to some clones.  This needs revisiting after WPA cleanups.  */
> @@ -301,18 +300,12 @@ inline_transform (struct cgraph_node *no
>       save_inline_function_body (node);
>
>     for (e = node->callees; e; e = e->next_callee)
> -    {
> -      cgraph_redirect_edge_call_stmt_to_callee (e);
> -      if (!e->inline_failed || warn_inline)
> -        inline_p = true;
> -    }
> +    cgraph_redirect_edge_call_stmt_to_callee (e);
> +
> +  timevar_push (TV_INTEGRATION);
> +  todo = optimize_inline_calls (current_function_decl);
> +  timevar_pop (TV_INTEGRATION);
>
> -  if (inline_p)
> -    {
> -      timevar_push (TV_INTEGRATION);
> -      todo = optimize_inline_calls (current_function_decl);
> -      timevar_pop (TV_INTEGRATION);
> -    }
>     cfun->always_inline_functions_inlined = true;
>     cfun->after_inlining = true;
>     return todo | execute_fixup_cfg ();
>
> this also looks like a recently introduced regression.
>
> I'm not sure about changing sorry () to error () (not even sure why
> it was sorry in the first place ...).  Any opinion from others?

given that the sorry was emitted under -Winline, that was even more 
confusing.

>
> Thanks,
> Richard.
>
>> Cheers
>>
>> Christian
>>
>

[-- Attachment #2: always_inline_warn.patch --]
[-- Type: text/plain, Size: 4617 bytes --]

Index: gcc/cgraph.c
===================================================================
Index: gcc/ipa-inline-transform.c
===================================================================
--- gcc/ipa-inline-transform.c	(revision 174264)
+++ gcc/ipa-inline-transform.c	(working copy)
@@ -288,7 +288,6 @@
 {
   unsigned int todo = 0;
   struct cgraph_edge *e;
-  bool inline_p = false;
 
   /* FIXME: Currently the pass manager is adding inline transform more than
      once to some clones.  This needs revisiting after WPA cleanups.  */
@@ -301,18 +300,12 @@
     save_inline_function_body (node);
 
   for (e = node->callees; e; e = e->next_callee)
-    {
       cgraph_redirect_edge_call_stmt_to_callee (e);
-      if (!e->inline_failed || warn_inline)
-        inline_p = true;
-    }
 
-  if (inline_p)
-    {
       timevar_push (TV_INTEGRATION);
       todo = optimize_inline_calls (current_function_decl);
       timevar_pop (TV_INTEGRATION);
-    }
+
   cfun->always_inline_functions_inlined = true;
   cfun->after_inlining = true;
   return todo | execute_fixup_cfg ();
Index: gcc/cgraphunit.c
===================================================================
--- gcc/cgraphunit.c	(revision 174264)
+++ gcc/cgraphunit.c	(working copy)
@@ -900,6 +900,13 @@
 	  DECL_ATTRIBUTES (decl) = remove_attribute ("weakref",
 						     DECL_ATTRIBUTES (decl));
 	}
+      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (decl)))
+	{
+	  if (! DECL_COMDAT (decl) && !DECL_DECLARED_INLINE_P (decl))
+	    warning (OPT_Wattributes,
+		     "always_inline function might not be inlinable");
+	}
+
       process_common_attributes (decl);
     }
   for (vnode = varpool_nodes; vnode != first_var; vnode = vnode->next)
Index: gcc/testsuite/gcc.dg/always_inline.c
===================================================================
--- gcc/testsuite/gcc.dg/always_inline.c	(revision 174264)
+++ gcc/testsuite/gcc.dg/always_inline.c	(working copy)
@@ -1,8 +1,7 @@
 /* { dg-do compile } */
-/* { dg-options "-Winline -O2" } */
 #include <stdarg.h>
 inline __attribute__ ((always_inline)) void
-e(int t, ...) /* { dg-message "sorry\[^\n\]*variable argument" "" } */
+e(int t, ...) /* { dg-error "variable argument" } */
 {
   va_list q;
   va_start (q, t);
Index: gcc/testsuite/gcc.dg/always_inline2.c
===================================================================
--- gcc/testsuite/gcc.dg/always_inline2.c	(revision 174264)
+++ gcc/testsuite/gcc.dg/always_inline2.c	(working copy)
@@ -1,8 +1,7 @@
 /* { dg-do compile } */
-/* { dg-options "-Winline -O2" } */
-inline __attribute__ ((always_inline)) void t(void); /* { dg-message "sorry\[^\n\]*body not available" "" } */
+inline __attribute__ ((always_inline)) void t(void); /* { dg-error "body not available" } */
 void
 q(void)
 {
-  t(); 				/* { dg-message "sorry\[^\n\]*called from here" "" } */
+  t(); 				/* { dg-error "called from here" } */
 }
Index: gcc/testsuite/gcc.dg/always_inline3.c
===================================================================
--- gcc/testsuite/gcc.dg/always_inline3.c	(revision 174264)
+++ gcc/testsuite/gcc.dg/always_inline3.c	(working copy)
@@ -1,11 +1,11 @@
 /* { dg-do compile } */
-/* { dg-options "-Winline -O2" } */
+/* { dg-options "-O2" } */
 int do_something_evil (void);
 inline __attribute__ ((always_inline)) void
-q2(void) /* { dg-message "sorry\[^\n\]*recursive" "" } */
+q2(void) /* { dg-error "recursive inlining" } */
 {
   if (do_something_evil ())
     return;
-  q2(); 			/* { dg-message "sorry\[^\n\]*called from here" "" } */
+  q2(); 			/* { dg-error "called from here" } */
   q2(); /* With -O2 we don't warn here, it is eliminated by tail recursion.  */
 }
Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c	(revision 174264)
+++ gcc/tree-inline.c	(working copy)
@@ -3192,7 +3192,7 @@
 	 As a bonus we can now give more details about the reason why a
 	 function is not inlinable.  */
       if (always_inline)
-	sorry (inline_forbidden_reason, fn);
+	error (inline_forbidden_reason, fn);
       else if (do_warning)
 	warning (OPT_Winline, inline_forbidden_reason, fn);
 
@@ -3744,9 +3744,9 @@
 	  /* Avoid warnings during early inline pass. */
 	  && cgraph_global_info_ready)
 	{
-	  sorry ("inlining failed in call to %q+F: %s", fn,
-		 _(cgraph_inline_failed_string (reason)));
-	  sorry ("called from here");
+	  error ("inlining failed in call to always_inline %q+F: %s", fn,
+		 cgraph_inline_failed_string (reason));
+	  error ("called from here");
 	}
       else if (warn_inline
 	       && DECL_DECLARED_INLINE_P (fn)

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Richard Guenther]

On Wed, Jun 1, 2011 at 3:01 PM, Christian Bruel <christian.bruel@st.com> wrote:
>
>
> On 06/01/2011 12:02 PM, Richard Guenther wrote:
>>
>> On Tue, May 31, 2011 at 6:03 PM, Christian Bruel<christian.bruel@st.com>
>>  wrote:
>>>
>>>
>>> On 05/31/2011 11:18 AM, Richard Guenther wrote:
>>>>
>>>> On Tue, May 31, 2011 at 9:54 AM, Christian Bruel<christian.bruel@st.com>
>>>>  wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> The attached patch fixes a few diagnostic discrepancies for
>>>>> always_inline
>>>>> failures.
>>>>>
>>>>> Illustrated by the fail_always_inline[12].c attached cases, the current
>>>>> behavior is one of:
>>>>>
>>>>> - success (with and without -Winline), silently not honoring
>>>>> always_inline
>>>>>   gcc fail_always_inline1.c -S -Winline -O0 -fpic
>>>>>   gcc fail_always_inline1.c -S -O2 -fpic
>>>>>
>>>>> - error: with -Winline but not without
>>>>>   gcc fail_always_inline1.c -S -Winline -O2 -fpic
>>>>>
>>>>> - error: without -Winline
>>>>>   gcc fail_always_inline2.c -S -fno-early-inlining -O2
>>>>>   or the original c++ attachment in this defect
>>>>>
>>>>> note that -Winline never warns, as stated in the documentation
>>>>>
>>>>> This simple patch consistently emits a warning (changing the sorry
>>>>> unimplemented message) whenever the attribute is not honored.
>>>>>
>>>>> My first will was to generate and error instead of the warning, but
>>>>> since
>>>>> it
>>>>> is possible that inlines is only performed at LTO time, an error would
>>>>> be
>>>>> inapropriate (Note that today this is not possible with -Winline that
>>>>> would
>>>>> abort).
>>>>>
>>>>> Another alternative I considered would be to emit the warning under
>>>>> -Winline
>>>>> rather than unconditionally, but this more a user misuse of the
>>>>> attribute,
>>>>> so should always be warned anyway. Or maybe a new -Winline-always that
>>>>> would
>>>>> be activated under -Wall ? Other opinion welcomed.
>>>>>
>>>>> Tested with standard bootstrap and regression on x86.
>>>>>
>>>>> Comments, and/or OK for trunk ?
>>>>
>>>> The patch is not ok, we may not fail to inline an always_inline
>>>> function.
>>>
>>> OK, I thought so that would be an error. but I was afraid to abort the
>>> inline of function with a missing body (provided by another file) by LTO,
>>> which would be a regression. rethinking about this and considering that a
>>> valid LTO program should be valid without LTO, and that the scope is the
>>> translation unit, that would be OK to always reject attribute_inline on
>>> functions without a body.
>>>
>>> To make this more consistent I proposed to warn
>>>>
>>>> whenever you take the address of an always_inline function
>>>> (because then you can confuse GCC by indirectly calling
>>>> such function which we might inline dependent on optimization
>>>> setting and which we might discover we didn't inline only
>>>> dependent on optimization setting).Honza proposed to move
>>>> the sorry()ing to when we feel the need to output the
>>>> always_inline function, thus when it was not optimized away,
>>>> but that would require us not preserving the body (do we?)
>>>> with -fpreserve-inline-functions.
>>>>
>>>
>>> But we don't know if taking the address of the function will result by a
>>> call to it, or how the pointer will be used. So I think the check should
>>> be
>>> done at the caller site. But I code like:
>>>
>>> inline __attribute__((always_inline))  int foo() { return 0; }
>>>
>>> static int (*ptr)() = foo;
>>>
>>> int
>>> bar()
>>> {
>>>  return ptr();
>>> }
>>>
>>> is not be (legitimately) inlined, but without diagnostic, I don't know
>>> where
>>> to look at this that at the moment.
>>
>> Yeah, the issue is that we only warn if we end up seeing a direct
>> call to an always_inline function that wasn't inlined.  The idea of
>> sorrying() for remaining always_inline bodies instead would also
>> catch the above, but so would
>>
>> inline __attribute__((always_inline))  int foo() { return 0; }
>> int (*ptr)() = foo;
>>
>> (address-taken but not called).
>>
>>>> For fail_always_inline1.c we should diagnose the appearant
>>>> misuse of always_inline with a warning, drop the attribute
>>>> but keep DECL_DISREGARD_INLINE_LIMITS set.
>>>>
>>>> Same for fail_always_inline2.c.
>>>>
>>>> I agree that sorry()ing for those cases is odd.  EIther we
>>>> should reject the declarations upfront
>>>> ("always-inline function will not be inlinable"), or we should
>>>> emit a warning of that kind and make sure to not sorry later.
>>>
>>> In addition to the error at the caller site, I've added the specific warn
>>> about the missing inline keyword.
>>
>> But not in a good place.  Please instead check it alongside the
>> other attribute checks in
>> cgraphunit.c:process_function_and_variable_attributes
>
> OK, the only difference is that we don't have the node analyzed here, so
> externally_visible, etc are not set. With the initial proposal the warning
> was emitted only if the function could not be inlined. Now it will be
> emitted for each  DECL_COMDAT (decl) && !DECL_DECLARED_INLINE_P (decl)) even
> if not preemptible, so conservatively we don't want to duplicate the
> availability check.

Hm, I'm confused.  Do all DECL_COMDAT functions have the
always_inline attribute set?  I would have expected

+      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (decl)))
+	{
+	  if (!DECL_DECLARED_INLINE_P (decl))
+	    warning (OPT_Wattributes,
+		     "always_inline not declared inline might not be inlinable");
+	}

do you get excessive warnings with this?

> see attached new patch for that.
>
>
>>
>>> Thanks for your comments, here is the new patch that I'm testing,
>>> (without
>>> the handling of indirect calls which can be treated separately).
>>
>> Index: gcc/ipa-inline-transform.c
>> ===================================================================
>> --- gcc/ipa-inline-transform.c  (revision 174264)
>> +++ gcc/ipa-inline-transform.c  (working copy)
>> @@ -302,9 +302,20 @@
>>
>>    for (e = node->callees; e; e = e->next_callee)
>>      {
>> -      cgraph_redirect_edge_call_stmt_to_callee (e);
>> +      gimple call = cgraph_redirect_edge_call_stmt_to_callee (e);
>> +
>> +      if (!inline_p)
>> +       {
>>        if (!e->inline_failed || warn_inline)
>>          inline_p = true;
>> +         else
>> +           {
>> +             tree fn = gimple_call_fndecl (call);
>> +
>> +             if (lookup_attribute ("always_inline", DECL_ATTRIBUTES
>> (fn)))
>> +               inline_p = true;
>> +           }
>> +       }
>>      }
>>
>>    if (inline_p)
>>
>> Honza - this conditional calling of optimize_inline_calls just if
>> warn_inline is on is extra ugly.  Does it really save that much
>> time to only conditionally run optimize_inline_calls?  If so
>> we should re-write that function completely.
>>
>
> I fully agree, and this avoids the double check for the inline_attribute.
> However one alternative could be to promote the error checking from
> expand_call_inline in inline_transform only in Winline or always_inline.
>
>> Christian, for now I suggest to instead do
>>
>> Index: ipa-inline-transform.c
>> ===================================================================
>> --- ipa-inline-transform.c      (revision 174520)
>> +++ ipa-inline-transform.c      (working copy)
>> @@ -288,7 +288,6 @@ inline_transform (struct cgraph_node *no
>>  {
>>    unsigned int todo = 0;
>>    struct cgraph_edge *e;
>> -  bool inline_p = false;
>>
>>    /* FIXME: Currently the pass manager is adding inline transform more
>> than
>>       once to some clones.  This needs revisiting after WPA cleanups.  */
>> @@ -301,18 +300,12 @@ inline_transform (struct cgraph_node *no
>>      save_inline_function_body (node);
>>
>>    for (e = node->callees; e; e = e->next_callee)
>> -    {
>> -      cgraph_redirect_edge_call_stmt_to_callee (e);
>> -      if (!e->inline_failed || warn_inline)
>> -        inline_p = true;
>> -    }
>> +    cgraph_redirect_edge_call_stmt_to_callee (e);
>> +
>> +  timevar_push (TV_INTEGRATION);
>> +  todo = optimize_inline_calls (current_function_decl);
>> +  timevar_pop (TV_INTEGRATION);
>>
>> -  if (inline_p)
>> -    {
>> -      timevar_push (TV_INTEGRATION);
>> -      todo = optimize_inline_calls (current_function_decl);
>> -      timevar_pop (TV_INTEGRATION);
>> -    }
>>    cfun->always_inline_functions_inlined = true;
>>    cfun->after_inlining = true;
>>    return todo | execute_fixup_cfg ();
>>
>> this also looks like a recently introduced regression.
>>
>> I'm not sure about changing sorry () to error () (not even sure why
>> it was sorry in the first place ...).  Any opinion from others?
>
> given that the sorry was emitted under -Winline, that was even more
> confusing.

Yeah, I can imagine that - I only noticed this when looking at your
patch context.

Thanks,
Richard

>>
>> Thanks,
>> Richard.
>>
>>> Cheers
>>>
>>> Christian
>>>
>>
>

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Jan Hubicka]

> >>
> >> whenever you take the address of an always_inline function
> >> (because then you can confuse GCC by indirectly calling
> >> such function which we might inline dependent on optimization
> >> setting and which we might discover we didn't inline only
> >> dependent on optimization setting).Honza proposed to move
> >> the sorry()ing to when we feel the need to output the
> >> always_inline function, thus when it was not optimized away,
> >> but that would require us not preserving the body (do we?)
> >> with -fpreserve-inline-functions.

I don't think we can preserve them with -fpreserve-inline-functions because
the ssa intrincisc or functions calling va_arg_pack can not be expanded
when not inlined into proper context and having those in headers would imply
units using those headers to not compile with -fpreserve-inline-functions...

> 
> Honza - this conditional calling of optimize_inline_calls just if
> warn_inline is on is extra ugly.  Does it really save that much
> time to only conditionally run optimize_inline_calls?  If so
> we should re-write that function completely.

I don't think it is big deal to call that function each time.  It used
to be more expensive than it is now and the conditional was there just
because it was always there as far as I recall.

Honza

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Christian Bruel]

>> OK, the only difference is that we don't have the node analyzed here, so
>> externally_visible, etc are not set. With the initial proposal the warning
>> was emitted only if the function could not be inlined. Now it will be
>> emitted for each  DECL_COMDAT (decl)&&  !DECL_DECLARED_INLINE_P (decl)) even
>> if not preemptible, so conservatively we don't want to duplicate the
>> availability check.
>
> Hm, I'm confused.  Do all DECL_COMDAT functions have the
> always_inline attribute set?  I would have expected
>
> +      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (decl)))
> +	{
> +	  if (!DECL_DECLARED_INLINE_P (decl))
> +	    warning (OPT_Wattributes,
> +		     "always_inline not declared inline might not be inlinable");
> +	}
>

I meant !DECL_COMDAT || !DECL_DECLARED_INLINE_P. but I was 
overprecautious. Didn't realize that member functions was already marked 
with DECLARED_INLINED_P even if not explicitly set. So OK one check is 
enough

> do you get excessive warnings with this?
>

No I don't. That's enough to catch the original issue

Cheers

Christian

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Richard Guenther]

On Mon, Jun 6, 2011 at 10:58 AM, Christian Bruel <christian.bruel@st.com> wrote:
>
>
>>> OK, the only difference is that we don't have the node analyzed here, so
>>> externally_visible, etc are not set. With the initial proposal the
>>> warning
>>> was emitted only if the function could not be inlined. Now it will be
>>> emitted for each  DECL_COMDAT (decl)&&  !DECL_DECLARED_INLINE_P (decl))
>>> even
>>> if not preemptible, so conservatively we don't want to duplicate the
>>> availability check.
>>
>> Hm, I'm confused.  Do all DECL_COMDAT functions have the
>> always_inline attribute set?  I would have expected
>>
>> +      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (decl)))
>> +       {
>> +         if (!DECL_DECLARED_INLINE_P (decl))
>> +           warning (OPT_Wattributes,
>> +                    "always_inline not declared inline might not be
>> inlinable");
>> +       }
>>
>
> I meant !DECL_COMDAT || !DECL_DECLARED_INLINE_P. but I was overprecautious.
> Didn't realize that member functions was already marked with
> DECLARED_INLINED_P even if not explicitly set. So OK one check is enough
>
>> do you get excessive warnings with this?
>>
>
> No I don't. That's enough to catch the original issue

Ok.  The patch is ok with the !DECL_DECLARED_INLINE_P check
if it still passes bootstrap & regtest.

Thanks,
Richard.

> Cheers
>
> Christian
>

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Christian Bruel]

Hello Richard,

On 06/06/2011 11:55 AM, Richard Guenther wrote:
> On Mon, Jun 6, 2011 at 10:58 AM, Christian Bruel<christian.bruel@st.com>  wrote:
>>
>>
>>>> OK, the only difference is that we don't have the node analyzed here, so
>>>> externally_visible, etc are not set. With the initial proposal the
>>>> warning
>>>> was emitted only if the function could not be inlined. Now it will be
>>>> emitted for each  DECL_COMDAT (decl)&&    !DECL_DECLARED_INLINE_P (decl))
>>>> even
>>>> if not preemptible, so conservatively we don't want to duplicate the
>>>> availability check.
>>>
>>> Hm, I'm confused.  Do all DECL_COMDAT functions have the
>>> always_inline attribute set?  I would have expected
>>>
>>> +      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (decl)))
>>> +       {
>>> +         if (!DECL_DECLARED_INLINE_P (decl))
>>> +           warning (OPT_Wattributes,
>>> +                    "always_inline not declared inline might not be
>>> inlinable");
>>> +       }
>>>
>>
>> I meant !DECL_COMDAT || !DECL_DECLARED_INLINE_P. but I was overprecautious.
>> Didn't realize that member functions was already marked with
>> DECLARED_INLINED_P even if not explicitly set. So OK one check is enough
>>
>>> do you get excessive warnings with this?
>>>
>>
>> No I don't. That's enough to catch the original issue
>

Unfortunately still not satisfactory, I've been testing it against a few 
packages, and I notice excessive warnings with the use of __typeof 
(__error) that doesn't propagate the inline keyword.

For instance, a reduced use extracted from the glibc

extern __inline __attribute__ ((__always_inline__))  void
error ()
{

}

extern void
__error ()
{
}

extern __typeof (__error) error __attribute__ ((weak, alias ("__error")));

emits an annoying warning on the error redefinition.

So, a check in addition of the DECL_DECLARED_INLINED_P is needed, 
TREE_ADDRESSABLE seems appropriate, since in the case of missing inline 
the function would be emitted. So I'm testing:

if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (decl))
	  && !DECL_DECLARED_INLINE_P (decl)
	  && TREE_ADDRESSABLE (decl))

other idea ? or should be just drop this warning ?

> Ok.  The patch is ok with the !DECL_DECLARED_INLINE_P check
> if it still passes bootstrap&  regtest.
>

thanks, for now I postpone until glibc is ok and more legacy checks.

Cheers

Christian

> Thanks,
> Richard.
>
>> Cheers
>>
>> Christian
>>
>

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Richard Guenther]

On Wed, Jun 8, 2011 at 9:46 AM, Christian Bruel <christian.bruel@st.com> wrote:
> Hello Richard,
>
> On 06/06/2011 11:55 AM, Richard Guenther wrote:
>>
>> On Mon, Jun 6, 2011 at 10:58 AM, Christian Bruel<christian.bruel@st.com>
>>  wrote:
>>>
>>>
>>>>> OK, the only difference is that we don't have the node analyzed here,
>>>>> so
>>>>> externally_visible, etc are not set. With the initial proposal the
>>>>> warning
>>>>> was emitted only if the function could not be inlined. Now it will be
>>>>> emitted for each  DECL_COMDAT (decl)&&    !DECL_DECLARED_INLINE_P
>>>>> (decl))
>>>>> even
>>>>> if not preemptible, so conservatively we don't want to duplicate the
>>>>> availability check.
>>>>
>>>> Hm, I'm confused.  Do all DECL_COMDAT functions have the
>>>> always_inline attribute set?  I would have expected
>>>>
>>>> +      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (decl)))
>>>> +       {
>>>> +         if (!DECL_DECLARED_INLINE_P (decl))
>>>> +           warning (OPT_Wattributes,
>>>> +                    "always_inline not declared inline might not be
>>>> inlinable");
>>>> +       }
>>>>
>>>
>>> I meant !DECL_COMDAT || !DECL_DECLARED_INLINE_P. but I was
>>> overprecautious.
>>> Didn't realize that member functions was already marked with
>>> DECLARED_INLINED_P even if not explicitly set. So OK one check is enough
>>>
>>>> do you get excessive warnings with this?
>>>>
>>>
>>> No I don't. That's enough to catch the original issue
>>
>
> Unfortunately still not satisfactory, I've been testing it against a few
> packages, and I notice excessive warnings with the use of __typeof (__error)
> that doesn't propagate the inline keyword.
>
> For instance, a reduced use extracted from the glibc
>
> extern __inline __attribute__ ((__always_inline__))  void
> error ()
> {
>
> }
>
> extern void
> __error ()
> {
> }
>
> extern __typeof (__error) error __attribute__ ((weak, alias ("__error")));
>
> emits an annoying warning on the error redefinition.
>
> So, a check in addition of the DECL_DECLARED_INLINED_P is needed,
> TREE_ADDRESSABLE seems appropriate, since in the case of missing inline the
> function would be emitted. So I'm testing:
>
> if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (decl))
>          && !DECL_DECLARED_INLINE_P (decl)
>          && TREE_ADDRESSABLE (decl))
>
> other idea ? or should be just drop this warning ?

Hmm.  Honza, any idea on the above?  Christian, I suppose you
could check if the cgraph node for that decl has the redefined_extern_inline
flag set (checking TREE_ADDRESSABLE looks bogus to me).
I'm not sure how the frontend merges those two decls - I suppose
it will have a weak always-inline function with body :/

Richard.

>> Ok.  The patch is ok with the !DECL_DECLARED_INLINE_P check
>> if it still passes bootstrap&  regtest.
>>
>
> thanks, for now I postpone until glibc is ok and more legacy checks.
>
> Cheers
>
> Christian
>
>> Thanks,
>> Richard.
>>
>>> Cheers
>>>
>>> Christian
>>>
>>
>

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Christian Bruel]

On 06/08/2011 11:11 AM, Richard Guenther wrote:
> On Wed, Jun 8, 2011 at 9:46 AM, Christian Bruel<christian.bruel@st.com>  wrote:
>> Hello Richard,
>>
>> On 06/06/2011 11:55 AM, Richard Guenther wrote:
>>>
>>> On Mon, Jun 6, 2011 at 10:58 AM, Christian Bruel<christian.bruel@st.com>
>>>   wrote:
>>>>
>>>>
>>>>>> OK, the only difference is that we don't have the node analyzed here,
>>>>>> so
>>>>>> externally_visible, etc are not set. With the initial proposal the
>>>>>> warning
>>>>>> was emitted only if the function could not be inlined. Now it will be
>>>>>> emitted for each  DECL_COMDAT (decl)&&      !DECL_DECLARED_INLINE_P
>>>>>> (decl))
>>>>>> even
>>>>>> if not preemptible, so conservatively we don't want to duplicate the
>>>>>> availability check.
>>>>>
>>>>> Hm, I'm confused.  Do all DECL_COMDAT functions have the
>>>>> always_inline attribute set?  I would have expected
>>>>>
>>>>> +      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (decl)))
>>>>> +       {
>>>>> +         if (!DECL_DECLARED_INLINE_P (decl))
>>>>> +           warning (OPT_Wattributes,
>>>>> +                    "always_inline not declared inline might not be
>>>>> inlinable");
>>>>> +       }
>>>>>
>>>>
>>>> I meant !DECL_COMDAT || !DECL_DECLARED_INLINE_P. but I was
>>>> overprecautious.
>>>> Didn't realize that member functions was already marked with
>>>> DECLARED_INLINED_P even if not explicitly set. So OK one check is enough
>>>>
>>>>> do you get excessive warnings with this?
>>>>>
>>>>
>>>> No I don't. That's enough to catch the original issue
>>>
>>
>> Unfortunately still not satisfactory, I've been testing it against a few
>> packages, and I notice excessive warnings with the use of __typeof (__error)
>> that doesn't propagate the inline keyword.
>>
>> For instance, a reduced use extracted from the glibc
>>
>> extern __inline __attribute__ ((__always_inline__))  void
>> error ()
>> {
>>
>> }
>>
>> extern void
>> __error ()
>> {
>> }
>>
>> extern __typeof (__error) error __attribute__ ((weak, alias ("__error")));
>>
>> emits an annoying warning on the error redefinition.
>>
>> So, a check in addition of the DECL_DECLARED_INLINED_P is needed,
>> TREE_ADDRESSABLE seems appropriate, since in the case of missing inline the
>> function would be emitted. So I'm testing:
>>
>> if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (decl))
>>           &&  !DECL_DECLARED_INLINE_P (decl)
>>           &&  TREE_ADDRESSABLE (decl))
>>
>> other idea ? or should be just drop this warning ?
>
> Hmm.  Honza, any idea on the above?  Christian, I suppose you
> could check if the cgraph node for that decl has the redefined_extern_inline
> flag set (checking TREE_ADDRESSABLE looks bogus to me).
> I'm not sure how the frontend merges those two decls - I suppose
> it will have a weak always-inline function with body :/
>

ooops, yes, TREE_ADDRESSABLE was in 4.5 :
    "In a FUNCTION_DECL, nonzero means its address is needed.
    So it must be compiled even if it is an inline function."
but indeed in 4.6 and above is is:
    "In a FUNCTION_DECL it has no meaning."

so can't use TREE_ADDRESSABLE, (I'm also patching the 4.5 locally). I'll 
check if redefined_extern_value does the job.

thanks

Christian

> Richard.
>
>>> Ok.  The patch is ok with the !DECL_DECLARED_INLINE_P check
>>> if it still passes bootstrap&    regtest.
>>>
>>
>> thanks, for now I postpone until glibc is ok and more legacy checks.
>>
>> Cheers
>>
>> Christian
>>
>>> Thanks,
>>> Richard.
>>>
>>>> Cheers
>>>>
>>>> Christian
>>>>
>>>
>>
>

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Christian Bruel]

>>
>> Unfortunately still not satisfactory, I've been testing it against a few
>> packages, and I notice excessive warnings with the use of __typeof (__error)
>> that doesn't propagate the inline keyword.
>>
>> For instance, a reduced use extracted from the glibc
>>
>> extern __inline __attribute__ ((__always_inline__))  void
>> error ()
>> {
>>
>> }
>>
>> extern void
>> __error ()
>> {
>> }
>>
>> extern __typeof (__error) error __attribute__ ((weak, alias ("__error")));
>>
>> emits an annoying warning on the error redefinition.
>>
>> So, a check in addition of the DECL_DECLARED_INLINED_P is needed,
>> TREE_ADDRESSABLE seems appropriate, since in the case of missing inline the
>> function would be emitted. So I'm testing:
>>
>> if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (decl))
>>           &&  !DECL_DECLARED_INLINE_P (decl)
>>           &&  TREE_ADDRESSABLE (decl))
>>
>> other idea ? or should be just drop this warning ?
>
> Hmm.  Honza, any idea on the above?  Christian, I suppose you
> could check if the cgraph node for that decl has the redefined_extern_inline
> flag set (checking TREE_ADDRESSABLE looks bogus to me).
> I'm not sure how the frontend merges those two decls - I suppose
> it will have a weak always-inline function with body :/

redefined_extern_inline is not set here. But DECL_STRUCT_FUNCTION(decl)) 
seems even best since it makes no sense to try the inline if the body is 
not available. So the former works well here.

Now, another annoying case, that I reduced from Xorg packages built from 
the glibc with -fstack-protector-all -D_FORTIFY_SOURCE=2.

to the following code:
---------------------------------
#include <stdlib.h>

char *
realpath(path, resolved)
         const char *path;
         char *resolved;
{
    ...
    return (NULL);
}
--------------------------------
preprocesses as:

extern char *__realpath_alias (__const char *__restrict __name, char 
*__restrict __resolved) __asm__ ("" "realpath") __attribute__ 
((__nothrow__)) __attribute__ ((__warn_unused_result__));

extern __inline __attribute__ ((__always_inline__)) __attribute__ 
((__artificial__)) __attribute__ ((__warn_unused_result__)) char *
__attribute__ ((__nothrow__)) realpath (__const char *__restrict __name, 
char *__restrict __resolved)
{
   return __realpath_alias (__name, __resolved);
}

char *
realpath(path, resolved)
  const char *path;
  char *resolved;
{
  return (((void *)0));
}

---

The problem is that the second redefinition, inherits the attributes of 
from the first extern inline declaration. So we get the warning emitted 
for this one..

It should be fine to redefine an extern inline function. is it ? So it 
would be a frontend bug to not reset the attributes when meeting the 
redefinition. Unless the first definition is an declaration and the 
attribute applies to all.

I also thought to test for the attribute artificial before emitting the 
warning, but that doesn't look correct, since this is only use from 
debugging information.

So the question is : is the redefinition of an extern inline function OK 
(I think yes), and should it inherit the attribute of the first one ?

Any idea ?

Many thanks

Christian


>
> Richard.

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Christian Bruel]

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

In addition to the approved part of the patch, I finally got the 
testsuite and a full Linux distribution to build without false warnings, 
with the additional following changes:

* two false warnings detected during a Linux distrib rebuild

   - __typeof (__error) doesn't propagate the inline keyword. even if 
the function had attribute always_inline.
   - extern inline function redefinition would inherit the attribute, 
even if they are not inline

   DECL_UNINLINABLE is set for redefining extern inline functions. 
(commented in c-decl.c:2340). So indeed testing this flags in the 
cgraphunit.c part fixed both.
  I checked that this doesn't conflict with other DECL_UNINLINABLE 
warning emitted from ree_inlinable_function_p. (which is called after)

* One silent bug revealed, undetected in the GCC testuite:

   - PR43564.c compiled in -O0, the "__clz" function was silently not 
inlined. Fixed in ipa-inline.c:inline_forbidden_p_stmt

* LTO pitfall: It's OK to not have a body to inline when generating the 
LTO Gimple. Enforced by PR20090218-1_0.c

* In addition to the parts of the patch that was approved, I had to fix 
the tests that didn't use the inline keyword but enforced the 
always_inline attribute to pass without the warning
  I choose to add -Wno-attributes to the option list. Could as well fix 
the test by adding the inline keyword, but I preferred to not modify the 
original test when possible.

* A minor code optimization in ipa-inline-transform.c: Don't call 
optimize_inline_calls if there is not callee.

Comments, OK to apply ?

Many thanks

Christian

2011-06-16  Christian Bruel  <christian.bruel@st.com>
	
	PR 49139/43654
	* cgraphunit.c (process_function_and_variable_attributes): warn
	for always_inline functions that are not inline.
	* ipa-inline-transform.c (inline_transform): Always call
	optimize_inline.
	* ipa-inline.c (can_inline_edge_p): Check always_inline.
	* tree-inline.c (tree_inlinable_function_p): Call error instead
	of sorry.
	(expand_call_inline): Likewise.

2011-06-16  Christian Bruel  <christian.bruel@st.com>
	
	* gcc.dg/always_inline.c: Removed -Winline. Update checks
	* gcc.dg/always_inline2.c: Likewise.
	* gcc.dg/always_inline3.c: Likewise.
	* gcc.dg/debug/pr41264-1.c: Add -Wno-attributes.
	* gcc.dg/inline_1.c: Likewise.
	* gcc.dg/inline_2.c: Likewise.
	* gcc.dg/inline_3.c: Likewise.
	* gcc.dg/inline_4.c: Likewise.
	* gcc.dg/20051201-1.c: Likewise.
	* gcc.dg/torture/pta-structcopy-1.c: Likewise.
	* gcc.dg/inline-22.c: Likewise.
	* gcc.dg/lto/20090218-1_0.c: Set inline keyword.
	* gcc.dg/lto/20090218-1_1.c: Likewise.
	* g++.dg/ipa/devirt-7.C: Likewise.
	* gcc.dg/uninit-pred-5_a.c: Likewise.
	* gcc.dg/uninit-pred-5_b.c: Likewise.
	* gcc.dg/fail_always_inline.c: New.

[-- Attachment #2: always_inline.patch --]
[-- Type: text/plain, Size: 4345 bytes --]

2011-06-16  Christian Bruel  <christian.bruel@st.com>
	
	PR 49139/43654
	* cgraphunit.c (process_function_and_variable_attributes): warn when
	always_inline functions that are not inline.
	* ipa-inline-transform.c (inline_transform): Always call optimize_inline.
	* ipa-inline.c (can_inline_edge_p): Check always_inline.
	* tree-inline.c (tree_inlinable_function_p): Use error instead of sorry.
	(expand_call_inline): Likewise.
	
Index: ipa-inline-transform.c
===================================================================
--- ipa-inline-transform.c	(revision 175201)
+++ ipa-inline-transform.c	(working copy)
@@ -348,8 +348,7 @@
 {
   unsigned int todo = 0;
   struct cgraph_edge *e;
-  bool inline_p = false;
-
+ 
   /* FIXME: Currently the pass manager is adding inline transform more than
      once to some clones.  This needs revisiting after WPA cleanups.  */
   if (cfun->after_inlining)
@@ -361,20 +360,17 @@
     save_inline_function_body (node);
 
   for (e = node->callees; e; e = e->next_callee)
+    cgraph_redirect_edge_call_stmt_to_callee (e);
+
+  timevar_push (TV_INTEGRATION);
+  if (node->callees)
     {
-      cgraph_redirect_edge_call_stmt_to_callee (e);
-      if (!e->inline_failed || warn_inline)
-        inline_p = true;
       /* Redirecting edges might lead to a need for vops to be recomputed.  */
       todo |= TODO_update_ssa_only_virtuals;
-    }
-
-  if (inline_p)
-    {
-      timevar_push (TV_INTEGRATION);
       todo = optimize_inline_calls (current_function_decl);
-      timevar_pop (TV_INTEGRATION);
     }
+  timevar_pop (TV_INTEGRATION);
+
   cfun->always_inline_functions_inlined = true;
   cfun->after_inlining = true;
   return todo | execute_fixup_cfg ();
Index: cgraphunit.c
===================================================================
--- cgraphunit.c	(revision 175201)
+++ cgraphunit.c	(working copy)
@@ -986,6 +986,14 @@
 	  DECL_ATTRIBUTES (decl) = remove_attribute ("weakref",
 						     DECL_ATTRIBUTES (decl));
 	}
+
+      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (decl))
+	  && !DECL_DECLARED_INLINE_P (decl)
+	  /* redefining always_inline function makes it DECL_UNINLINABLE.  */
+	  && !DECL_UNINLINABLE (decl))
+	warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes,
+		    "always_inline function might not be inlinable");
+     
       process_common_attributes (decl);
     }
   for (vnode = varpool_nodes; vnode != first_var; vnode = vnode->next)
Index: ipa-inline.c
===================================================================
--- ipa-inline.c	(revision 175201)
+++ ipa-inline.c	(working copy)
@@ -318,8 +318,10 @@
 			     ? callee_tree
 			     : optimization_default_node);
 
-      if ((caller_opt->x_optimize > callee_opt->x_optimize)
-	  || (caller_opt->x_optimize_size != callee_opt->x_optimize_size))
+      if (((caller_opt->x_optimize > callee_opt->x_optimize)
+	   || (caller_opt->x_optimize_size != callee_opt->x_optimize_size))
+	  /* gcc.dg/pr43564.c.  look at forced inline even in -O0.  */
+	  && !lookup_attribute ("always_inline", DECL_ATTRIBUTES (e->callee->decl)))
 	{
           e->inline_failed = CIF_TARGET_OPTIMIZATION_MISMATCH;
 	  inlinable = false;
Index: tree-inline.c
===================================================================
--- tree-inline.c	(revision 175201)
+++ tree-inline.c	(working copy)
@@ -3192,7 +3192,7 @@
 	 As a bonus we can now give more details about the reason why a
 	 function is not inlinable.  */
       if (always_inline)
-	sorry (inline_forbidden_reason, fn);
+	error (inline_forbidden_reason, fn);
       else if (do_warning)
 	warning (OPT_Winline, inline_forbidden_reason, fn);
 
@@ -3742,11 +3742,13 @@
 
       if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (fn))
 	  /* Avoid warnings during early inline pass. */
-	  && cgraph_global_info_ready)
+	  && cgraph_global_info_ready
+	  /* PR 20090218-1_0.c. Body can be provided by another module. */
+	  && (reason != CIF_BODY_NOT_AVAILABLE || !flag_generate_lto))
 	{
-	  sorry ("inlining failed in call to %q+F: %s", fn,
-		 _(cgraph_inline_failed_string (reason)));
-	  sorry ("called from here");
+	  error ("inlining failed in call to always_inline %q+F: %s", fn,
+		 cgraph_inline_failed_string (reason));
+	  error ("called from here");
 	}
       else if (warn_inline
 	       && DECL_DECLARED_INLINE_P (fn)

[-- Attachment #3: always_inline-tests.patch --]
[-- Type: text/plain, Size: 9107 bytes --]

Index: testsuite/ChangeLog
2011-06-16  Christian Bruel  <christian.bruel@st.com>
	
	* gcc.dg/always_inline.c: Removed -Winline. Update checks
	* gcc.dg/always_inline2.c: Likewise.
	* gcc.dg/always_inline3.c: Likewise.
	* gcc.dg/debug/pr41264-1.c: Add -Wno-attributes.
	* gcc.dg/inline_1.c: Likewise.
	* gcc.dg/inline_2.c: Likewise.
	* gcc.dg/inline_3.c: Likewise.
	* gcc.dg/inline_4.c: Likewise.
	* gcc.dg/20051201-1.c: Likewise.
	* gcc.dg/torture/pta-structcopy-1.c: Likewise.
	* gcc.dg/inline-22.c: Likewise.
	* gcc.dg/lto/20090218-1_0.c: Set inline keyword.
	* gcc.dg/lto/20090218-1_1.c: Likewise.
	* g++.dg/ipa/devirt-7.C: Likewise.
	* gcc.dg/uninit-pred-5_a.c: Likewise.
	* gcc.dg/uninit-pred-5_b.c: Likewise.
	* gcc.dg/fail_always_inline.c: New.

Index: testsuite/gcc.dg/always_inline.c
===================================================================
--- testsuite/gcc.dg/always_inline.c	(revision 175201)
+++ testsuite/gcc.dg/always_inline.c	(working copy)
@@ -1,8 +1,8 @@
 /* { dg-do compile } */
-/* { dg-options "-Winline -O2" } */
+/* { dg-options "-O2" } */
 #include <stdarg.h>
 inline __attribute__ ((always_inline)) void
-e(int t, ...) /* { dg-message "sorry\[^\n\]*variable argument" "" } */
+e(int t, ...) /* { dg-error "variable argument lists" } */
 {
   va_list q;
   va_start (q, t);
Index: testsuite/gcc.dg/inline_2.c
===================================================================
--- testsuite/gcc.dg/inline_2.c	(revision 175201)
+++ testsuite/gcc.dg/inline_2.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline=0:100 -fdisable-ipa-inline" } */
+/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline=0:100 -fdisable-ipa-inline -Wno-attributes" } */
 int g;
 __attribute__((always_inline)) void bar (void)
 {
Index: testsuite/gcc.dg/20051201-1.c
===================================================================
--- testsuite/gcc.dg/20051201-1.c	(revision 175201)
+++ testsuite/gcc.dg/20051201-1.c	(working copy)
@@ -2,7 +2,7 @@
    tree_flow_call_edges_add.  */
 
 /* { dg-do compile } */
-/* { dg-options "-O1 -fprofile-generate" } */
+/* { dg-options "-O1 -fprofile-generate -Wno-attributes" } */
 
 static __attribute__ ((always_inline)) void 
 baz ()
Index: testsuite/gcc.dg/debug/pr41264-1.c
===================================================================
--- testsuite/gcc.dg/debug/pr41264-1.c	(revision 175201)
+++ testsuite/gcc.dg/debug/pr41264-1.c	(working copy)
@@ -1,4 +1,5 @@
 /* { dg-do compile } */
+/* { dg-options "-Wno-attributes" } */
 
 #if (__SIZEOF_INT__ <= 2)	
 typedef unsigned long hashval_t;
Index: testsuite/gcc.dg/uninit-pred-5_a.c
===================================================================
--- testsuite/gcc.dg/uninit-pred-5_a.c	(revision 175201)
+++ testsuite/gcc.dg/uninit-pred-5_a.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-Wuninitialized -O2" } */
+/* { dg-options "-Wuninitialized -Wno-attributes -O2" } */
 
 int g;
 int bar();
Index: testsuite/gcc.dg/inline_3.c
===================================================================
--- testsuite/gcc.dg/inline_3.c	(revision 175201)
+++ testsuite/gcc.dg/inline_3.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile   { target i?86-*-linux* x86_64-*-linux* } } */
-/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline=foo,foo2 -fdisable-ipa-inline" } */
+/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline=foo,foo2 -fdisable-ipa-inline -Wno-attributes" } */
 int g;
 __attribute__((always_inline)) void bar (void)
 {
Index: testsuite/gcc.dg/inline-22.c
===================================================================
--- testsuite/gcc.dg/inline-22.c	(revision 175201)
+++ testsuite/gcc.dg/inline-22.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-funit-at-a-time" } */
+/* { dg-options "-funit-at-a-time -Wno-attributes" } */
 /* Verify we can inline without a complete prototype and with promoted
    arguments.  See also PR32492.  */
 __attribute__((always_inline)) void f1() {}
Index: testsuite/gcc.dg/always_inline2.c
===================================================================
--- testsuite/gcc.dg/always_inline2.c	(revision 175201)
+++ testsuite/gcc.dg/always_inline2.c	(working copy)
@@ -1,8 +1,8 @@
 /* { dg-do compile } */
-/* { dg-options "-Winline -O2" } */
-inline __attribute__ ((always_inline)) void t(void); /* { dg-message "sorry\[^\n\]*body not available" "" } */
+/* { dg-options "-O2" } */
+inline __attribute__ ((always_inline)) void t(void); /* { dg-error "body not available" } */
 void
 q(void)
 {
-  t(); 				/* { dg-message "sorry\[^\n\]*called from here" "" } */
+  t(); 				/* { dg-error "called from here" } */
 }
Index: testsuite/gcc.dg/uninit-pred-5_b.c
===================================================================
--- testsuite/gcc.dg/uninit-pred-5_b.c	(revision 175201)
+++ testsuite/gcc.dg/uninit-pred-5_b.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-Wuninitialized -O2" } */
+/* { dg-options "-Wuninitialized -Wno-attributes -O2" } */
 
 int g;
 int bar();
Index: testsuite/gcc.dg/inline_4.c
===================================================================
--- testsuite/gcc.dg/inline_4.c	(revision 175201)
+++ testsuite/gcc.dg/inline_4.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile  { target i?86-*-linux* x86_64-*-linux* } } */
-/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline=foo2 -fdisable-ipa-inline" } */
+/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline=foo2 -fdisable-ipa-inline -Wno-attributes" } */
 int g;
 __attribute__((always_inline)) void bar (void)
 {
Index: testsuite/gcc.dg/lto/20090218-1_0.c
===================================================================
--- testsuite/gcc.dg/lto/20090218-1_0.c	(revision 175201)
+++ testsuite/gcc.dg/lto/20090218-1_0.c	(working copy)
@@ -1,4 +1,4 @@
-void set_mem_alias_set ()  __attribute__ ((always_inline));
+void inline set_mem_alias_set ()  __attribute__ ((always_inline));
 void emit_push_insn () {
   set_mem_alias_set ();
 }
Index: testsuite/gcc.dg/lto/20090218-1_1.c
===================================================================
--- testsuite/gcc.dg/lto/20090218-1_1.c	(revision 175201)
+++ testsuite/gcc.dg/lto/20090218-1_1.c	(working copy)
@@ -4,6 +4,6 @@
 }
 static void  __attribute__ ((noinline)) get_mem_attrs () {
 }
-void  __attribute__ ((always_inline)) set_mem_alias_set () {
+void  inline __attribute__ ((always_inline)) set_mem_alias_set () {
   get_mem_attrs ();
 }
Index: testsuite/gcc.dg/torture/pta-structcopy-1.c
===================================================================
--- testsuite/gcc.dg/torture/pta-structcopy-1.c	(revision 175201)
+++ testsuite/gcc.dg/torture/pta-structcopy-1.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do run } */
-/* { dg-options "-fdump-tree-ealias" } */
+/* { dg-options "-fdump-tree-ealias -Wno-attributes" } */
 /* { dg-skip-if "" { *-*-* } { "-O0" } { "" } } */
 
 struct X
Index: testsuite/gcc.dg/tree-ssa/pr40087.c
===================================================================
--- testsuite/gcc.dg/tree-ssa/pr40087.c	(revision 175201)
+++ testsuite/gcc.dg/tree-ssa/pr40087.c	(working copy)
@@ -3,7 +3,7 @@
 
 extern void abort (void);
 
-static void __attribute__((always_inline))
+static inline void __attribute__((always_inline))
 reverse(int *first, int *last)
 {
   if (first == last--) 
Index: testsuite/gcc.dg/always_inline3.c
===================================================================
--- testsuite/gcc.dg/always_inline3.c	(revision 175201)
+++ testsuite/gcc.dg/always_inline3.c	(working copy)
@@ -1,11 +1,11 @@
 /* { dg-do compile } */
-/* { dg-options "-Winline -O2" } */
+/* { dg-options "-O2" } */
 int do_something_evil (void);
 inline __attribute__ ((always_inline)) void
-q2(void) /* { dg-message "sorry\[^\n\]*recursive" "" } */
+q2(void) /* { dg-error "recursive inlining" } */
 {
   if (do_something_evil ())
     return;
-  q2(); 			/* { dg-message "sorry\[^\n\]*called from here" "" } */
+  q2(); 			/* { dg-error "called from here" } */
   q2(); /* With -O2 we don't warn here, it is eliminated by tail recursion.  */
 }
Index: testsuite/gcc.dg/inline_1.c
===================================================================
--- testsuite/gcc.dg/inline_1.c	(revision 175201)
+++ testsuite/gcc.dg/inline_1.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline -fdisable-ipa-inline" } */
+/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline -fdisable-ipa-inline -Wno-attributes" } */
 int g;
 __attribute__((always_inline)) void bar (void)
 {
Index: testsuite/g++.dg/ipa/devirt-7.C
===================================================================
--- testsuite/g++.dg/ipa/devirt-7.C	(revision 175201)
+++ testsuite/g++.dg/ipa/devirt-7.C	(working copy)
@@ -56,7 +56,7 @@
   return 1;
 }
 
-int __attribute__ ((always_inline))
+int inline __attribute__ ((always_inline))
 A::middleman_1 (int i)
 {
   return this->foo (i);

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Rainer Orth]

Christian Bruel <christian.bruel@st.com> writes:

> 2011-06-16  Christian Bruel  <christian.bruel@st.com>
> 	
> 	PR 49139/43654

Please use the correct PR number format here:

	PR middle-end/49139
        PR middle-end/43654

Otherwise the check-in notices don't get into the PRs.

Thanks.
        Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Richard Guenther]

On Mon, Jun 20, 2011 at 3:32 PM, Christian Bruel <christian.bruel@st.com> wrote:
> In addition to the approved part of the patch, I finally got the testsuite
> and a full Linux distribution to build without false warnings, with the
> additional following changes:
>
> * two false warnings detected during a Linux distrib rebuild
>
>  - __typeof (__error) doesn't propagate the inline keyword. even if the
> function had attribute always_inline.
>  - extern inline function redefinition would inherit the attribute, even if
> they are not inline
>
>  DECL_UNINLINABLE is set for redefining extern inline functions. (commented
> in c-decl.c:2340). So indeed testing this flags in the cgraphunit.c part
> fixed both.
>  I checked that this doesn't conflict with other DECL_UNINLINABLE warning
> emitted from ree_inlinable_function_p. (which is called after)
>
> * One silent bug revealed, undetected in the GCC testuite:
>
>  - PR43564.c compiled in -O0, the "__clz" function was silently not inlined.
> Fixed in ipa-inline.c:inline_forbidden_p_stmt

-      if ((caller_opt->x_optimize > callee_opt->x_optimize)
-	  || (caller_opt->x_optimize_size != callee_opt->x_optimize_size))
+      if (((caller_opt->x_optimize > callee_opt->x_optimize)
+	   || (caller_opt->x_optimize_size != callee_opt->x_optimize_size))
+	  /* gcc.dg/pr43564.c.  look at forced inline even in -O0.  */
+	  && !lookup_attribute ("always_inline", DECL_ATTRIBUTES (e->callee->decl)))

please check DECL_DISREGARD_INLINE_LIMITS instead.

> * LTO pitfall: It's OK to not have a body to inline when generating the LTO
> Gimple. Enforced by PR20090218-1_0.c

I'm not 100% sure here ;)  At least as we still output a regular object
file without the function being inlined.  But as we're going towars
slim-LTO (hopefully) I guess it's ok.

> * In addition to the parts of the patch that was approved, I had to fix the
> tests that didn't use the inline keyword but enforced the always_inline
> attribute to pass without the warning
>  I choose to add -Wno-attributes to the option list. Could as well fix the
> test by adding the inline keyword, but I preferred to not modify the
> original test when possible.
>
> * A minor code optimization in ipa-inline-transform.c: Don't call
> optimize_inline_calls if there is not callee.
>
> Comments, OK to apply ?

Ok with the above change and the PRs mentioned in the changelog
properly (see Rainers comment).

Thanks,
Richard.

> Many thanks
>
> Christian
>
> 2011-06-16  Christian Bruel  <christian.bruel@st.com>
>
>        PR 49139/43654
>        * cgraphunit.c (process_function_and_variable_attributes): warn
>        for always_inline functions that are not inline.
>        * ipa-inline-transform.c (inline_transform): Always call
>        optimize_inline.
>        * ipa-inline.c (can_inline_edge_p): Check always_inline.
>        * tree-inline.c (tree_inlinable_function_p): Call error instead
>        of sorry.
>        (expand_call_inline): Likewise.
>
> 2011-06-16  Christian Bruel  <christian.bruel@st.com>
>
>        * gcc.dg/always_inline.c: Removed -Winline. Update checks
>        * gcc.dg/always_inline2.c: Likewise.
>        * gcc.dg/always_inline3.c: Likewise.
>        * gcc.dg/debug/pr41264-1.c: Add -Wno-attributes.
>        * gcc.dg/inline_1.c: Likewise.
>        * gcc.dg/inline_2.c: Likewise.
>        * gcc.dg/inline_3.c: Likewise.
>        * gcc.dg/inline_4.c: Likewise.
>        * gcc.dg/20051201-1.c: Likewise.
>        * gcc.dg/torture/pta-structcopy-1.c: Likewise.
>        * gcc.dg/inline-22.c: Likewise.
>        * gcc.dg/lto/20090218-1_0.c: Set inline keyword.
>        * gcc.dg/lto/20090218-1_1.c: Likewise.
>        * g++.dg/ipa/devirt-7.C: Likewise.
>        * gcc.dg/uninit-pred-5_a.c: Likewise.
>        * gcc.dg/uninit-pred-5_b.c: Likewise.
>        * gcc.dg/fail_always_inline.c: New.
>

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Mike Stump]

On Jun 20, 2011, at 6:41 AM, Rainer Orth wrote:
> Christian Bruel <christian.bruel@st.com> writes:
> 
>> 2011-06-16  Christian Bruel  <christian.bruel@st.com>
>> 	
>> 	PR 49139/43654
> 
> Please use the correct PR number format here:
> 
> 	PR middle-end/49139
>        PR middle-end/43654
> 
> Otherwise the check-in notices don't get into the PRs.

Also, I think there is a preference to pick the best bug report to make this a fix for, and list just one number, and then close the second as a dup of the first, after the first is fixed.

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Christian Bruel]

On 06/20/2011 03:41 PM, Rainer Orth wrote:
> Christian Bruel<christian.bruel@st.com>  writes:
>
>> 2011-06-16  Christian Bruel<christian.bruel@st.com>
>> 	
>> 	PR 49139/43654
>
> Please use the correct PR number format here:
>
> 	PR middle-end/49139
>          PR middle-end/43654
>
> Otherwise the check-in notices don't get into the PRs.

OK thanks, in fact I was referring to the file gcc.dg/pr43564.c (there 
was a typo in the number btw). But good indeed to send the notice into 
the original PR as well.

Cheers

Christian

>
> Thanks.
>          Rainer
>

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Richard Guenther]

On Mon, Jun 20, 2011 at 3:56 PM, Christian Bruel <christian.bruel@st.com> wrote:
> On 06/20/2011 03:41 PM, Rainer Orth wrote:
>>
>> Christian Bruel<christian.bruel@st.com>  writes:
>>
>>> 2011-06-16  Christian Bruel<christian.bruel@st.com>
>>>
>>>        PR 49139/43654
>>
>> Please use the correct PR number format here:
>>
>>        PR middle-end/49139
>>         PR middle-end/43654
>>
>> Otherwise the check-in notices don't get into the PRs.
>
> OK thanks, in fact I was referring to the file gcc.dg/pr43564.c (there was a
> typo in the number btw). But good indeed to send the notice into the
> original PR as well.

The code now looks like

  if (node->callees)
    {
      /* Redirecting edges might lead to a need for vops to be recomputed.  */
      todo |= TODO_update_ssa_only_virtuals;
      todo = optimize_inline_calls (current_function_decl);
    }

I have committed an obvious patch.

Richard.

> Cheers
>
> Christian
>
>>
>> Thanks.
>>         Rainer
>>
>

Re: [PATH] PR/49139 fix always_inline failures diagnostics

[Richard Guenther]

On Tue, Jun 21, 2011 at 1:11 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Mon, Jun 20, 2011 at 3:56 PM, Christian Bruel <christian.bruel@st.com> wrote:
>> On 06/20/2011 03:41 PM, Rainer Orth wrote:
>>>
>>> Christian Bruel<christian.bruel@st.com>  writes:
>>>
>>>> 2011-06-16  Christian Bruel<christian.bruel@st.com>
>>>>
>>>>        PR 49139/43654
>>>
>>> Please use the correct PR number format here:
>>>
>>>        PR middle-end/49139
>>>         PR middle-end/43654
>>>
>>> Otherwise the check-in notices don't get into the PRs.
>>
>> OK thanks, in fact I was referring to the file gcc.dg/pr43564.c (there was a
>> typo in the number btw). But good indeed to send the notice into the
>> original PR as well.
>
> The code now looks like
>
>  if (node->callees)
>    {
>      /* Redirecting edges might lead to a need for vops to be recomputed.  */
>      todo |= TODO_update_ssa_only_virtuals;
>      todo = optimize_inline_calls (current_function_decl);
>    }
>
> I have committed an obvious patch.

Ick - that broke bootstrap.  FIxing as follows.

RIchard.

2011-06-21  Richard Guenther  <rguenther@suse.de>

        * ipa-inline-transform.c (inline_transform): Fix previous
        change.

Index: gcc/ipa-inline-transform.c
===================================================================
--- gcc/ipa-inline-transform.c  (revision 175253)
+++ gcc/ipa-inline-transform.c  (working copy)
@@ -364,13 +364,13 @@ inline_transform (struct cgraph_node *no

   timevar_push (TV_INTEGRATION);
   if (node->callees)
-    {
-      todo = optimize_inline_calls (current_function_decl);
-      /* Redirecting edges might lead to a need for vops to be recomputed.  */
-      todo |= TODO_update_ssa_only_virtuals;
-    }
+    todo = optimize_inline_calls (current_function_decl);
   timevar_pop (TV_INTEGRATION);

+  if (!(todo & TODO_update_ssa_any))
+    /* Redirecting edges might lead to a need for vops to be recomputed.  */
+    todo |= TODO_update_ssa_only_virtuals;
+
   cfun->always_inline_functions_inlined = true;
   cfun->after_inlining = true;
   return todo | execute_fixup_cfg ();