Tim Deegan
2013-Sep-19 14:39 UTC
[PATCH] x86: mark BUG()s and assertion failures as terminal.
This helps avoid static analysis false-positives, and might lead to
better code density as the compiler knows it doesn''t have to restore
spilled state &c.
Signed-off-by: Tim Deegan <tim@xen.org>
---
xen/include/asm-x86/bug.h | 11 ++++++++---
xen/include/xen/compiler.h | 6 ++++++
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/xen/include/asm-x86/bug.h b/xen/include/asm-x86/bug.h
index e5dd559..956bfd2 100644
--- a/xen/include/asm-x86/bug.h
+++ b/xen/include/asm-x86/bug.h
@@ -46,12 +46,17 @@ struct bug_frame {
#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
-#define BUG() BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL)
+#define BUG() do { \
+ BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL); \
+ unreachable(); \
+} while (0)
#define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL)
-#define assert_failed(msg) \
- BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg)
+#define assert_failed(msg) do { \
+ BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \
+ unreachable(); \
+} while (0)
extern const struct bug_frame __start_bug_frames[],
__stop_bug_frames_0[],
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 7009a09..7d6805c 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -14,6 +14,12 @@
#define always_inline __inline__ __attribute__ ((always_inline))
#define noinline __attribute__((noinline))
+#if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__
< 5))
+#define unreachable() do {} while (1)
+#else
+#define unreachable() __builtin_unreachable()
+#endif
+
#ifdef __clang__
/* Clang can replace some vars with new automatic ones that go in .data;
* mark all explicit-segment vars ''used'' to prevent that. */
--
1.7.10.4
Keir Fraser
2013-Sep-19 14:56 UTC
Re: [PATCH] x86: mark BUG()s and assertion failures as terminal.
On 19/09/2013 15:39, "Tim Deegan" <tim@xen.org> wrote:> This helps avoid static analysis false-positives, and might lead to > better code density as the compiler knows it doesn''t have to restore > spilled state &c. > > Signed-off-by: Tim Deegan <tim@xen.org> > --- > xen/include/asm-x86/bug.h | 11 ++++++++--- > xen/include/xen/compiler.h | 6 ++++++ > 2 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/xen/include/asm-x86/bug.h b/xen/include/asm-x86/bug.h > index e5dd559..956bfd2 100644 > --- a/xen/include/asm-x86/bug.h > +++ b/xen/include/asm-x86/bug.h > @@ -46,12 +46,17 @@ struct bug_frame { > > > #define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL) > -#define BUG() BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL) > +#define BUG() do { \ > + BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, NULL); \ > + unreachable(); \ > +} while (0) > > #define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, > NULL) > > -#define assert_failed(msg) \ > - BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg) > +#define assert_failed(msg) do { \ > + BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \ > + unreachable(); \ > +} while (0) > > extern const struct bug_frame __start_bug_frames[], > __stop_bug_frames_0[], > diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h > index 7009a09..7d6805c 100644 > --- a/xen/include/xen/compiler.h > +++ b/xen/include/xen/compiler.h > @@ -14,6 +14,12 @@ > #define always_inline __inline__ __attribute__ ((always_inline)) > #define noinline __attribute__((noinline)) > > +#if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5))Do you mean for gcc-3.4 to use __builtin_unreachable()? This might be clearer, correcter, and better match the prevailing compiler.h style, if it was switched round to handle the __builtin_unreachable() case first. -- Keir> +#define unreachable() do {} while (1) > +#else > +#define unreachable() __builtin_unreachable() > +#endif > + > #ifdef __clang__ > /* Clang can replace some vars with new automatic ones that go in .data; > * mark all explicit-segment vars ''used'' to prevent that. */
Tim Deegan
2013-Sep-19 15:07 UTC
Re: [PATCH] x86: mark BUG()s and assertion failures as terminal.
At 15:56 +0100 on 19 Sep (1379606210), Keir Fraser wrote:> On 19/09/2013 15:39, "Tim Deegan" <tim@xen.org> wrote: > > @@ -14,6 +14,12 @@ > > #define always_inline __inline__ __attribute__ ((always_inline)) > > #define noinline __attribute__((noinline)) > > > > +#if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5)) > > Do you mean for gcc-3.4 to use __builtin_unreachable()?No, what I want is for clang and all GCCs >= 4.5 to use the builtin.> This might be > clearer, correcter, and better match the prevailing compiler.h style, if it > was switched round to handle the __builtin_unreachable() case first.Switched around, it looks like this: #if (defined(__clang__) || ((__GNUC__ == 4) && (__GNUC_MINOR__ >= 5)) || (__GNUC__ > 4)) #define unreachable() __builtin_unreachable() #else #define unreachable() do {} while (1) #endif Not sure that''s any clearer or correcter, really. Tim.
Tim Deegan
2013-Sep-19 15:10 UTC
Re: [PATCH] x86: mark BUG()s and assertion failures as terminal.
At 16:07 +0100 on 19 Sep (1379606849), Tim Deegan wrote:> At 15:56 +0100 on 19 Sep (1379606210), Keir Fraser wrote: > > On 19/09/2013 15:39, "Tim Deegan" <tim@xen.org> wrote: > > > @@ -14,6 +14,12 @@ > > > #define always_inline __inline__ __attribute__ ((always_inline)) > > > #define noinline __attribute__((noinline)) > > > > > > +#if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5)) > > > > Do you mean for gcc-3.4 to use __builtin_unreachable()? > > No, what I want is for clang and all GCCs >= 4.5 to use the builtin.Oh wait, I understand your question now: gcc 3.4 will already have failed the version check at the top of the file, so I don''t need to check for it here. Tim.
Keir Fraser
2013-Sep-19 15:13 UTC
Re: [PATCH] x86: mark BUG()s and assertion failures as terminal.
On 19/09/2013 16:07, "Tim Deegan" <tim@xen.org> wrote:>>> +#if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5)) >> >> Do you mean for gcc-3.4 to use __builtin_unreachable()? > > No, what I want is for clang and all GCCs >= 4.5 to use the builtin.Then this original #if doesn''t correctly handle __GNUC__ < 4.>> This might be >> clearer, correcter, and better match the prevailing compiler.h style, if it >> was switched round to handle the __builtin_unreachable() case first. > > Switched around, it looks like this: > > #if (defined(__clang__) || ((__GNUC__ == 4) && (__GNUC_MINOR__ >= 5)) || > (__GNUC__ > 4)) > #define unreachable() __builtin_unreachable() > #else > #define unreachable() do {} while (1) > #endif > > Not sure that''s any clearer or correcter, really.Well the GCC version check is correct in this one. Looks good to me. Replace this hunk in the original patch and you can have my ack: Acked-by: Keir Fraser <keir@xen.org> -- Keir
Keir Fraser
2013-Sep-19 15:17 UTC
Re: [PATCH] x86: mark BUG()s and assertion failures as terminal.
On 19/09/2013 16:10, "Tim Deegan" <tim@xen.org> wrote:> At 16:07 +0100 on 19 Sep (1379606849), Tim Deegan wrote: >> At 15:56 +0100 on 19 Sep (1379606210), Keir Fraser wrote: >>> On 19/09/2013 15:39, "Tim Deegan" <tim@xen.org> wrote: >>>> @@ -14,6 +14,12 @@ >>>> #define always_inline __inline__ __attribute__ ((always_inline)) >>>> #define noinline __attribute__((noinline)) >>>> >>>> +#if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5)) >>> >>> Do you mean for gcc-3.4 to use __builtin_unreachable()? >> >> No, what I want is for clang and all GCCs >= 4.5 to use the builtin. > > Oh wait, I understand your question now: gcc 3.4 will already have > failed the version check at the top of the file, so I don''t need to > check for it here.Argh, I was looking at a 4.1 branch. :) Then my Ack applies to your original patch as is! -- Keir> Tim.
Andrew Cooper
2013-Sep-19 15:44 UTC
Re: [PATCH] x86: mark BUG()s and assertion failures as terminal.
On 19/09/2013 15:39, Tim Deegan wrote:> This helps avoid static analysis false-positives, and might lead to > better code density as the compiler knows it doesn''t have to restore > spilled state &c. > > Signed-off-by: Tim Deegan <tim@xen.org> >Out of interest, I tried looking at some numbers for this. Using gcc 4.7.2 (Debian Wheezy), .text decreased by 656 bytes and .init.text decreased by 71 bytes. While those are expected, .rodata decreased by 1224 bytes. I am at a loss to explain the decrease in .rodata, but did double check my compiling, and find the decrease still present. Either way, it looks to be a useful improvement. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tim Deegan
2013-Sep-19 16:52 UTC
Re: [PATCH] x86: mark BUG()s and assertion failures as terminal.
At 16:44 +0100 on 19 Sep (1379609065), Andrew Cooper wrote:> On 19/09/2013 15:39, Tim Deegan wrote: > > This helps avoid static analysis false-positives, and might lead to > > better code density as the compiler knows it doesn''t have to restore > > spilled state &c. > > > > Signed-off-by: Tim Deegan <tim@xen.org> > > > > Out of interest, I tried looking at some numbers for this. > > Using gcc 4.7.2 (Debian Wheezy), .text decreased by 656 bytes and > .init.text decreased by 71 bytes. While those are expected, .rodata > decreased by 1224 bytes. I am at a loss to explain the decrease in > .rodata, but did double check my compiling, and find the decrease still > present..rodata contains the bug frames themselves -- it looks like 150 ASSERT()s are now unreachable. :) I''m sure that includes a lot of duplication from ASSERT()s in header files. The remaining handful of bytes seem to be from GCC inlining one or two things it hadn''t before, which makes the embedded copy of the symbol table a little smaller. Tim.