Prev: [PATCH 01/25] x86: fix size for ex trampoline with 32bit
Next: [patch 1/7] [patch] DT3155: Use pci_get_device
From: David Daney on 22 Dec 2009 20:30
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(a)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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Daney on 22 Dec 2009 20:40
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(a)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
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 30 Dec 2009 14:20
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 5 Jan 2010 06:40
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Howells on 5 Jan 2010 13:00
Sam Ravnborg <sam(a)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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: [PATCH 01/25] x86: fix size for ex trampoline with 32bit
Next: [patch 1/7] [patch] DT3155: Use pci_get_device