When I sent the first version, I had not realized that Roland McGrath
had only a day or two earlier submitted a very similar patch (although
one that only fixed up the x86 case).
I have been working on this quite a while now, starting with adding
the required support to GCC, so with an eye towards finishing it up I
have this new version.
From the announcement of the first version:
Starting with version 4.5, GCC has a new built-in function called
__builtin_unreachable(). The function tells the compiler that control
flow will never reach that point. Currently we trick the compiler by
putting in for(;;); but this has the disadvantage that extra code is
emitted for an endless loop. For an i386 kernel using
__builtin_unreachable() results in an defaultconfig that is nearly 4000
bytes smaller.
This patch set adds support to compiler.h creating a
new macro usable in the kernel called unreachable(). If the compiler
lacks __builtin_unreachable(), it just expands to for(;;).
The x86 and MIPS patches I actually tested with a GCC-4.5 snapshot.
Lacking the ability to test the rest of the architectures, I just did
what seemed right without even trying to compile the kernel.
For version 2:
I fixed a couple of checkpatch issues, and simplified the
unreachable() macro for the pre-GCC-4.5 case (as suggested by Richard
Henderson). Also several Acked-by: were added.
New in this version (as suggested by Ingo Molnar) I added 11/11 which
uses unreachable() in asm-generic/bug.h for !CONFIG_BUG case. This
one may be a little controversial as it will end up making code
slightly larger when !CONFIG_BUG and you are using a pre-GCC-4.5
compiler.
I will reply with the 11 patches.
David Daney (11):
Add support for GCC-4.5's __builtin_unreachable() to compiler.h (v2)
x86: Convert BUG() to use unreachable()
MIPS: Convert BUG() to use unreachable()
s390: Convert BUG() to use unreachable()
mn10300: Convert BUG() to use unreachable()
parisc: Convert BUG() to use unreachable()
powerpc: Convert BUG() to use unreachable()
alpha: Convert BUG() to use unreachable()
avr32: Convert BUG() to use unreachable()
blackfin: Convert BUG() to use unreachable()
Use unreachable() in asm-generic/bug.h for !CONFIG_BUG case.
arch/alpha/include/asm/bug.h | 3 ++-
arch/avr32/include/asm/bug.h | 2 +-
arch/blackfin/include/asm/bug.h | 2 +-
arch/mips/include/asm/bug.h | 4 +---
arch/mn10300/include/asm/bug.h | 3 ++-
arch/parisc/include/asm/bug.h | 4 ++--
arch/powerpc/include/asm/bug.h | 2 +-
arch/s390/include/asm/bug.h | 2 +-
arch/x86/include/asm/bug.h | 4 ++--
include/asm-generic/bug.h | 4 ++--
include/linux/compiler-gcc4.h | 14 ++++++++++++++
include/linux/compiler.h | 5 +++++
12 files changed, 34 insertions(+), 15 deletions(-)
Starting with version 4.5, GCC has a new built-in function
__builtin_unreachable() that can be used in places like the kernel's
BUG() where inline assembly is used to transfer control flow. This
eliminated the need for an endless loop in these places.
The patch adds a new macro 'unreachable()' that will expand to either
__builtin_unreachable() or an endless loop depending on the compiler
version.
Change from v1: Simplify unreachable() for non-GCC 4.5 case.
Signed-off-by: David Daney <[email protected]>
Acked-by: Ralf Baechle <[email protected]>
---
include/linux/compiler-gcc4.h | 14 ++++++++++++++
include/linux/compiler.h | 5 +++++
2 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
index 450fa59..ab3af40 100644
--- a/include/linux/compiler-gcc4.h
+++ b/include/linux/compiler-gcc4.h
@@ -36,4 +36,18 @@
the kernel context */
#define __cold __attribute__((__cold__))
+
+#if __GNUC_MINOR__ >= 5
+/*
+ * Mark a position in code as unreachable. This can be used to
+ * suppress control flow warnings after asm blocks that transfer
+ * control elsewhere.
+ *
+ * Early snapshots of gcc 4.5 don't support this and we can't detect
+ * this in the preprocessor, but we can live with this because they're
+ * unreleased. Really, we need to have autoconf for the kernel.
+ */
+#define unreachable() __builtin_unreachable()
+#endif
+
#endif
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 04fb513..59f2089 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -144,6 +144,11 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
# define barrier() __memory_barrier()
#endif
+/* Unreachable code */
+#ifndef unreachable
+# define unreachable() do { } while (1)
+#endif
+
#ifndef RELOC_HIDE
# define RELOC_HIDE(ptr, off) \
({ unsigned long __ptr; \
--
1.6.2.5
Use the new unreachable() macro instead of for(;;);. When
allyesconfig is built with a GCC-4.5 snapshot on i686 the size of the
text segment is reduced by 3987 bytes (from 6827019 to 6823032).
Signed-off-by: David Daney <[email protected]>
Acked-by: "H. Peter Anvin" <[email protected]>
CC: Thomas Gleixner <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: [email protected]
---
arch/x86/include/asm/bug.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index d9cf1cd..f654d1b 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -22,14 +22,14 @@ do { \
".popsection" \
: : "i" (__FILE__), "i" (__LINE__), \
"i" (sizeof(struct bug_entry))); \
- for (;;) ; \
+ unreachable(); \
} while (0)
#else
#define BUG() \
do { \
asm volatile("ud2"); \
- for (;;) ; \
+ unreachable(); \
} while (0)
#endif
--
1.6.2.5
Use the new unreachable() macro instead of while(1);
Signed-off-by: David Daney <[email protected]>
Acked-by: Ralf Baechle <[email protected]>
---
arch/mips/include/asm/bug.h | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/arch/mips/include/asm/bug.h b/arch/mips/include/asm/bug.h
index 6cf29c2..540c98a 100644
--- a/arch/mips/include/asm/bug.h
+++ b/arch/mips/include/asm/bug.h
@@ -11,9 +11,7 @@
static inline void __noreturn BUG(void)
{
__asm__ __volatile__("break %0" : : "i" (BRK_BUG));
- /* Fool GCC into thinking the function doesn't return. */
- while (1)
- ;
+ unreachable();
}
#define HAVE_ARCH_BUG
--
1.6.2.5
Use the new unreachable() macro instead of for(;;);
Signed-off-by: David Daney <[email protected]>
Acked-by: Martin Schwidefsky <[email protected]>
CC: Heiko Carstens <[email protected]>
CC: [email protected]
CC: [email protected]
---
arch/s390/include/asm/bug.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/s390/include/asm/bug.h b/arch/s390/include/asm/bug.h
index 7efd0ab..efb74fd 100644
--- a/arch/s390/include/asm/bug.h
+++ b/arch/s390/include/asm/bug.h
@@ -49,7 +49,7 @@
#define BUG() do { \
__EMIT_BUG(0); \
- for (;;); \
+ unreachable(); \
} while (0)
#define WARN_ON(x) ({ \
--
1.6.2.5
Use the new unreachable() macro instead of while(1).
Signed-off-by: David Daney <[email protected]>
CC: David Howells <[email protected]>
CC: Koichi Yasutake <[email protected]>
CC: [email protected]
---
arch/mn10300/include/asm/bug.h | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/arch/mn10300/include/asm/bug.h b/arch/mn10300/include/asm/bug.h
index aa6a388..447a7e6 100644
--- a/arch/mn10300/include/asm/bug.h
+++ b/arch/mn10300/include/asm/bug.h
@@ -27,7 +27,8 @@ do { \
: \
: "i"(__FILE__), "i"(__LINE__) \
); \
-} while (1)
+ unreachable(); \
+} while (0)
#define HAVE_ARCH_BUG
#endif /* CONFIG_BUG */
--
1.6.2.5
Use the new unreachable() macro instead of for(;;);
Signed-off-by: David Daney <[email protected]>
CC: Kyle McMartin <[email protected]>
CC: Helge Deller <[email protected]>
CC: [email protected]
---
arch/parisc/include/asm/bug.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/parisc/include/asm/bug.h b/arch/parisc/include/asm/bug.h
index 8cfc553..75e46c5 100644
--- a/arch/parisc/include/asm/bug.h
+++ b/arch/parisc/include/asm/bug.h
@@ -32,14 +32,14 @@
"\t.popsection" \
: : "i" (__FILE__), "i" (__LINE__), \
"i" (0), "i" (sizeof(struct bug_entry)) ); \
- for(;;) ; \
+ unreachable(); \
} while(0)
#else
#define BUG() \
do { \
asm volatile(PARISC_BUG_BREAK_ASM : : ); \
- for(;;) ; \
+ unreachable(); \
} while(0)
#endif
--
1.6.2.5
Use the new unreachable() macro instead of for(;;);
Signed-off-by: David Daney <[email protected]>
CC: Benjamin Herrenschmidt <[email protected]>
CC: Paul Mackerras <[email protected]>
CC: [email protected]
---
arch/powerpc/include/asm/bug.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index 64e1fdc..2c15212 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -68,7 +68,7 @@
_EMIT_BUG_ENTRY \
: : "i" (__FILE__), "i" (__LINE__), \
"i" (0), "i" (sizeof(struct bug_entry))); \
- for(;;) ; \
+ unreachable(); \
} while (0)
#define BUG_ON(x) do { \
--
1.6.2.5
Use the new unreachable() macro instead of for(;;);
Signed-off-by: David Daney <[email protected]>
CC: Richard Henderson <[email protected]>
CC: Ivan Kokshaysky <[email protected]>
CC: [email protected]
---
arch/alpha/include/asm/bug.h | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/arch/alpha/include/asm/bug.h b/arch/alpha/include/asm/bug.h
index 1720c8a..f091682 100644
--- a/arch/alpha/include/asm/bug.h
+++ b/arch/alpha/include/asm/bug.h
@@ -13,7 +13,8 @@
"call_pal %0 # bugchk\n\t" \
".long %1\n\t.8byte %2" \
: : "i"(PAL_bugchk), "i"(__LINE__), "i"(__FILE__)); \
- for ( ; ; ); } while (0)
+ unreachable(); \
+ } while (0)
#define HAVE_ARCH_BUG
#endif
--
1.6.2.5
Use the new unreachable() macro instead of for(;;);
Signed-off-by: David Daney <[email protected]>
Acked-by: Haavard Skinnemoen <[email protected]>
---
arch/avr32/include/asm/bug.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/avr32/include/asm/bug.h b/arch/avr32/include/asm/bug.h
index 331d45b..2aa373c 100644
--- a/arch/avr32/include/asm/bug.h
+++ b/arch/avr32/include/asm/bug.h
@@ -52,7 +52,7 @@
#define BUG() \
do { \
_BUG_OR_WARN(0); \
- for (;;); \
+ unreachable(); \
} while (0)
#define WARN_ON(condition) \
--
1.6.2.5
Use the new unreachable() macro instead of for(;;);
Signed-off-by: David Daney <[email protected]>
CC: Mike Frysinger <[email protected]>
CC: [email protected]
---
arch/blackfin/include/asm/bug.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/blackfin/include/asm/bug.h b/arch/blackfin/include/asm/bug.h
index 655e495..0b213a9 100644
--- a/arch/blackfin/include/asm/bug.h
+++ b/arch/blackfin/include/asm/bug.h
@@ -41,7 +41,7 @@
#define BUG() \
do { \
_BUG_OR_WARN(0); \
- for (;;); \
+ unreachable(); \
} while (0)
#define WARN_ON(condition) \
--
1.6.2.5
The subject says it all (most). The only drawback here is that for a
pre-GCC-5.4 compiler, instead of expanding to nothing we now expand
BUG() to an endless loop. Before the patch when configured with
!CONFIG_BUG() you might get some warnings, but the code would be
small. After the patch there are no warnings, but there is an endless
loop at each BUG() site.
Of course for the GCC-4.5 case we get the best of both worlds.
Signed-off-by: David Daney <[email protected]>
Suggested-by: Ingo Molnar <[email protected]>
CC: Ingo Molnar <[email protected]>
---
include/asm-generic/bug.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 4b67559..e952242 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -89,11 +89,11 @@ 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
-#define BUG_ON(condition) do { if (condition) ; } while(0)
+#define BUG_ON(condition) do { if (condition) unreachable(); } while (0)
#endif
#ifndef HAVE_ARCH_WARN_ON
--
1.6.2.5
On Mon, Sep 14, 2009 at 5:55 PM, David Daney <[email protected]> wrote:
> The subject says it all (most). The only drawback here is that for a
> pre-GCC-5.4 compiler, instead of expanding to nothing we now expand
> BUG() to an endless loop. Before the patch when configured with
> !CONFIG_BUG() you might get some warnings, but the code would be
> small. After the patch there are no warnings, but there is an endless
> loop at each BUG() site.
>
> Of course for the GCC-4.5 case we get the best of both worlds.
>
> Signed-off-by: David Daney <[email protected]>
> Suggested-by: Ingo Molnar <[email protected]>
> CC: Ingo Molnar <[email protected]>
> ---
> include/asm-generic/bug.h | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index 4b67559..e952242 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -89,11 +89,11 @@ 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
> -#define BUG_ON(condition) do { if (condition) ; } while(0)
> +#define BUG_ON(condition) do { if (condition) unreachable(); } while (0)
> #endif
>
> #ifndef HAVE_ARCH_WARN_ON
> --
This seems wrong to me. Wouldn't you always want to do the endless
loop? In the absence of an arch-specific method to jump to an
exception handler, it isn't really unreachable. On gcc 4.5 this would
essentially become a no-op.
--
Brian Gerst
Brian Gerst wrote:
> On Mon, Sep 14, 2009 at 5:55 PM, David Daney <[email protected]> wrote:
>> The subject says it all (most). The only drawback here is that for a
>> pre-GCC-5.4 compiler, instead of expanding to nothing we now expand
>> BUG() to an endless loop. Before the patch when configured with
>> !CONFIG_BUG() you might get some warnings, but the code would be
>> small. After the patch there are no warnings, but there is an endless
>> loop at each BUG() site.
>>
>> Of course for the GCC-4.5 case we get the best of both worlds.
>>
>> Signed-off-by: David Daney <[email protected]>
>> Suggested-by: Ingo Molnar <[email protected]>
>> CC: Ingo Molnar <[email protected]>
>> ---
>> include/asm-generic/bug.h | 4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
>> index 4b67559..e952242 100644
>> --- a/include/asm-generic/bug.h
>> +++ b/include/asm-generic/bug.h
>> @@ -89,11 +89,11 @@ 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
>> -#define BUG_ON(condition) do { if (condition) ; } while(0)
>> +#define BUG_ON(condition) do { if (condition) unreachable(); } while (0)
>> #endif
>>
>> #ifndef HAVE_ARCH_WARN_ON
>> --
>
> This seems wrong to me. Wouldn't you always want to do the endless
> loop? In the absence of an arch-specific method to jump to an
> exception handler, it isn't really unreachable. On gcc 4.5 this would
> essentially become a no-op.
>
Several points:
* When you hit a BUG() you are screwed.
* When you configure with !CONFIG_BUG you are asserting that you don't
want to try to trap on BUG();.
The existing code just falls through to whatever happens to follow the
BUG(). This is not what the programmer intended, but the person that
chose !CONFIG_BUG decided that they would like undefined behavior in
order to save a few bytes of code.
With the patch one of two things will happen:
pre-GCC-4.5) We will now enter an endless loop and not fall through.
This makes the code slightly larger than pre patch.
post-GCC-4.5) We do something totally undefined. It will not
necessarily fall through to the code after the BUG() It could really
end up doing almost anything. On the plus side, we save a couple of
bytes of code and eliminate some compiler warnings.
If you don't like it, don't configure with !CONFIG_BUG. But the patch
doesn't really change the fact that hitting a BUG() with !CONFIG_BUG
leads to undefined behavior. It only makes the case where you don't hit
BUG() nicer.
David Daney
On Mon, 14 Sep 2009, David Daney wrote:
>
> The existing code just falls through to whatever happens to follow the BUG().
Brian was talking BUG_ON().
And the existing !CONFIG_BUG BUG_ON() is actually set up so that gcc will
just optimize it away entirely (yet give the same compile-time warnings as
the "real" BUG_ON() does).
Changing it to "if (cond) unreachable()" is likely to generate _more_
code, which is against the whole point of wanting to disable CONFIG_BUG.
Linus
On Mon, Sep 14, 2009 at 7:28 PM, David Daney <[email protected]> wrote:
> Brian Gerst wrote:
>>
>> On Mon, Sep 14, 2009 at 5:55 PM, David Daney <[email protected]>
>> wrote:
>>>
>>> The subject says it all (most). The only drawback here is that for a
>>> pre-GCC-5.4 compiler, instead of expanding to nothing we now expand
>>> BUG() to an endless loop. Before the patch when configured with
>>> !CONFIG_BUG() you might get some warnings, but the code would be
>>> small. After the patch there are no warnings, but there is an endless
>>> loop at each BUG() site.
>>>
>>> Of course for the GCC-4.5 case we get the best of both worlds.
>>>
>>> Signed-off-by: David Daney <[email protected]>
>>> Suggested-by: Ingo Molnar <[email protected]>
>>> CC: Ingo Molnar <[email protected]>
>>> ---
>>> include/asm-generic/bug.h | 4 ++--
>>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
>>> index 4b67559..e952242 100644
>>> --- a/include/asm-generic/bug.h
>>> +++ b/include/asm-generic/bug.h
>>> @@ -89,11 +89,11 @@ 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
>>> -#define BUG_ON(condition) do { if (condition) ; } while(0)
>>> +#define BUG_ON(condition) do { if (condition) unreachable(); } while (0)
>>> #endif
>>>
>>> #ifndef HAVE_ARCH_WARN_ON
>>> --
>>
>> This seems wrong to me. Wouldn't you always want to do the endless
>> loop? In the absence of an arch-specific method to jump to an
>> exception handler, it isn't really unreachable. On gcc 4.5 this would
>> essentially become a no-op.
>>
>
> Several points:
>
> * When you hit a BUG() you are screwed.
>
> * When you configure with !CONFIG_BUG you are asserting that you don't want
> to try to trap on BUG();.
>
> The existing code just falls through to whatever happens to follow the
> BUG(). This is not what the programmer intended, but the person that chose
> !CONFIG_BUG decided that they would like undefined behavior in order to save
> a few bytes of code.
>
> With the patch one of two things will happen:
>
> pre-GCC-4.5) We will now enter an endless loop and not fall through. This
> makes the code slightly larger than pre patch.
>
> post-GCC-4.5) We do something totally undefined. It will not necessarily
> fall through to the code after the BUG() It could really end up doing
> almost anything. On the plus side, we save a couple of bytes of code and
> eliminate some compiler warnings.
>
> If you don't like it, don't configure with !CONFIG_BUG. But the patch
> doesn't really change the fact that hitting a BUG() with !CONFIG_BUG leads
> to undefined behavior. It only makes the case where you don't hit BUG()
> nicer.
>
> David Daney
>
Let me rephrase this. The original BUG() is simply a no-op, not an
infinite loop. GCC will optimize it away (and possibly other dead
code around it). Adding unreachable() makes the code do potentially
unpredictable things. It's not necessary. The same goes for BUG_ON.
In that case the test does get optimized away too, but is still needed
to silence warnings about unused variables, etc.
--
Brian Gerst
Further uses of unreachable().
Signed-off-by: Ralf Baechle <[email protected]>
arch/mips/fw/arc/memory.c | 5 +++--
arch/mips/kernel/signal.c | 5 +++--
arch/mips/kernel/signal32.c | 5 +++--
arch/mips/kernel/signal_n32.c | 3 ++-
4 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/arch/mips/fw/arc/memory.c b/arch/mips/fw/arc/memory.c
index 8b8eea2..d248f64 100644
--- a/arch/mips/fw/arc/memory.c
+++ b/arch/mips/fw/arc/memory.c
@@ -11,6 +11,7 @@
* because on some machines like SGI IP27 the ARC memory configuration data
* completly bogus and alternate easier to use mechanisms are available.
*/
+#include <linux/compiler.h>
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/types.h>
@@ -80,7 +81,7 @@ static inline int memtype_classify_arcs(union linux_memtypes type)
default:
BUG();
}
- while(1); /* Nuke warning. */
+ unreachable(); /* Nuke warning. */
}
static inline int memtype_classify_arc(union linux_memtypes type)
@@ -100,7 +101,7 @@ static inline int memtype_classify_arc(union linux_memtypes type)
default:
BUG();
}
- while(1); /* Nuke warning. */
+ unreachable(); /* Nuke warning. */
}
static int __init prom_memtype_classify(union linux_memtypes type)
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index 6254041..eefd278 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -8,6 +8,7 @@
* Copyright (C) 1999, 2000 Silicon Graphics, Inc.
*/
#include <linux/cache.h>
+#include <linux/compiler.h>
#include <linux/sched.h>
#include <linux/mm.h>
#include <linux/personality.h>
@@ -422,7 +423,7 @@ asmlinkage void sys_sigreturn(nabi_no_regargs struct pt_regs regs)
"j\tsyscall_exit"
:/* no outputs */
:"r" (®s));
- /* Unreached */
+ unreachable();
badframe:
force_sig(SIGSEGV, current);
@@ -468,7 +469,7 @@ asmlinkage void sys_rt_sigreturn(nabi_no_regargs struct pt_regs regs)
"j\tsyscall_exit"
:/* no outputs */
:"r" (®s));
- /* Unreached */
+ unreachable();
badframe:
force_sig(SIGSEGV, current);
diff --git a/arch/mips/kernel/signal32.c b/arch/mips/kernel/signal32.c
index 2e74075..d41e267 100644
--- a/arch/mips/kernel/signal32.c
+++ b/arch/mips/kernel/signal32.c
@@ -9,6 +9,7 @@
*/
#include <linux/cache.h>
#include <linux/compat.h>
+#include <linux/compiler.h>
#include <linux/sched.h>
#include <linux/mm.h>
#include <linux/smp.h>
@@ -526,7 +527,7 @@ asmlinkage void sys32_sigreturn(nabi_no_regargs struct pt_regs regs)
"j\tsyscall_exit"
:/* no outputs */
:"r" (®s));
- /* Unreached */
+ unreachable();
badframe:
force_sig(SIGSEGV, current);
@@ -583,7 +584,7 @@ asmlinkage void sys32_rt_sigreturn(nabi_no_regargs struct pt_regs regs)
"j\tsyscall_exit"
:/* no outputs */
:"r" (®s));
- /* Unreached */
+ unreachable();
badframe:
force_sig(SIGSEGV, current);
diff --git a/arch/mips/kernel/signal_n32.c b/arch/mips/kernel/signal_n32.c
index bb277e8..24ebaa5 100644
--- a/arch/mips/kernel/signal_n32.c
+++ b/arch/mips/kernel/signal_n32.c
@@ -16,6 +16,7 @@
* Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
*/
#include <linux/cache.h>
+#include <linux/compiler.h>
#include <linux/sched.h>
#include <linux/mm.h>
#include <linux/smp.h>
@@ -167,7 +168,7 @@ asmlinkage void sysn32_rt_sigreturn(nabi_no_regargs struct pt_regs regs)
"j\tsyscall_exit"
:/* no outputs */
:"r" (®s));
- /* Unreached */
+ unreachable();
badframe:
force_sig(SIGSEGV, current);
Linus Torvalds wrote:
>
> On Mon, 14 Sep 2009, David Daney wrote:
>> The existing code just falls through to whatever happens to follow the BUG().
>
> Brian was talking BUG_ON().
>
> And the existing !CONFIG_BUG BUG_ON() is actually set up so that gcc will
> just optimize it away entirely (yet give the same compile-time warnings as
> the "real" BUG_ON() does).
>
> Changing it to "if (cond) unreachable()" is likely to generate _more_
> code, which is against the whole point of wanting to disable CONFIG_BUG.
>
Yes, you are correct. I said the same thing in the log message for the
patch.
Really it may be too early for this patch to be appropriate for your
tree. GCC-4.5 will probably not be released for several more months,
and it will be several years before a GCC with __builtin_unreachable()
is being used by the majority of people compiling kernels.
Ingo had suggested the approach of this patch as a way of eliminating
many warnings when using !CONFIG_BUG. I think it clearly makes sense
for compilers that support __builtin_unreachable(), but clearly it is
not an unquestionable win if we end up generating larger code.
With this particular patch, I don't really care if you merge it or not.
Perhaps I shouldn't have made it part of the set.
The rest of the set I think would make sense for 2.6.32 or 2.6.33.
David Daney
Brian Gerst wrote:
> On Mon, Sep 14, 2009 at 7:28 PM, David Daney <[email protected]> wrote:
>> Brian Gerst wrote:
>>> On Mon, Sep 14, 2009 at 5:55 PM, David Daney <[email protected]>
>>> wrote:
>>>> The subject says it all (most). The only drawback here is that for a
>>>> pre-GCC-5.4 compiler, instead of expanding to nothing we now expand
>>>> BUG() to an endless loop. Before the patch when configured with
>>>> !CONFIG_BUG() you might get some warnings, but the code would be
>>>> small. After the patch there are no warnings, but there is an endless
>>>> loop at each BUG() site.
>>>>
>>>> Of course for the GCC-4.5 case we get the best of both worlds.
>>>>
>>>> Signed-off-by: David Daney <[email protected]>
>>>> Suggested-by: Ingo Molnar <[email protected]>
>>>> CC: Ingo Molnar <[email protected]>
>>>> ---
>>>> include/asm-generic/bug.h | 4 ++--
>>>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
>>>> index 4b67559..e952242 100644
>>>> --- a/include/asm-generic/bug.h
>>>> +++ b/include/asm-generic/bug.h
>>>> @@ -89,11 +89,11 @@ 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
>>>> -#define BUG_ON(condition) do { if (condition) ; } while(0)
>>>> +#define BUG_ON(condition) do { if (condition) unreachable(); } while (0)
>>>> #endif
>>>>
>>>> #ifndef HAVE_ARCH_WARN_ON
>>>> --
>>> This seems wrong to me. Wouldn't you always want to do the endless
>>> loop? In the absence of an arch-specific method to jump to an
>>> exception handler, it isn't really unreachable. On gcc 4.5 this would
>>> essentially become a no-op.
>>>
>> Several points:
>>
>> * When you hit a BUG() you are screwed.
>>
>> * When you configure with !CONFIG_BUG you are asserting that you don't want
>> to try to trap on BUG();.
>>
>> The existing code just falls through to whatever happens to follow the
>> BUG(). This is not what the programmer intended, but the person that chose
>> !CONFIG_BUG decided that they would like undefined behavior in order to save
>> a few bytes of code.
>>
>> With the patch one of two things will happen:
>>
>> pre-GCC-4.5) We will now enter an endless loop and not fall through. This
>> makes the code slightly larger than pre patch.
>>
>> post-GCC-4.5) We do something totally undefined. It will not necessarily
>> fall through to the code after the BUG() It could really end up doing
>> almost anything. On the plus side, we save a couple of bytes of code and
>> eliminate some compiler warnings.
>>
>> If you don't like it, don't configure with !CONFIG_BUG. But the patch
>> doesn't really change the fact that hitting a BUG() with !CONFIG_BUG leads
>> to undefined behavior. It only makes the case where you don't hit BUG()
>> nicer.
>>
>> David Daney
>>
>
> Let me rephrase this. The original BUG() is simply a no-op, not an
> infinite loop. GCC will optimize it away (and possibly other dead
> code around it). Adding unreachable() makes the code do potentially
> unpredictable things.
The code already does unpredictable things (also known as undefined
behavior) without the patch. Consider this code:
enum values {GOOD, BAD, RUN_NORMALLY};
int foo(int a)
{
if (a = GOOD)
return RUN_NORMALLY;
BUG();
}
void bar(void)
{
if (foo(BAD) == RUN_NORMALLY)
do_something_useful();
else
irreversibly_damage_hardware();
}
Q: What does this do with CONFIG_BUG?
A: It traps in BUG().
Q: What does this do with !CONFIG_BUG?
A: The compiler issues a warning about reaching the end of a non-void
function. At runtime we don't know what happens.
With my patch the answer to the second question changes to:
A: No compiler warnings are issued. Depending on compiler version code
may be larger. Runtime behavior depends on compiler version (either an
endless loop in BUG, or undefined).
Since the behavior of the program when configured !CONFIG_BUG is
undefined for cases that would trap had CONFIG_BUG be selected, the only
tangible differences pre and post patch are:
GCC-4.4: No warnings, slightly larger code.
GCC-4.5: No warnings, code should not be any larger.
> It's not necessary.
Many patches are 'not necessary', the question should be: are they
desirable.
> The same goes for BUG_ON.
> In that case the test does get optimized away too, but is still needed
> to silence warnings about unused variables, etc.
For the GCC-4.5 case, the patch is even better. Not only does the
evaluation of the condition get optimized away, the compiler knows the
condition is false in the code following the the BUG() and can propagate
that knowledge into optimizations on the following code.
David Daney