[PATCH] x86: remove unneeded endless loop in BUG()

[PATCH] x86: remove unneeded endless loop in BUG()

[Petr Tesarik]

Since __builtin_trap() will always generate an illegal instruction, we
can replace the explicit asm("ud2") with it.

This way gcc will understand that the function never returns, plus it
won't emit any extra instructions.

Signed-off-by: Petr Tesarik <ptesarik@suse.cz>

diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index d9cf1cd..9b7c50a 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -4,6 +4,13 @@
 #ifdef CONFIG_BUG
 #define HAVE_ARCH_BUG
 
+#define DO_BUG()						\
+do {								\
+	__builtin_trap();					\
+	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
+	panic("BUG!");						\
+} while(0)
+
 #ifdef CONFIG_DEBUG_BUGVERBOSE
 
 #ifdef CONFIG_X86_32
@@ -14,7 +21,7 @@
 
 #define BUG()							\
 do {								\
-	asm volatile("1:\tud2\n"				\
+	asm volatile("1:\n"					\
 		     ".pushsection __bug_table,\"a\"\n"		\
 		     __BUG_C0					\
 		     "\t.word %c1, 0\n"				\
@@ -22,15 +29,11 @@ do {								\
 		     ".popsection"				\
 		     : : "i" (__FILE__), "i" (__LINE__),	\
 		     "i" (sizeof(struct bug_entry)));		\
-	for (;;) ;						\
+	DO_BUG();					\
 } while (0)
 
 #else
-#define BUG()							\
-do {								\
-	asm volatile("ud2");					\
-	for (;;) ;						\
-} while (0)
+#define BUG	DO_BUG
 #endif
 
 #endif /* !CONFIG_BUG */

Re: [PATCH] x86: remove unneeded endless loop in BUG()

[H. Peter Anvin]

Petr Tesarik wrote:
> Since __builtin_trap() will always generate an illegal instruction, we
> can replace the explicit asm("ud2") with it.
> 
> This way gcc will understand that the function never returns, plus it
> won't emit any extra instructions.
> 
> Signed-off-by: Petr Tesarik <ptesarik@suse.cz>

I like this.  Will apply shortly.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] x86: remove unneeded endless loop in BUG()

[Ingo Molnar]

* Petr Tesarik <ptesarik@suse.cz> wrote:

> Since __builtin_trap() will always generate an illegal 
> instruction, we can replace the explicit asm("ud2") with it.
> 
> This way gcc will understand that the function never returns, 
> plus it won't emit any extra instructions.
> 
> Signed-off-by: Petr Tesarik <ptesarik@suse.cz>

> +#define DO_BUG()						\
> +do {								\
> +	__builtin_trap();					\
> +	printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
> +	panic("BUG!");						\
> +} while(0)
> +
>  #ifdef CONFIG_DEBUG_BUGVERBOSE
>  
>  #ifdef CONFIG_X86_32
> @@ -14,7 +21,7 @@
>  
>  #define BUG()							\
>  do {								\
> -	asm volatile("1:\tud2\n"				\
> +	asm volatile("1:\n"					\
>  		     ".pushsection __bug_table,\"a\"\n"		\
>  		     __BUG_C0					\
>  		     "\t.word %c1, 0\n"				\
> @@ -22,15 +29,11 @@ do {								\
>  		     ".popsection"				\
>  		     : : "i" (__FILE__), "i" (__LINE__),	\
>  		     "i" (sizeof(struct bug_entry)));		\
> -	for (;;) ;						\
> +	DO_BUG();					\

the problem is that the DO_BUG() will generate the u2d 
instruction into a random place where GCC puts it. It certainly 
wont be in the place where the __bug_table logic above expects 
it.

The result will be cryptic crashes instead of a clean BUG 
message assert.

	Ingo

Re: [PATCH] x86: remove unneeded endless loop in BUG()

[H. Peter Anvin]

Ingo Molnar wrote:
> 
> the problem is that the DO_BUG() will generate the u2d 
> instruction into a random place where GCC puts it. It certainly 
> wont be in the place where the __bug_table logic above expects 
> it.
> 
> The result will be cryptic crashes instead of a clean BUG 
> message assert.
> 

For that to happen, it would have to move the asm volatile relative to 
the __builtin_trap(), which seems slightly unlikely -- are there any 
cases at which this has been known to happen, or is that conjecture on 
your part?

It would be more of a "right thing" to do this with a label on the 
__builtin_trap(), but the problem with that labels have function scope 
even if they occur inside a block.

	-hpa

Re: [PATCH] x86: remove unneeded endless loop in BUG()

[Jeremy Fitzhardinge]

H. Peter Anvin wrote:
> For that to happen, it would have to move the asm volatile relative to 
> the __builtin_trap(), which seems slightly unlikely -- are there any 
> cases at which this has been known to happen, or is that conjecture on 
> your part?

There's no reason to expect an "asm volatile" to stay put; the 
"volatile" has nothing to do with preventing code motion.  The only way 
to make an asm stick in one place is with data dependencies, and I'm not 
sure what the dependency for __builtin_trap might be.  I'm guessing its 
a "memory" clobber, but that's pure guesswork.

> It would be more of a "right thing" to do this with a label on the 
> __builtin_trap(), but the problem with that labels have function scope 
> even if they occur inside a block.

No, you can declare them local (__label__ foo).  But gcc will happily do 
crazy/crappy things with them if you don't actually end up gotoing them 
(even indirectly).  I've had a couple of go-rounds with the fine helpful 
gcc folks on this very issue...

It would be worth checking the archives from about the time I did the 
generic BUG stuff; around dec-jan 2006-7, I think.

    J

Re: [PATCH] x86: remove unneeded endless loop in BUG()

[H. Peter Anvin]

Jeremy Fitzhardinge wrote:
> 
> There's no reason to expect an "asm volatile" to stay put; the 
> "volatile" has nothing to do with preventing code motion.  The only way 
> to make an asm stick in one place is with data dependencies, and I'm not 
> sure what the dependency for __builtin_trap might be.  I'm guessing its 
> a "memory" clobber, but that's pure guesswork.
> 

I looked through this with HJL; the bottom line is that although it 
probably would work, it's not worth it.

I submitted GCC bug 39252 to get __builtin_not_reached(); instead.

	-hpa

Re: [PATCH] x86: remove unneeded endless loop in BUG()

[Petr Tesarik]

H. Peter Anvin píše v Čt 19. 02. 2009 v 12:48 -0800:
> Jeremy Fitzhardinge wrote:
> > 
> > There's no reason to expect an "asm volatile" to stay put; the 
> > "volatile" has nothing to do with preventing code motion.  The only way 
> > to make an asm stick in one place is with data dependencies, and I'm not 
> > sure what the dependency for __builtin_trap might be.  I'm guessing its 
> > a "memory" clobber, but that's pure guesswork.
> > 
> 
> I looked through this with HJL; the bottom line is that although it 
> probably would work, it's not worth it.
> 
> I submitted GCC bug 39252 to get __builtin_not_reached(); instead.

Thank you! I like it this way, too.

Petr Tesarik

Re: [PATCH] x86: remove unneeded endless loop in BUG()

[H. Peter Anvin]

Ingo Molnar wrote:
> 
> the problem is that the DO_BUG() will generate the u2d 
> instruction into a random place where GCC puts it. It certainly 
> wont be in the place where the __bug_table logic above expects 
> it.
> 
> The result will be cryptic crashes instead of a clean BUG 
> message assert.
> 

I went and talked to H.J. Lu about this.

He said __builtin_trap(); is functionally treated as an asm volatile, 
and that it is most likely impossible that gcc could do anything wrong 
here (he did specifically state that nothing can move across the asm 
volatile, and there are no data dependencies between the asm volatile 
and the __builtin_trap).

He also agreed that the right way to do this is __builtin_not_reached(), 
and I promised to submit a feature request for this for a future version 
of gcc.

Given that, I would suggest we back out the patch, and that when 
__builtin_not_reached(); is supported, we can simply do:

#if __GNUC__ is recent enough
# define not_reached() __builtin_not_reached()
#else
# define not_reached() for(;;)
#endif

OK?

	-hpa

Re: [PATCH] x86: remove unneeded endless loop in BUG()

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> Ingo Molnar wrote:
>>
>> the problem is that the DO_BUG() will generate the u2d instruction into 
>> a random place where GCC puts it. It certainly wont be in the place 
>> where the __bug_table logic above expects it.
>>
>> The result will be cryptic crashes instead of a clean BUG message 
>> assert.
>>
>
> I went and talked to H.J. Lu about this.
>
> He said __builtin_trap(); is functionally treated as an asm volatile,  
> and that it is most likely impossible that gcc could do anything wrong  
> here (he did specifically state that nothing can move across the asm  
> volatile, and there are no data dependencies between the asm volatile  
> and the __builtin_trap).
>
> He also agreed that the right way to do this is __builtin_not_reached(),  
> and I promised to submit a feature request for this for a future version  
> of gcc.
>
> Given that, I would suggest we back out the patch, and that when  
> __builtin_not_reached(); is supported, we can simply do:
>
> #if __GNUC__ is recent enough
> # define not_reached() __builtin_not_reached()
> #else
> # define not_reached() for(;;)
> #endif
>
> OK?

Yeah, sounds good!

	Ingo