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 <[email protected]>
---
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
On Thu, 20 Nov 2008 17:26:36 -0800
David Daney <[email protected]> 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.
On Fri, 21 Nov 2008, Alan Cox wrote:
> On Thu, 20 Nov 2008 17:26:36 -0800
> David Daney <[email protected]> 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 -- [email protected]
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
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
"Maciej W. Rozycki" <[email protected]> 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, [email protected]
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."
Geert Uytterhoeven wrote:
> On Fri, 21 Nov 2008, Alan Cox wrote:
>> On Thu, 20 Nov 2008 17:26:36 -0800
>> David Daney <[email protected]> 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
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 <[email protected]> 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 -- [email protected]
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
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
On Thu, 20 Nov 2008 17:26:36 -0800
David Daney <[email protected]> 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.
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
On Thu, Nov 20, 2008 at 05:26:36PM -0800, David Daney wrote:
> From: David Daney <[email protected]>
> Date: Thu, 20 Nov 2008 17:26:36 -0800
> To: linux-mips <[email protected]>
> CC: [email protected]
> 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
* Andrew Morton <[email protected]> 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
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 <[email protected]>
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 */
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
Ingo Molnar wrote:
> * Andrew Morton <[email protected]> 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