[PATCH] BUG(): CONFIG_BUG=n version of BUG() should be unreachable()

[PATCH] BUG(): CONFIG_BUG=n version of BUG() should be unreachable()

[Alexander Beregalov]

Previouss definition of BUG() as 'do {} while(0)' produced compilation
warnings when BUG() was used in default branch of switch() statement
(control reaches end of non-void function).

Example:
unsigned long function()
{
	switch() {
	case 1:
		return 1;
	case 2:
		return 2;
	default:
		BUG();
}

Using unreachable() fixes the problem.

Signed-off-by: Alexander Beregalov <a.beregalov@gmail.com>

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 18c435d..1106439 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -89,7 +89,7 @@ extern void warn_slowpath_null(const char *file, const int line);
 
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
-#define BUG() do {} while(0)
+#define BUG() unreachable() 
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-- 
1.6.6.rc4

Re: [PATCH] BUG(): CONFIG_BUG=n version of BUG() should be unreachable()

[David Daney]

Alexander Beregalov wrote:
> Previouss definition of BUG() as 'do {} while(0)' produced compilation
> warnings when BUG() was used in default branch of switch() statement
> (control reaches end of non-void function).
> 
> Example:
> unsigned long function()
> {
> 	switch() {
> 	case 1:
> 		return 1;
> 	case 2:
> 		return 2;
> 	default:
> 		BUG();
> }
> 
> Using unreachable() fixes the problem.
> 
> Signed-off-by: Alexander Beregalov <a.beregalov@gmail.com>
> 

NAK.


> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index 18c435d..1106439 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -89,7 +89,7 @@ extern void warn_slowpath_null(const char *file, const int line);
>  
>  #else /* !CONFIG_BUG */
>  #ifndef HAVE_ARCH_BUG
> -#define BUG() do {} while(0)
> +#define BUG() unreachable() 
>  #endif
>  
>  #ifndef HAVE_ARCH_BUG_ON

You can only use 'unreachable()' in situations where it is truly 
unreachable.  In the case above you will reach it in the default case.

I would suggest one of the following instead:

#define BUG() BUILD_BUG_ON(1)

#define BUG() do {} while(1)


David Daney

Re: [PATCH] BUG(): CONFIG_BUG=n version of BUG() should be unreachable()

[David Daney]

David Daney wrote:
> Alexander Beregalov wrote:
>> Previouss definition of BUG() as 'do {} while(0)' produced compilation
>> warnings when BUG() was used in default branch of switch() statement
>> (control reaches end of non-void function).
>>
>> Example:
>> unsigned long function()
>> {
>>     switch() {
>>     case 1:
>>         return 1;
>>     case 2:
>>         return 2;
>>     default:
>>         BUG();
>> }
>>
>> Using unreachable() fixes the problem.
>>
>> Signed-off-by: Alexander Beregalov <a.beregalov@gmail.com>
>>
> 
> NAK.
> 

Well that may be too strong an objection, but I would really recommend 
deeper consideration.

If you use: #define BUG() __builtin_unreachable()

which is what your patch does for GCC >= 4.5, it is truly undefined what 
happens if it is ever reached.  One typical thing that might happen is 
that you start to try to execute data.  It is unclear to me if it is 
preferable in the kernel to do that, rather than loop endlessly.  You 
would likely achieve smaller code, but at what cost?

David Daney

> 
>> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
>> index 18c435d..1106439 100644
>> --- a/include/asm-generic/bug.h
>> +++ b/include/asm-generic/bug.h
>> @@ -89,7 +89,7 @@ extern void warn_slowpath_null(const char *file, 
>> const int line);
>>  
>>  #else /* !CONFIG_BUG */
>>  #ifndef HAVE_ARCH_BUG
>> -#define BUG() do {} while(0)
>> +#define BUG() unreachable()  #endif
>>  
>>  #ifndef HAVE_ARCH_BUG_ON
> 
> You can only use 'unreachable()' in situations where it is truly 
> unreachable.  In the case above you will reach it in the default case.
> 
> I would suggest one of the following instead:
> 
> #define BUG() BUILD_BUG_ON(1)
> 
> #define BUG() do {} while(1)
> 
> 
> David Daney
> 

[PATCH] BUG(): CONFIG_BUG=n version of BUG() should be unreachable()

[Sam Ravnborg]

Previouss definition of BUG() as 'do {} while(0)' produced compilation
warnings when BUG() was used in default branch of switch() statement
(control reaches end of non-void function).

Example:
unsigned long function()
{
	switch() {
	case 1:
		return 1;
	case 2:
		return 2;
	default:
		BUG();
}

Using unreachable() fixes the problem.

Signed-off-by: Alexander Beregalov <a.beregalov@gmail.com>
[sam: add "for(;;) ;" so code indeed does not return]
Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: David Daney <ddaney@caviumnetworks.com>
---

The patch I had in mind was this.
We introduce an endless loop and just to be sure we tell gcc
that we never get past this.
This also address the concerns rasied by David.

Arnd - this is asm-generic stuff. If you agree (and Alexander can confirm
that it works for him) then I expect you to take it.

	Sam


diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 18c435d..f540529 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -89,7 +89,12 @@ extern void warn_slowpath_null(const char *file, const int line);
 
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
-#define BUG() do {} while(0)
+/* We do not want to return from here */
+#define BUG() do {							\
+		for (;;)						\
+			/* endless loop*/;				\
+		unreachable();						\
+} while(0)
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON

Re: [PATCH] BUG(): CONFIG_BUG=n version of BUG() should be unreachable()

[Arnd Bergmann]

On Wednesday 23 December 2009, David Daney wrote:
> David Daney wrote:
> 
> Well that may be too strong an objection, but I would really recommend 
> deeper consideration.
> 
> If you use: #define BUG() __builtin_unreachable()
> 
> which is what your patch does for GCC >= 4.5, it is truly undefined what 
> happens if it is ever reached.  One typical thing that might happen is 
> that you start to try to execute data.  It is unclear to me if it is 
> preferable in the kernel to do that, rather than loop endlessly.  You 
> would likely achieve smaller code, but at what cost?

That is exactly what I was about to reply at first as well, but the
definition is BUG() is really "this should never happen". Normally,
i.e. CONFIG_BUG=y, we will print a stack dump and kill the running
task here. The case that Alexander is patching is for !CONFIG_BUG,
where we intentionally remove the handling for the unexpected bug
in order to reduce code size. This option is really just for people
that want to squeeze out every possibly byte from the kernel object
code, while everyone else just enables CONFIG_BUG.

Currently, this is "do { } while (0)", which on old compilers is
the best approximation of doing the right thing there, but may
cause build warnings.

__builtin_unreachable() is even better on gcc-4.5, because gcc may
save a few more instructions and not print warnings any more. Getting
into an undefined state here is not an issue, because if we get to 
a BUG() statement, the system state is already known to be broken
and !CONFIG_BUG means we don't even try to to improve it any more.

The alternative "do { } while (1)" is not ideal, because an
endless loop still requires more code (typically one instruction)
than doing nothing at all.

If there are only than a handful of places that actually cause a warning,
using "do { } while (0)" (or __builtin_unreachable where available) and
fixing up the code using it might be better.

	Arnd

Re: [PATCH] BUG(): CONFIG_BUG=n version of BUG() should be unreachable()

[David Daney]

Arnd Bergmann wrote:
> On Wednesday 23 December 2009, David Daney wrote:
>> David Daney wrote:
>>
>> Well that may be too strong an objection, but I would really recommend 
>> deeper consideration.
>>
>> If you use: #define BUG() __builtin_unreachable()
>>
>> which is what your patch does for GCC >= 4.5, it is truly undefined what 
>> happens if it is ever reached.  One typical thing that might happen is 
>> that you start to try to execute data.  It is unclear to me if it is 
>> preferable in the kernel to do that, rather than loop endlessly.  You 
>> would likely achieve smaller code, but at what cost?
> 
> That is exactly what I was about to reply at first as well, but the
> definition is BUG() is really "this should never happen". Normally,
> i.e. CONFIG_BUG=y, we will print a stack dump and kill the running
> task here. The case that Alexander is patching is for !CONFIG_BUG,
> where we intentionally remove the handling for the unexpected bug
> in order to reduce code size. This option is really just for people
> that want to squeeze out every possibly byte from the kernel object
> code, while everyone else just enables CONFIG_BUG.
> 
> Currently, this is "do { } while (0)", which on old compilers is
> the best approximation of doing the right thing there, but may
> cause build warnings.
> 
> __builtin_unreachable() is even better on gcc-4.5, because gcc may
> save a few more instructions and not print warnings any more. Getting
> into an undefined state here is not an issue, because if we get to 
> a BUG() statement, the system state is already known to be broken
> and !CONFIG_BUG means we don't even try to to improve it any more.
> 
> The alternative "do { } while (1)" is not ideal, because an
> endless loop still requires more code (typically one instruction)
> than doing nothing at all.
> 

Well "do { } while (1)" is exactly the expansion of unreachable() for 
GCC < 4.5.  Since GCC-4.5 has not been released yet, most people will 
get these extra looping instructions if you change BUG in this way.


You might also consider the discussions here:

http://marc.info/?l=linux-kernel&m=125296549116694&w=2

When I suggested a similar patch.


> If there are only than a handful of places that actually cause a warning,
> using "do { } while (0)" (or __builtin_unreachable where available) and
> fixing up the code using it might be better.
> 
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] BUG(): CONFIG_BUG=n version of BUG() should be unreachable()

[Arnd Bergmann]

On Monday 04 January 2010, David Daney wrote:
> Arnd Bergmann wrote:
> > The alternative "do { } while (1)" is not ideal, because an
> > endless loop still requires more code (typically one instruction)
> > than doing nothing at all.
> > 
> 
> Well "do { } while (1)" is exactly the expansion of unreachable() for 
> GCC < 4.5.  Since GCC-4.5 has not been released yet, most people will 
> get these extra looping instructions if you change BUG in this way.

Yes, that is why I wrote the final paragraph, saying
 
> > If there are only than a handful of places that actually cause a warning,
> > using "do { } while (0)" (or __builtin_unreachable where available) and
> > fixing up the code using it might be better.

	Arnd

Re: [PATCH] BUG(): CONFIG_BUG=n version of BUG() should be unreachable()

[David Howells]

Sam Ravnborg <sam@ravnborg.org> wrote:

> +#define BUG() do {							\
> +		for (;;)						\
> +			/* endless loop*/;				\
> +		unreachable();						\
> +} while(0)

Can you not do:

	#define BUG() do {						\
			unreachable();					\
	} while(1)

instead?  If the compiler is interpreting unreachable() to really mean that
what comes after will not be reached, then the condition/loop at the end of
the block should be optimised away.

David

Re: [PATCH] BUG(): CONFIG_BUG=n version of BUG() should be unreachable()

[Arnd Bergmann]

On Tuesday 05 January 2010, David Howells wrote:
> Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> > +#define BUG() do {                                                   \
> > +             for (;;)                                                \
> > +                     /* endless loop*/;                              \
> > +             unreachable();                                          \
> > +} while(0)
> 
> Can you not do:
> 
>         #define BUG() do {                                              \
>                         unreachable();                                  \
>         } while(1)
> 
> instead?  If the compiler is interpreting unreachable() to really mean that
> what comes after will not be reached, then the condition/loop at the end of
> the block should be optimised away.

Forcing the loop here is really wrong because it needlessly
causes extra code to be emitted. We don't really want controlled
error handling here (that is the definition of CONFIG_BUG=n),
so this is only about shutting up the compiler warning.

I guess the best would be something like
#if defined (__GNUC__) && (__GNUC_MAJOR__ == 4) && (__GNUC_MINOR__ >= 5)
#define BUG() __builtin_unreachable()
#else
#define BUG() do { } while (0) /* this may cause a warning */
#endif

I still haven't found out how many warnings we are talking about
here, maybe we can just silence them by adding individual
unreachable() statements after BUG();

	Arnd