[PATCH] x86: mark BUG()s and assertion failures as terminal.

[PATCH] x86: mark BUG()s and assertion failures as terminal.

[Tim Deegan]

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

Re: [PATCH] x86: mark BUG()s and assertion failures as terminal.

[Keir Fraser]

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. */

Re: [PATCH] x86: mark BUG()s and assertion failures as terminal.

[Tim Deegan]

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.

Re: [PATCH] x86: mark BUG()s and assertion failures as terminal.

[Tim Deegan]

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.

Re: [PATCH] x86: mark BUG()s and assertion failures as terminal.

[Keir Fraser]

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

Re: [PATCH] x86: mark BUG()s and assertion failures as terminal.

[Keir Fraser]

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.

Re: [PATCH] x86: mark BUG()s and assertion failures as terminal.

[Andrew Cooper]

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>

Re: [PATCH] x86: mark BUG()s and assertion failures as terminal.

[Tim Deegan]

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.