Re: [Xen-devel] [PATCH v3 1/2] xen: add config option to include failing condition in BUG_ON() message

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 21/01/2020 10:13, Juergen Gross wrote:
> Today a triggering BUG_ON() will only print source file and line
> information. Add the possibility to print the triggering condition like
> ASSERT().
Any reason to only limit the change for BUG_ON? How about WARN_ON?

> Do that by introducing BUG_VERBOSE() and add a Kconfig option to make
> BUG_ON use BUG_VERBOSE().
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> V3:
> - move kconfig option out of DEBUG || expert section (Jan Beulich)
> - fix ARM build (Jan Beulich)
> - eliminate BUG_ON_VERBOSE() (Jan Beulich)
> ---
>   xen/Kconfig.debug         | 8 +++++++-
>   xen/include/asm-arm/bug.h | 6 ++++--
>   xen/include/asm-x86/bug.h | 5 +++--
>   xen/include/xen/lib.h     | 4 ++++
>   4 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
> index b3511e81a2..75c855e4ae 100644
> --- a/xen/Kconfig.debug
> +++ b/xen/Kconfig.debug
> @@ -11,6 +11,13 @@ config DEBUG
>   
>   	  You probably want to say 'N' here.
>   
> +config DEBUG_BUGVERBOSE
> +       bool "Verbose BUG_ON messages"
> +       default DEBUG
> +       ---help---
> +         In case a BUG_ON triggers additionally print the triggering
> +         condition on the console.
> +
>   if DEBUG || EXPERT = "y"
>   
>   config CRASH_DEBUG
> @@ -81,7 +88,6 @@ config PERF_ARRAYS
>         ---help---
>           Enables software performance counter array histograms.
>   
> -
While I agree this should be dropped this is a spurious line, this feels 
a bit out of context. So I would suggest to mention it in the commit 
message or split it in a separate patch.
>   config VERBOSE_DEBUG
>         bool "Verbose debug messages"
>         default DEBUG
> diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h
> index 36c803357c..90b231e77a 100644
> --- a/xen/include/asm-arm/bug.h
> +++ b/xen/include/asm-arm/bug.h
> @@ -60,11 +60,13 @@ struct bug_frame {
>   
>   #define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "")
>   
> -#define BUG() do {                                              \
> -    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, "");        \
> +#define BUG_VERBOSE(msg) do {                                   \
> +    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, msg);       \
I am afraid this is not going to be enough to make it work on Arm. You 
also need to update do_bug_frame() to print the string.
I would actually expect a similar change required on the x86 side. Do 
you mind explaining how this works?
>       unreachable();                                              \
>   } while (0)
>   
> +#define BUG() BUG_VERBOSE("")
> +
>   #define assert_failed(msg) do {                                 \
>       BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg);     \
>       unreachable();                                              \
> diff --git a/xen/include/asm-x86/bug.h b/xen/include/asm-x86/bug.h
> index 9bb4a19420..46d282777f 100644
> --- a/xen/include/asm-x86/bug.h
> +++ b/xen/include/asm-x86/bug.h
> @@ -60,10 +60,11 @@ struct bug_frame {
>   
>   
>   #define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, NULL)
> -#define BUG() do {                                              \
> -    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, NULL);      \
> +#define BUG_VERBOSE(msg) do {                                   \
> +    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, msg);       \
>       unreachable();                                              \
>   } while (0)
> +#define BUG() BUG_VERBOSE(NULL)
>   
>   #define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL)
>   
> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index 5d718bbdba..543b75952a 100644
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -22,7 +22,11 @@
>   #include <xen/string.h>
>   #include <asm/bug.h>
>   
> +#ifdef CONFIG_DEBUG_BUGVERBOSE
> +#define BUG_ON(p)  do { if (unlikely(p)) BUG_VERBOSE(#p);  } while (0)
> +#else
>   #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
> +#endif
>   #define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
>   
>   #if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel