* [PATCH] MIPS: Make BUG() __noreturn.
@ 2008-11-21 1:26 David Daney
2008-11-21 10:00 ` Alan Cox
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: David Daney @ 2008-11-21 1:26 UTC (permalink / raw)
To: linux-mips; +Cc: linux-kernel
MIPS: Make BUG() __noreturn.
Often we do things like put BUG() in the default clause of a case
statement. Since it was not declared __noreturn, this could sometimes
lead to bogus compiler warnings that variables were used
uninitialized.
There is a small problem in that we have to put a magic while(1); loop to
fool GCC into really thinking it is noreturn. This makes the new
BUG() function 3 instructions long instead of just 1, but I think it
is worth it as it is now unnecessary to do extra work to silence the
'used uninitialized' warnings.
I also re-wrote BUG_ON so that if it is given a constant condition, it
just does BUG() instead of loading a constant value in to a register
and testing it.
Signed-off-by: David Daney <ddaney@caviumnetworks.com>
---
arch/mips/include/asm/bug.h | 29 ++++++++++++++++++++---------
1 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/arch/mips/include/asm/bug.h b/arch/mips/include/asm/bug.h
index 7eb63de..08ea468 100644
--- a/arch/mips/include/asm/bug.h
+++ b/arch/mips/include/asm/bug.h
@@ -7,20 +7,31 @@
#include <asm/break.h>
-#define BUG() \
-do { \
- __asm__ __volatile__("break %0" : : "i" (BRK_BUG)); \
-} while (0)
+static inline void __noreturn BUG(void)
+{
+ __asm__ __volatile__("break %0" : : "i" (BRK_BUG));
+ /* Fool GCC into thinking the function doesn't return. */
+ while (1)
+ ;
+}
#define HAVE_ARCH_BUG
#if (_MIPS_ISA > _MIPS_ISA_MIPS1)
-#define BUG_ON(condition) \
-do { \
- __asm__ __volatile__("tne $0, %0, %1" \
- : : "r" (condition), "i" (BRK_BUG)); \
-} while (0)
+static inline void __BUG_ON(unsigned long condition)
+{
+ if (__builtin_constant_p(condition)) {
+ if (condition)
+ BUG();
+ else
+ return;
+ }
+ __asm__ __volatile__("tne $0, %0, %1"
+ : : "r" (condition), "i" (BRK_BUG));
+}
+
+#define BUG_ON(C) __BUG_ON((unsigned long)(C))
#define HAVE_ARCH_BUG_ON
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH] MIPS: Make BUG() __noreturn.
2008-11-21 1:26 [PATCH] MIPS: Make BUG() __noreturn David Daney
@ 2008-11-21 10:00 ` Alan Cox
2008-11-21 10:27 ` Geert Uytterhoeven
2008-11-21 23:00 ` Andrew Morton
2008-11-22 9:39 ` Ralf Baechle
2 siblings, 1 reply; 15+ messages in thread
From: Alan Cox @ 2008-11-21 10:00 UTC (permalink / raw)
To: David Daney; +Cc: linux-mips, linux-kernel
On Thu, 20 Nov 2008 17:26:36 -0800
David Daney <ddaney@caviumnetworks.com> wrote:
> MIPS: Make BUG() __noreturn.
>
> Often we do things like put BUG() in the default clause of a case
> statement. Since it was not declared __noreturn, this could sometimes
> lead to bogus compiler warnings that variables were used
> uninitialized.
>
> There is a small problem in that we have to put a magic while(1); loop to
> fool GCC into really thinking it is noreturn.
That sounds like your __noreturn macro is wrong.
Try using __attribute__ ((__noreturn__))
if that works then fix up the __noreturn definitions for the MIPS and gcc
you have.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MIPS: Make BUG() __noreturn.
2008-11-21 10:00 ` Alan Cox
@ 2008-11-21 10:27 ` Geert Uytterhoeven
2008-11-21 11:14 ` Maciej W. Rozycki
2008-11-21 16:40 ` David Daney
0 siblings, 2 replies; 15+ messages in thread
From: Geert Uytterhoeven @ 2008-11-21 10:27 UTC (permalink / raw)
To: Alan Cox; +Cc: David Daney, linux-mips, linux-kernel
On Fri, 21 Nov 2008, Alan Cox wrote:
> On Thu, 20 Nov 2008 17:26:36 -0800
> David Daney <ddaney@caviumnetworks.com> wrote:
>
> > MIPS: Make BUG() __noreturn.
> >
> > Often we do things like put BUG() in the default clause of a case
> > statement. Since it was not declared __noreturn, this could sometimes
> > lead to bogus compiler warnings that variables were used
> > uninitialized.
> >
> > There is a small problem in that we have to put a magic while(1); loop to
> > fool GCC into really thinking it is noreturn.
>
> That sounds like your __noreturn macro is wrong.
>
> Try using __attribute__ ((__noreturn__))
>
> if that works then fix up the __noreturn definitions for the MIPS and gcc
> you have.
Nope, gcc is too smart:
$ cat a.c
int f(void) __attribute__((__noreturn__));
int f(void)
{
}
$ gcc -c -Wall a.c
a.c: In function f:
a.c:6: warning: `noreturn' function does return
$
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] MIPS: Make BUG() __noreturn.
2008-11-21 10:27 ` Geert Uytterhoeven
@ 2008-11-21 11:14 ` Maciej W. Rozycki
2008-11-21 12:58 ` Andreas Schwab
2008-11-21 16:40 ` David Daney
1 sibling, 1 reply; 15+ messages in thread
From: Maciej W. Rozycki @ 2008-11-21 11:14 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Alan Cox, David Daney, linux-mips, linux-kernel
On Fri, 21 Nov 2008, Geert Uytterhoeven wrote:
> > That sounds like your __noreturn macro is wrong.
> >
> > Try using __attribute__ ((__noreturn__))
> >
> > if that works then fix up the __noreturn definitions for the MIPS and gcc
> > you have.
>
> Nope, gcc is too smart:
>
> $ cat a.c
>
> int f(void) __attribute__((__noreturn__));
>
> int f(void)
> {
> }
>
> $ gcc -c -Wall a.c
> a.c: In function f:
> a.c:6: warning: `noreturn' function does return
> $
Hmm, in the case of your example the warning is justified, because the
(virtual) "return" statement of your function is in a unconditional block.
Otherwise it looks like the attribute is useless -- it looks like it can
only be used for functions where GCC can determine the function does not
return anyway. Which means it is redundant.
The cases where within the function concerned there is a volatile asm or
a conditional block which cannot be determined with simple static analysis
that it does stop look like legitimate ones for the use of the "noreturn"
attribute and my opinion is GCC should not warn about them with -Wall,
though a separate -W<whatever> option for the inquisitive would make sense
to me. It might be worthwhile to have a look into archives of the GCC
mailing lists to see how the implementation has evolved into the current
form and if no useful conclusion can be made, to bring the issue now
and/or file a bug report.
Maciej
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] MIPS: Make BUG() __noreturn.
2008-11-21 11:14 ` Maciej W. Rozycki
@ 2008-11-21 12:58 ` Andreas Schwab
0 siblings, 0 replies; 15+ messages in thread
From: Andreas Schwab @ 2008-11-21 12:58 UTC (permalink / raw)
To: Maciej W. Rozycki
Cc: Geert Uytterhoeven, Alan Cox, David Daney, linux-mips,
linux-kernel
"Maciej W. Rozycki" <macro@linux-mips.org> writes:
> Otherwise it looks like the attribute is useless -- it looks like it can
> only be used for functions where GCC can determine the function does not
> return anyway. Which means it is redundant.
The purpose of the attribute is to tell the _callers_ of this function
that it does not return. It does not change how the attributed function
itself is compiled.
Andreas.
--
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MIPS: Make BUG() __noreturn.
2008-11-21 10:27 ` Geert Uytterhoeven
2008-11-21 11:14 ` Maciej W. Rozycki
@ 2008-11-21 16:40 ` David Daney
2008-11-21 18:46 ` Geert Uytterhoeven
1 sibling, 1 reply; 15+ messages in thread
From: David Daney @ 2008-11-21 16:40 UTC (permalink / raw)
To: Geert Uytterhoeven, gcc; +Cc: Alan Cox, linux-mips, linux-kernel, Adam Nemet
Geert Uytterhoeven wrote:
> On Fri, 21 Nov 2008, Alan Cox wrote:
>> On Thu, 20 Nov 2008 17:26:36 -0800
>> David Daney <ddaney@caviumnetworks.com> wrote:
>>
>>> MIPS: Make BUG() __noreturn.
>>>
>>> Often we do things like put BUG() in the default clause of a case
>>> statement. Since it was not declared __noreturn, this could sometimes
>>> lead to bogus compiler warnings that variables were used
>>> uninitialized.
>>>
>>> There is a small problem in that we have to put a magic while(1); loop to
>>> fool GCC into really thinking it is noreturn.
>> That sounds like your __noreturn macro is wrong.
>>
>> Try using __attribute__ ((__noreturn__))
>>
>> if that works then fix up the __noreturn definitions for the MIPS and gcc
>> you have.
>
> Nope, gcc is too smart:
>
> $ cat a.c
>
> int f(void) __attribute__((__noreturn__));
>
> int f(void)
> {
> }
>
> $ gcc -c -Wall a.c
> a.c: In function f:
> a.c:6: warning: `noreturn' function does return
> $
>
That's right.
I was discussing this issue with my colleague Adam Nemet, and we came
up with a couple of options:
1) Enhance the _builtin_trap() function so that we can specify the
break code that is emitted. This would allow us to do something
like:
static inline void __attribute__((noreturn)) BUG()
{
__builtin_trap(0x200);
}
2) Create a new builtin '__builtin_noreturn()' that expands to nothing
but has no CFG edges leaving it, which would allow:
static inline void __attribute__((noreturn)) BUG()
{
__asm__ __volatile__("break %0" : : "i" (0x200));
__builtin_noreturn();
}
David Daney
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] MIPS: Make BUG() __noreturn.
2008-11-21 16:40 ` David Daney
@ 2008-11-21 18:46 ` Geert Uytterhoeven
2008-11-21 22:16 ` Ralf Baechle
0 siblings, 1 reply; 15+ messages in thread
From: Geert Uytterhoeven @ 2008-11-21 18:46 UTC (permalink / raw)
To: David Daney; +Cc: gcc, Alan Cox, linux-mips, linux-kernel, Adam Nemet
On Fri, 21 Nov 2008, David Daney wrote:
> Geert Uytterhoeven wrote:
> > On Fri, 21 Nov 2008, Alan Cox wrote:
> > > On Thu, 20 Nov 2008 17:26:36 -0800
> > > David Daney <ddaney@caviumnetworks.com> wrote:
> > >
> > > > MIPS: Make BUG() __noreturn.
> > > >
> > > > Often we do things like put BUG() in the default clause of a case
> > > > statement. Since it was not declared __noreturn, this could sometimes
> > > > lead to bogus compiler warnings that variables were used
> > > > uninitialized.
> > > >
> > > > There is a small problem in that we have to put a magic while(1); loop
> > > > to
> > > > fool GCC into really thinking it is noreturn.
> > > That sounds like your __noreturn macro is wrong.
> > >
> > > Try using __attribute__ ((__noreturn__))
> > >
> > > if that works then fix up the __noreturn definitions for the MIPS and gcc
> > > you have.
> >
> > Nope, gcc is too smart:
> >
> > $ cat a.c
> >
> > int f(void) __attribute__((__noreturn__));
> >
> > int f(void)
> > {
> > }
> >
> > $ gcc -c -Wall a.c
> > a.c: In function f:
> > a.c:6: warning: `noreturn' function does return
> > $
>
> That's right.
>
> I was discussing this issue with my colleague Adam Nemet, and we came
> up with a couple of options:
>
> 1) Enhance the _builtin_trap() function so that we can specify the
> break code that is emitted. This would allow us to do something
> like:
>
> static inline void __attribute__((noreturn)) BUG()
> {
> __builtin_trap(0x200);
> }
>
> 2) Create a new builtin '__builtin_noreturn()' that expands to nothing
> but has no CFG edges leaving it, which would allow:
>
> static inline void __attribute__((noreturn)) BUG()
> {
> __asm__ __volatile__("break %0" : : "i" (0x200));
> __builtin_noreturn();
> }
Now I remember, yes, __builtin_trap() is how we fixed it on m68k:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=e8006b060f3982a969c5170aa869628d54dd30d8
Of course, if you need a different trap code than the default, you're in
trouble.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] MIPS: Make BUG() __noreturn.
2008-11-21 18:46 ` Geert Uytterhoeven
@ 2008-11-21 22:16 ` Ralf Baechle
2008-11-24 19:04 ` Maciej W. Rozycki
0 siblings, 1 reply; 15+ messages in thread
From: Ralf Baechle @ 2008-11-21 22:16 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: David Daney, gcc, Alan Cox, linux-mips, linux-kernel, Adam Nemet
On Fri, Nov 21, 2008 at 07:46:43PM +0100, Geert Uytterhoeven wrote:
> > up with a couple of options:
> >
> > 1) Enhance the _builtin_trap() function so that we can specify the
> > break code that is emitted. This would allow us to do something
> > like:
> >
> > static inline void __attribute__((noreturn)) BUG()
> > {
> > __builtin_trap(0x200);
> > }
I had suggested this one before ...
> > 2) Create a new builtin '__builtin_noreturn()' that expands to nothing
> > but has no CFG edges leaving it, which would allow:
> >
> > static inline void __attribute__((noreturn)) BUG()
> > {
> > __asm__ __volatile__("break %0" : : "i" (0x200));
> > __builtin_noreturn();
> > }
>
> Now I remember, yes, __builtin_trap() is how we fixed it on m68k:
I like this interface.
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=e8006b060f3982a969c5170aa869628d54dd30d8
>
> Of course, if you need a different trap code than the default, you're in
> trouble.
MIPS ISA newer than MIPS I also have conditional break codes allowing
something like this:
#define BUG_ON(condition) \
do { \
__asm__ __volatile__("tne $0, %0, %1" \
: : "r" (condition), "i" (BRK_BUG)); \
} while (0)
that is test of condition and the trap as a single instruction. Note there
are break and trap instructions on MIPS and they are basically doing the
same job ...
Ralf
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] MIPS: Make BUG() __noreturn.
2008-11-21 22:16 ` Ralf Baechle
@ 2008-11-24 19:04 ` Maciej W. Rozycki
0 siblings, 0 replies; 15+ messages in thread
From: Maciej W. Rozycki @ 2008-11-24 19:04 UTC (permalink / raw)
To: Ralf Baechle
Cc: Geert Uytterhoeven, David Daney, gcc, Alan Cox, linux-mips,
linux-kernel, Adam Nemet
On Fri, 21 Nov 2008, Ralf Baechle wrote:
> MIPS ISA newer than MIPS I also have conditional break codes allowing
> something like this:
>
> #define BUG_ON(condition) \
> do { \
> __asm__ __volatile__("tne $0, %0, %1" \
> : : "r" (condition), "i" (BRK_BUG)); \
> } while (0)
>
> that is test of condition and the trap as a single instruction. Note there
> are break and trap instructions on MIPS and they are basically doing the
> same job ...
GCC is actually smart enough to combine sequences like:
if (something)
__builtin_trap();
into appropriate conditional trap instructions on MIPS. As noted by David
trap codes other than the default cannot be emitted this way though.
Maciej
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MIPS: Make BUG() __noreturn.
2008-11-21 1:26 [PATCH] MIPS: Make BUG() __noreturn David Daney
2008-11-21 10:00 ` Alan Cox
@ 2008-11-21 23:00 ` Andrew Morton
2008-11-21 23:48 ` David Daney
2008-11-23 9:58 ` Ingo Molnar
2008-11-22 9:39 ` Ralf Baechle
2 siblings, 2 replies; 15+ messages in thread
From: Andrew Morton @ 2008-11-21 23:00 UTC (permalink / raw)
To: David Daney; +Cc: linux-mips, linux-kernel
On Thu, 20 Nov 2008 17:26:36 -0800
David Daney <ddaney@caviumnetworks.com> wrote:
> MIPS: Make BUG() __noreturn.
>
> Often we do things like put BUG() in the default clause of a case
> statement. Since it was not declared __noreturn, this could sometimes
> lead to bogus compiler warnings that variables were used
> uninitialized.
>
> There is a small problem in that we have to put a magic while(1); loop to
> fool GCC into really thinking it is noreturn. This makes the new
> BUG() function 3 instructions long instead of just 1, but I think it
> is worth it as it is now unnecessary to do extra work to silence the
> 'used uninitialized' warnings.
>
> I also re-wrote BUG_ON so that if it is given a constant condition, it
> just does BUG() instead of loading a constant value in to a register
> and testing it.
>
Yup, this change will fix some compile warnings which will never be
fixed in any other way for mips.
> +static inline void __noreturn BUG(void)
> +{
> + __asm__ __volatile__("break %0" : : "i" (BRK_BUG));
> + /* Fool GCC into thinking the function doesn't return. */
> + while (1)
> + ;
> +}
This kind of sucks, doesn't it? It adds instructions into the kernel
text, very frequently on fast paths. Those instructions are never
executed, and we're blowing away i-cache just to quash compiler
warnings.
For example, this:
--- a/arch/x86/include/asm/bug.h~a
+++ a/arch/x86/include/asm/bug.h
@@ -22,14 +22,12 @@ do { \
".popsection" \
: : "i" (__FILE__), "i" (__LINE__), \
"i" (sizeof(struct bug_entry))); \
- for (;;) ; \
} while (0)
#else
#define BUG() \
do { \
asm volatile("ud2"); \
- for (;;) ; \
} while (0)
#endif
_
reduces the size of i386 mm/vmalloc.o text by 56 bytes.
I wonder if there is any clever way in which we can do this without
introducing additional runtime cost.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] MIPS: Make BUG() __noreturn.
2008-11-21 23:00 ` Andrew Morton
@ 2008-11-21 23:48 ` David Daney
2008-11-23 9:58 ` Ingo Molnar
1 sibling, 0 replies; 15+ messages in thread
From: David Daney @ 2008-11-21 23:48 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-mips, linux-kernel, gcc
Andrew Morton wrote:
>
>
> Yup, this change will fix some compile warnings which will never be
> fixed in any other way for mips.
>
>> +static inline void __noreturn BUG(void)
>> +{
>> + __asm__ __volatile__("break %0" : : "i" (BRK_BUG));
>> + /* Fool GCC into thinking the function doesn't return. */
>> + while (1)
>> + ;
>> +}
>
> This kind of sucks, doesn't it? It adds instructions into the kernel
> text, very frequently on fast paths. Those instructions are never
> executed, and we're blowing away i-cache just to quash compiler
> warnings.
>
> For example, this:
>
> --- a/arch/x86/include/asm/bug.h~a
> +++ a/arch/x86/include/asm/bug.h
> @@ -22,14 +22,12 @@ do { \
> ".popsection" \
> : : "i" (__FILE__), "i" (__LINE__), \
> "i" (sizeof(struct bug_entry))); \
> - for (;;) ; \
> } while (0)
>
> #else
> #define BUG() \
> do { \
> asm volatile("ud2"); \
> - for (;;) ; \
> } while (0)
> #endif
>
> _
>
> reduces the size of i386 mm/vmalloc.o text by 56 bytes.
>
> I wonder if there is any clever way in which we can do this without
> introducing additional runtime cost.
As I said in the other part of the thread, We are working on a GCC patch
that adds a new built-in function '__builtin_noreturn()', that you could
substitute for 'for(;;);' that emits no instructions in this case.
David Daney
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] MIPS: Make BUG() __noreturn.
2008-11-21 23:00 ` Andrew Morton
2008-11-21 23:48 ` David Daney
@ 2008-11-23 9:58 ` Ingo Molnar
2008-11-24 9:20 ` Ralf Baechle
2008-11-25 0:16 ` Jeremy Fitzhardinge
1 sibling, 2 replies; 15+ messages in thread
From: Ingo Molnar @ 2008-11-23 9:58 UTC (permalink / raw)
To: Andrew Morton; +Cc: David Daney, linux-mips, linux-kernel
* Andrew Morton <akpm@linux-foundation.org> wrote:
> > +static inline void __noreturn BUG(void)
> > +{
> > + __asm__ __volatile__("break %0" : : "i" (BRK_BUG));
> > + /* Fool GCC into thinking the function doesn't return. */
> > + while (1)
> > + ;
> > +}
>
> This kind of sucks, doesn't it? It adds instructions into the
> kernel text, very frequently on fast paths. Those instructions are
> never executed, and we're blowing away i-cache just to quash
> compiler warnings.
>
> For example, this:
>
> --- a/arch/x86/include/asm/bug.h~a
> +++ a/arch/x86/include/asm/bug.h
> @@ -22,14 +22,12 @@ do { \
> ".popsection" \
> : : "i" (__FILE__), "i" (__LINE__), \
> "i" (sizeof(struct bug_entry))); \
> - for (;;) ; \
> } while (0)
>
> #else
> #define BUG() \
> do { \
> asm volatile("ud2"); \
> - for (;;) ; \
> } while (0)
> #endif
>
> _
>
> reduces the size of i386 mm/vmalloc.o text by 56 bytes.
yes - the total image effect is significantly - recently looked at how
much larger !CONFIG_BUG builds would get if we inserted an infinite
loop into them - it was in the 50K text range (!).
but in the x86 ud2 case we could guarantee that we wont ever return
from that exception. Mind sending a patch with a signoff, a
description and an infinite loop in the u2d handler?
Ingo
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] MIPS: Make BUG() __noreturn.
2008-11-23 9:58 ` Ingo Molnar
@ 2008-11-24 9:20 ` Ralf Baechle
2008-11-25 0:16 ` Jeremy Fitzhardinge
1 sibling, 0 replies; 15+ messages in thread
From: Ralf Baechle @ 2008-11-24 9:20 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Andrew Morton, David Daney, linux-mips, linux-kernel
On Sun, Nov 23, 2008 at 10:58:18AM +0100, Ingo Molnar wrote:
> yes - the total image effect is significantly - recently looked at how
> much larger !CONFIG_BUG builds would get if we inserted an infinite
> loop into them - it was in the 50K text range (!).
>
> but in the x86 ud2 case we could guarantee that we wont ever return
> from that exception. Mind sending a patch with a signoff, a
> description and an infinite loop in the u2d handler?
The infinite loop is necessary to keep gcc from creating pointless warnings.
But I did play a bit further with bug.h, this time on x86. Result below.
Ralf
[PATCH] x86: Optimize BUG() codesize.
Turning the i386 BUG() into an inline function shaves off 4064 bytes for
a defconfig kernel and 16 bytes for the same kernel with
CONFIG_DEBUG_BUGVERBOSE cleared. Tested with gcc 4.3.0.
Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index 3def206..3b3bf2a 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -1,9 +1,10 @@
#ifndef _ASM_X86_BUG_H
#define _ASM_X86_BUG_H
-#ifdef CONFIG_BUG
#define HAVE_ARCH_BUG
+#include <asm-generic/bug.h>
+#ifdef CONFIG_BUG
#ifdef CONFIG_DEBUG_BUGVERBOSE
#ifdef CONFIG_X86_32
@@ -12,28 +13,27 @@
# define __BUG_C0 "2:\t.quad 1b, %c0\n"
#endif
-#define BUG() \
-do { \
- asm volatile("1:\tud2\n" \
- ".pushsection __bug_table,\"a\"\n" \
- __BUG_C0 \
- "\t.word %c1, 0\n" \
- "\t.org 2b+%c2\n" \
- ".popsection" \
- : : "i" (__FILE__), "i" (__LINE__), \
- "i" (sizeof(struct bug_entry))); \
- for (;;) ; \
-} while (0)
+static inline void BUG(void)
+{
+ asm volatile("1:\tud2\n"
+ ".pushsection __bug_table,\"a\"\n"
+ __BUG_C0
+ "\t.word %c1, 0\n"
+ "\t.org 2b+%c2\n"
+ ".popsection"
+ : : "i" (__FILE__), "i" (__LINE__),
+ "i" (sizeof(struct bug_entry)));
+ for (;;) ;
+}
#else
-#define BUG() \
-do { \
- asm volatile("ud2"); \
- for (;;) ; \
-} while (0)
+static inline void BUG(void)
+{
+ asm volatile("ud2");
+ for (;;) ;
+}
#endif
#endif /* !CONFIG_BUG */
-#include <asm-generic/bug.h>
#endif /* _ASM_X86_BUG_H */
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH] MIPS: Make BUG() __noreturn.
2008-11-23 9:58 ` Ingo Molnar
2008-11-24 9:20 ` Ralf Baechle
@ 2008-11-25 0:16 ` Jeremy Fitzhardinge
1 sibling, 0 replies; 15+ messages in thread
From: Jeremy Fitzhardinge @ 2008-11-25 0:16 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Andrew Morton, David Daney, linux-mips, linux-kernel
Ingo Molnar wrote:
> * Andrew Morton <akpm@linux-foundation.org> wrote:
>
>
>>> +static inline void __noreturn BUG(void)
>>> +{
>>> + __asm__ __volatile__("break %0" : : "i" (BRK_BUG));
>>> + /* Fool GCC into thinking the function doesn't return. */
>>> + while (1)
>>> + ;
>>> +}
>>>
>> This kind of sucks, doesn't it? It adds instructions into the
>> kernel text, very frequently on fast paths. Those instructions are
>> never executed, and we're blowing away i-cache just to quash
>> compiler warnings.
>>
>> For example, this:
>>
>> --- a/arch/x86/include/asm/bug.h~a
>> +++ a/arch/x86/include/asm/bug.h
>> @@ -22,14 +22,12 @@ do { \
>> ".popsection" \
>> : : "i" (__FILE__), "i" (__LINE__), \
>> "i" (sizeof(struct bug_entry))); \
>> - for (;;) ; \
>> } while (0)
>>
>> #else
>> #define BUG() \
>> do { \
>> asm volatile("ud2"); \
>> - for (;;) ; \
>> } while (0)
>> #endif
>>
>> _
>>
>> reduces the size of i386 mm/vmalloc.o text by 56 bytes.
>>
>
> yes - the total image effect is significantly - recently looked at how
> much larger !CONFIG_BUG builds would get if we inserted an infinite
> loop into them - it was in the 50K text range (!).
>
> but in the x86 ud2 case we could guarantee that we wont ever return
> from that exception. Mind sending a patch with a signoff, a
> description and an infinite loop in the u2d handler?
>
There are two arguments against making BUG() a noreturn:
* if you compile without BUG enabled, then it won't be noreturn anyway
* making it noreturn kills the lifetime of any variables that would
otherwise be considered alive, making the DWARF debug info at that
point less reliable (which is a pain even for post-mortem debugging)
The counter-argument is that not making it noreturn will keep variables
alive that wouldn't otherwise be, causing greater register pressure,
spillage, etc.
If adding an infinite loop really adds 50k to the image, the extra size
must come from the changes to variable lifetime rather than the loop
instructions themselves (which are only 2 bytes per instance, and we
don't have 25,000 BUGs in the kernel, do we?).
J
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] MIPS: Make BUG() __noreturn.
2008-11-21 1:26 [PATCH] MIPS: Make BUG() __noreturn David Daney
2008-11-21 10:00 ` Alan Cox
2008-11-21 23:00 ` Andrew Morton
@ 2008-11-22 9:39 ` Ralf Baechle
2 siblings, 0 replies; 15+ messages in thread
From: Ralf Baechle @ 2008-11-22 9:39 UTC (permalink / raw)
To: David Daney; +Cc: linux-mips, linux-kernel
On Thu, Nov 20, 2008 at 05:26:36PM -0800, David Daney wrote:
> From: David Daney <ddaney@caviumnetworks.com>
> Date: Thu, 20 Nov 2008 17:26:36 -0800
> To: linux-mips <linux-mips@linux-mips.org>
> CC: linux-kernel@vger.kernel.org
> Subject: [PATCH] MIPS: Make BUG() __noreturn.
> Content-Type: text/plain; charset=ISO-8859-1; format=flowed
>
> MIPS: Make BUG() __noreturn.
Please don't repeat the subject in the body of a patch email. Git takes
the subject followed by the body upto the --- line as the log message so
this is just duplication that will need to be manually deleted again.
> Often we do things like put BUG() in the default clause of a case
> statement. Since it was not declared __noreturn, this could sometimes
> lead to bogus compiler warnings that variables were used
> uninitialized.
>
> There is a small problem in that we have to put a magic while(1); loop to
> fool GCC into really thinking it is noreturn. This makes the new
> BUG() function 3 instructions long instead of just 1, but I think it
> is worth it as it is now unnecessary to do extra work to silence the
> 'used uninitialized' warnings.
>
> I also re-wrote BUG_ON so that if it is given a constant condition, it
> just does BUG() instead of loading a constant value in to a register
> and testing it.
I don't like the endless loop in the BUG() macros but at this time it seems
the best solution. Looking forward to __builtin_noreturn().
Patch applied,
Ralf
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2008-11-25 0:16 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-11-21 1:26 [PATCH] MIPS: Make BUG() __noreturn David Daney
2008-11-21 10:00 ` Alan Cox
2008-11-21 10:27 ` Geert Uytterhoeven
2008-11-21 11:14 ` Maciej W. Rozycki
2008-11-21 12:58 ` Andreas Schwab
2008-11-21 16:40 ` David Daney
2008-11-21 18:46 ` Geert Uytterhoeven
2008-11-21 22:16 ` Ralf Baechle
2008-11-24 19:04 ` Maciej W. Rozycki
2008-11-21 23:00 ` Andrew Morton
2008-11-21 23:48 ` David Daney
2008-11-23 9:58 ` Ingo Molnar
2008-11-24 9:20 ` Ralf Baechle
2008-11-25 0:16 ` Jeremy Fitzhardinge
2008-11-22 9:39 ` Ralf Baechle
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox