[PATCH 0/2] __builtin_unreachable

[PATCH 0/2] __builtin_unreachable

[Roland McGrath]

The latest GCC has a better way than "for (;;)" to indicate that a code
path cannot be reached due to reasons the compiler doesn't understand (such
as code in an asm).  These patches provide UNREACHABLE() as a macro to hide
the details of this, and then use it for the BUG() macro on x86, saving
some dead code otherwise generated.

Other arch's BUG() may want to this too instead of "for (;;)" or __builtin_trap.

There are numerous matches from "git grep -n 'for *(;;) *;'" but it takes
someone who knows each bit of code to know where that means UNREACHABLE()
and where it really wants an infinite loop.


The following changes since commit 74fca6a42863ffacaf7ba6f1936a9f228950f657:
  Linus Torvalds (1):
        Linux 2.6.31

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/frob/linux-2.6-roland.git topic/builtin_unreachable

Roland McGrath (2):
      UNREACHABLE() macro
      x86: BUG(): use UNREACHABLE()

 arch/x86/include/asm/bug.h    |    4 ++--
 include/linux/compiler-gcc4.h |   13 +++++++++++++
 include/linux/compiler.h      |    9 +++++++++
 3 files changed, 24 insertions(+), 2 deletions(-)


Thanks,
Roland

[PATCH 1/2] UNREACHABLE() macro

[Roland McGrath]

This adds the UNREACHABLE() macro to <linux/compiler.h>.  The latest
GCC has __builtin_unreachable(), which makes it generate better code
(i.e. omit some unreachable instructions).

Signed-off-by: Roland McGrath <roland@redhat.com>
CC: Jakub Jelinek <jakub@redhat.com>
---
 include/linux/compiler-gcc4.h |   13 +++++++++++++
 include/linux/compiler.h      |    9 +++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
index 450fa59..320a9ad 100644
--- a/include/linux/compiler-gcc4.h
+++ b/include/linux/compiler-gcc4.h
@@ -37,3 +37,16 @@
 #define __cold			__attribute__((__cold__))
 
 #endif
+
+/*
+ * __builtin_unreachable is available in GCC 4.5+ and
+ * also in Fedora/Red Hat GCC 4.4.1-10+.
+ */
+#if (__GNUC_MINOR__ > 4					\
+     || (__GNUC_MINOR__ == 4				\
+	 && defined __GNUC_RH_RELEASE__			\
+	 && (__GNUC_PATCHLEVEL__ > 1			\
+	     || (__GNUC_PATCHLEVEL__ == 1		\
+		 && __GNUC_RH_RELEASE__ >= 10))))
+#define UNREACHABLE()		__builtin_unreachable()
+#endif
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 04fb513..e3dc896 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -256,6 +256,15 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 #define __cold
 #endif
 
+/*
+ * Tell the compiler following code is unreachable and should disappear.
+ * If nothing else, just make it obvious to avoid warnings about missing
+ * return or whatnot.  Some compilers define a way to optimize it better.
+ */
+#ifndef UNREACHABLE
+#define UNREACHABLE() for (;;)
+#endif
+
 /* Simple shorthand for a section definition */
 #ifndef __section
 # define __section(S) __attribute__ ((__section__(#S)))

[PATCH 2/2] x86: BUG(): use UNREACHABLE()

[Roland McGrath]

This uses the new UNREACHABLE() macro in x86's BUG() macro.  In my
test builds, this saved 2022 bytes of text in vmlinux on x86_64,
3051 bytes on i686.  (I didn't bother to calculate the additional
savings in .ko text.)

Signed-off-by: Roland McGrath <roland@redhat.com>
CC: Jakub Jelinek <jakub@redhat.com>
---
 arch/x86/include/asm/bug.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index d9cf1cd..a6458a3 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -22,14 +22,14 @@ do {								\
 		     ".popsection"				\
 		     : : "i" (__FILE__), "i" (__LINE__),	\
 		     "i" (sizeof(struct bug_entry)));		\
-	for (;;) ;						\
+	UNREACHABLE();						\
 } while (0)
 
 #else
 #define BUG()							\
 do {								\
 	asm volatile("ud2");					\
-	for (;;) ;						\
+	UNREACHABLE();						\
 } while (0)
 #endif
 

Re: [PATCH 0/2] __builtin_unreachable

[Américo Wang]

On Thu, Sep 10, 2009 at 9:59 AM, Roland McGrath <roland@redhat.com> wrote:
> The latest GCC has a better way than "for (;;)" to indicate that a code
> path cannot be reached due to reasons the compiler doesn't understand (such
> as code in an asm).  These patches provide UNREACHABLE() as a macro to hide
> the details of this, and then use it for the BUG() macro on x86, saving
> some dead code otherwise generated.
>
> Other arch's BUG() may want to this too instead of "for (;;)" or __builtin_trap.
>
> There are numerous matches from "git grep -n 'for *(;;) *;'" but it takes
> someone who knows each bit of code to know where that means UNREACHABLE()
> and where it really wants an infinite loop.

Looks good!

Reviewed-by: WANG Cong <xiyou.wangcong@gmail.com>

Just curious, what different asm code will gcc generate for this? Comparing it
to for(;;) ? I am sorry that I don't have gcc 4.5 on hand.

Thanks!

Re: [PATCH 0/2] __builtin_unreachable

[Jakub Jelinek]

On Wed, Sep 09, 2009 at 06:59:23PM -0700, Roland McGrath wrote:
> The latest GCC has a better way than "for (;;)" to indicate that a code
> path cannot be reached due to reasons the compiler doesn't understand (such
> as code in an asm).  These patches provide UNREACHABLE() as a macro to hide
> the details of this, and then use it for the BUG() macro on x86, saving
> some dead code otherwise generated.
> 
> Other arch's BUG() may want to this too instead of "for (;;)" or __builtin_trap.

Just instead of for (;;) (or any place where an asm never returns). 
__builtin_trap () shouldn't be replaced - __builtin_unreachable () just says
that the location is never reachable, so everything after it not reachable
through other ways can be optimized out - while __builtin_trap () emits an
instruction that traps and then assumes anything after it is unreachable.

Also, I wonder if Fedora/RHEL specific GCC backport is desirable to be
handled in upstream kernel (i.e. whether compiler-gcc4.h shouldn't enable it
just for GCC 4.5+) and Fedora/RHEL should patch it for our backport.

	Jakub

Re: [PATCH 0/2] __builtin_unreachable

[H. Peter Anvin]

On 09/09/2009 09:42 PM, Américo Wang wrote:
> 
> Just curious, what different asm code will gcc generate for this? Comparing it
> to for(;;) ? I am sorry that I don't have gcc 4.5 on hand.
> 

It generates nothing.  It is you as the programmer guaranteeing that we
will never get there, and as such gcc will generate no code at all.

	-hpa

Re: [PATCH 1/2] UNREACHABLE() macro

[Andrew Morton]

On Wed,  9 Sep 2009 19:00:31 -0700 (PDT)
Roland McGrath <roland@redhat.com> wrote:

> +/*
> + * __builtin_unreachable is available in GCC 4.5+ and
> + * also in Fedora/Red Hat GCC 4.4.1-10+.
> + */
> +#if (__GNUC_MINOR__ > 4					\
> +     || (__GNUC_MINOR__ == 4				\
> +	 && defined __GNUC_RH_RELEASE__			\
> +	 && (__GNUC_PATCHLEVEL__ > 1			\
> +	     || (__GNUC_PATCHLEVEL__ == 1		\
> +		 && __GNUC_RH_RELEASE__ >= 10))))
> +#define UNREACHABLE()		__builtin_unreachable()
> +#endif

That's a bit of a mouthful.  Did you consider a runtime probe with
scripts/Kbuild.include's try-run, cc-option, etc?  

Re: [PATCH 1/2] UNREACHABLE() macro

[Kyle McMartin]

On Fri, Sep 11, 2009 at 01:45:28PM -0700, Andrew Morton wrote:
> On Wed,  9 Sep 2009 19:00:31 -0700 (PDT)
> Roland McGrath <roland@redhat.com> wrote:
> 
> > +/*
> > + * __builtin_unreachable is available in GCC 4.5+ and
> > + * also in Fedora/Red Hat GCC 4.4.1-10+.
> > + */
> > +#if (__GNUC_MINOR__ > 4					\
> > +     || (__GNUC_MINOR__ == 4				\
> > +	 && defined __GNUC_RH_RELEASE__			\
> > +	 && (__GNUC_PATCHLEVEL__ > 1			\
> > +	     || (__GNUC_PATCHLEVEL__ == 1		\
> > +		 && __GNUC_RH_RELEASE__ >= 10))))
> > +#define UNREACHABLE()		__builtin_unreachable()
> > +#endif
> 
> That's a bit of a mouthful.  Did you consider a runtime probe with
> scripts/Kbuild.include's try-run, cc-option, etc?  

If the crap for determining if it's a RH gcc is removed, then it becomes
really straightforward... Should probably just do that and then apply a
patch to Fedora if it's so damned important it can't just wait for the
next GCC release.

regards, Kyle

Re: [PATCH 1/2] UNREACHABLE() macro

[Roland McGrath]

> That's a bit of a mouthful.  Did you consider a runtime probe with
> scripts/Kbuild.include's try-run, cc-option, etc?  

I did not see any precedent in the sources for using those to test for
features by compiling particular test sources (i.e. in autoconf style).
I just followed the model I saw.  Those methods seem sufficiently costly
to make it a little questionable to pile on too many more that get
repeated all the time by make, so I just was not opening that can of worms.

As has been mentioned:

	#if __GNUC_MINOR__ > 4

is a sufficient test for the long run.  People using Fedora gcc-4.4 will
not mind applying a trivial kernel patch to get the benefits sooner.


Thanks,
Roland

Re: [PATCH 1/2] UNREACHABLE() macro

[Arjan van de Ven]

On Fri, 11 Sep 2009 14:55:25 -0700 (PDT)
Roland McGrath <roland@redhat.com> wrote:

> > That's a bit of a mouthful.  Did you consider a runtime probe with
> > scripts/Kbuild.include's try-run, cc-option, etc?  
> 
> I did not see any precedent in the sources for using those to test for
> features by compiling particular test sources (i.e. in autoconf

look at the stackprotector flags.. they work this way already.
It gets done once per kernel build...


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 1/2] UNREACHABLE() macro

[H. Peter Anvin]

On 09/11/2009 08:49 PM, Arjan van de Ven wrote:
> On Fri, 11 Sep 2009 14:55:25 -0700 (PDT)
> Roland McGrath <roland@redhat.com> wrote:
> 
>>> That's a bit of a mouthful.  Did you consider a runtime probe with
>>> scripts/Kbuild.include's try-run, cc-option, etc?  
>>
>> I did not see any precedent in the sources for using those to test for
>> features by compiling particular test sources (i.e. in autoconf
> 
> look at the stackprotector flags.. they work this way already.
> It gets done once per kernel build...
> 

That works for flags, but not for the presence of builtin functions.
You can't even just try compiling something, since it will turn into an
ordinary function if not present... not obvious until link.

	-hpa

Re: [PATCH 1/2] UNREACHABLE() macro

[Arjan van de Ven]

On Fri, 11 Sep 2009 21:43:42 -0700
"H. Peter Anvin" <hpa@kernel.org> wrote:

> On 09/11/2009 08:49 PM, Arjan van de Ven wrote:
> > On Fri, 11 Sep 2009 14:55:25 -0700 (PDT)
> > Roland McGrath <roland@redhat.com> wrote:
> > 
> >>> That's a bit of a mouthful.  Did you consider a runtime probe with
> >>> scripts/Kbuild.include's try-run, cc-option, etc?  
> >>
> >> I did not see any precedent in the sources for using those to test
> >> for features by compiling particular test sources (i.e. in autoconf
> > 
> > look at the stackprotector flags.. they work this way already.
> > It gets done once per kernel build...
> > 
> 
> That works for flags, but not for the presence of builtin functions.
> You can't even just try compiling something, since it will turn into
> an ordinary function if not present... not obvious until link.

"-DGG_HAS_UNREACHABLE" is a compiler flag ;-)
and the shell script can just do a "nm" on the resulting .o file to
detect if it turned into a function call... entirely possible.


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 1/2] UNREACHABLE() macro

[Andrew Morton]

On Fri, 11 Sep 2009 21:43:42 -0700 "H. Peter Anvin" <hpa@kernel.org> wrote:

> On 09/11/2009 08:49 PM, Arjan van de Ven wrote:
> > On Fri, 11 Sep 2009 14:55:25 -0700 (PDT)
> > Roland McGrath <roland@redhat.com> wrote:
> > 
> >>> That's a bit of a mouthful.  Did you consider a runtime probe with
> >>> scripts/Kbuild.include's try-run, cc-option, etc?  
> >>
> >> I did not see any precedent in the sources for using those to test for
> >> features by compiling particular test sources (i.e. in autoconf
> > 
> > look at the stackprotector flags.. they work this way already.
> > It gets done once per kernel build...
> > 
> 
> That works for flags, but not for the presence of builtin functions.
> You can't even just try compiling something, since it will turn into an
> ordinary function if not present... not obvious until link.
> 

Use -Wall -Werror and if the compiler doesn't know about
__builtin_unreachable() it will error out.

Re: [PATCH 1/2] UNREACHABLE() macro

[Jakub Jelinek]

On Fri, Sep 11, 2009 at 10:52:28PM -0700, Andrew Morton wrote:
> On Fri, 11 Sep 2009 21:43:42 -0700 "H. Peter Anvin" <hpa@kernel.org> wrote:
> 
> > On 09/11/2009 08:49 PM, Arjan van de Ven wrote:
> > > On Fri, 11 Sep 2009 14:55:25 -0700 (PDT)
> > > Roland McGrath <roland@redhat.com> wrote:
> > > 
> > >>> That's a bit of a mouthful.  Did you consider a runtime probe with
> > >>> scripts/Kbuild.include's try-run, cc-option, etc?  
> > >>
> > >> I did not see any precedent in the sources for using those to test for
> > >> features by compiling particular test sources (i.e. in autoconf
> > > 
> > > look at the stackprotector flags.. they work this way already.
> > > It gets done once per kernel build...
> > > 
> > 
> > That works for flags, but not for the presence of builtin functions.
> > You can't even just try compiling something, since it will turn into an
> > ordinary function if not present... not obvious until link.
> > 
> 
> Use -Wall -Werror and if the compiler doesn't know about
> __builtin_unreachable() it will error out.

Yeah, e.g. checking exit status of:
echo '__attribute__((noreturn)) void foo (void) { asm volatile ("" : : : "memory"); __builtin_unreachable (); }' | gcc -S -O2 -W -Wall -Werror -xc - -o /dev/null > /dev/null 2>/dev/null
works.

	Jakub

Re: [PATCH 1/2] UNREACHABLE() macro

[H. Peter Anvin]

On 09/11/2009 11:38 PM, Jakub Jelinek wrote:
>>>
>>> That works for flags, but not for the presence of builtin functions.
>>> You can't even just try compiling something, since it will turn into an
>>> ordinary function if not present... not obvious until link.
>>
>> Use -Wall -Werror and if the compiler doesn't know about
>> __builtin_unreachable() it will error out.
> 
> Yeah, e.g. checking exit status of:
> echo '__attribute__((noreturn)) void foo (void) { asm volatile ("" : : : "memory"); __builtin_unreachable (); }' | gcc -S -O2 -W -Wall -Werror -xc - -o /dev/null > /dev/null 2>/dev/null
> works.
> 

OK, that makes sense... although perhaps it would be cleaner to use
something that could work for other builtins.

One advantage of using "nm" would be that it could answer the general
question "does this version of gcc support __builtin_X() without
resorting to libgcc", which matters quite a bit for functions like
__builtin_ctzl() and so on.

	-hpa