[Xen-devel] [PATCH v3 0/2] xen: fix CONFIG_DEBUG_LOCKS

[Xen-devel] [PATCH v3 0/2] xen: fix CONFIG_DEBUG_LOCKS

[Juergen Gross]

CONFIG_DEBUG_LOCKS is using ASSERT() for catching issues making it
depend on CONFIG_DEBUG.

This series fixes that by using BUG_ON() instead. In order not to lose
the rather nice debugging information which condition was hit add a
config option to include a message similar to the one ASSERT() is
printing in case of BUG_ON() triggering.

Juergen Gross (2):
  xen: add config option to include failing condition in BUG_ON()
    message
  xen: make CONFIG_DEBUG_LOCKS usable without CONFIG_DEBUG

 xen/Kconfig.debug         | 8 +++++++-
 xen/common/spinlock.c     | 2 +-
 xen/include/asm-arm/bug.h | 6 ++++--
 xen/include/asm-x86/bug.h | 5 +++--
 xen/include/xen/lib.h     | 4 ++++
 5 files changed, 19 insertions(+), 6 deletions(-)

-- 
2.16.4


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

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

[Juergen Gross]

Today a triggering BUG_ON() will only print source file and line
information. Add the possibility to print the triggering condition like
ASSERT().

Do that by introducing BUG_VERBOSE() and add a Kconfig option to make
BUG_ON use BUG_VERBOSE().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
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.
 
-
 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);       \
     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)
-- 
2.16.4


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

[Xen-devel] [PATCH v3 2/2] xen: make CONFIG_DEBUG_LOCKS usable without CONFIG_DEBUG

[Juergen Gross]

In expert mode it is possible to enable CONFIG_DEBUG_LOCKS without
having enabled CONFIG_DEBUG. The coding is depending on CONFIG_DEBUG
as it is using ASSERT(), however.

Fix that by using BUG_ON() instead of ASSERT() in rel_lock().

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/common/spinlock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 286f916bca..344981c54a 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -86,7 +86,7 @@ static void got_lock(union lock_debug *debug)
 static void rel_lock(union lock_debug *debug)
 {
     if ( atomic_read(&spin_debug) > 0 )
-        ASSERT(debug->cpu == smp_processor_id());
+        BUG_ON(debug->cpu != smp_processor_id());
     debug->cpu = SPINLOCK_NO_CPU;
 }
 
-- 
2.16.4


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

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

[Jan Beulich]

On 21.01.2020 11: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().
> 
> Do that by introducing BUG_VERBOSE() and add a Kconfig option to make
> BUG_ON use BUG_VERBOSE().
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

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

[Julien Grall]

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@suse.com>
> ---
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jürgen Groß]

On 21.01.20 12:38, Julien Grall wrote:
> 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?

Hmm, why not.

Any objections?

> 
>>
>> Do that by introducing BUG_VERBOSE() and add a Kconfig option to make
>> BUG_ON use BUG_VERBOSE().
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> 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.

In fact this is the result of moving the new option in V3.

I'll mention the change in the commit message.

> 
>>   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?

You are right, of course.


Juergen

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