[PATCH 0 of 3] Introduce more debugging flexibility with ASSERT() macros

[PATCH 0 of 3] Introduce more debugging flexibility with ASSERT() macros

[Andrew Cooper]

The following three patches introduce several debugging macros I have
been using for a long time while debugging issues in Xen.

ASSERT_PRINK() is hopefully obvious, and ASSERT_RUN() is useful when
more complicated printing is required.

The final macro ASSERT_RUN_SINGLE() is not fit for upstream yet.  It is
designed to force all other PCPUs into a wait loop in an NMI context, so
the ASSERT()'ing processor can walk data structures without locks, and
without fear that values are changing under its feet.  I will work on
integrating this into the crash code (as it has a similar setup for the
start of the kexec_crash() path), and upstream when I have time.

~Andrew

[PATCH 1 of 3] xen/debug: Allow ASSERT() to be enabled in a non-debug build

[Andrew Cooper]

There are times when debugging that assertions are useful, without all
the other implications of a debug build.

Allow ASSERT() to be independently controlled, but defaults to the same
as $(debug)

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff -r 5fbdbf585f5f -r 2927e18e9a7c Config.mk
--- a/Config.mk
+++ b/Config.mk
@@ -12,6 +12,7 @@ realpath = $(wildcard $(foreach file,$(1
 # A debug build of Xen and tools?
 debug ?= y
 debug_symbols ?= $(debug)
+asserts ?= $(debug)
 
 XEN_COMPILE_ARCH    ?= $(shell uname -m | sed -e s/i.86/x86_32/ \
                          -e s/i86pc/x86_32/ -e s/amd64/x86_64/ -e s/arm.*/arm/)
diff -r 5fbdbf585f5f -r 2927e18e9a7c xen/Rules.mk
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -25,6 +25,10 @@ ifeq ($(perfc_arrays),y)
 perfc := y
 endif
 
+ifeq ($(asserts),y)
+CFLAGS += -DCONFIG_ASSERTS
+endif
+
 # Set ARCH/SUBARCH appropriately.
 override TARGET_SUBARCH  := $(XEN_TARGET_ARCH)
 override TARGET_ARCH     := $(shell echo $(XEN_TARGET_ARCH) | \
diff -r 5fbdbf585f5f -r 2927e18e9a7c xen/include/xen/lib.h
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -38,7 +38,7 @@ do {                                    
 } while (0)
 #endif
 
-#ifndef NDEBUG
+#ifdef CONFIG_ASSERTS
 #define ASSERT(p) \
     do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
 #else

[PATCH 2 of 3] xen/debug: Introduce ASSERT_PRINTK()

[Andrew Cooper]

This is a variant of ASSERT() which takes a predicate, and a variable
number of arguments which get fed to prink() before the BUG().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

--
This does use C99 varadic macros, but given that we use other C99
features without #ifdef guards, I felt it not necessary to guard this as
well.

diff -r 2927e18e9a7c -r 477ccdb9870e xen/include/xen/lib.h
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -38,11 +38,26 @@ do {                                    
 } while (0)
 #endif
 
+#ifndef assert_printk_failed
+#define assert_printk_failed(p, ...)                            \
+do {                                                            \
+    printk("Assertion '%s' failed, line %d, file %s\n", p ,     \
+                   __LINE__, __FILE__);                         \
+    printk(__VA_ARGS__);                                        \
+    BUG();                                                      \
+} while (0)
+#endif /* assert_printk_failed */
+
 #ifdef CONFIG_ASSERTS
 #define ASSERT(p) \
     do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
+
+#define ASSERT_PRINTK(p, ...)                                   \
+    do { if ( unlikely(!(p)) )                                  \
+            assert_printk_failed(#p, __VA_ARGS__); } while (0)
 #else
 #define ASSERT(p) do { if ( 0 && (p) ); } while (0)
+#define ASSERT_PRINTK(p, ...) do { if ( 0 && (p) ); } while (0)
 #endif
 
 #define ABS(_x) ({                              \

[PATCH 3 of 3] xen/debug: Introduce ASSERT_RUN()

[Andrew Cooper]

This is a variant of ASSERT() which takes a predicate, a function an
argument for the function.  It is designed for debugging in situations
where ASSERT_PRINTK() is perhaps not powerful enough.

It will run the given function with the given argument before the BUG()
which kills Xen.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff -r 477ccdb9870e -r 0a1a3f35f56a xen/include/xen/lib.h
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -48,6 +48,16 @@ do {                                    
 } while (0)
 #endif /* assert_printk_failed */
 
+#ifndef assert_run_failed
+#define assert_run_failed(p, func, arg)                         \
+do {                                                            \
+    printk("Assertion '%s' failed, line %d, file %s\n", p ,     \
+                   __LINE__, __FILE__);                         \
+    (func)((arg));                                              \
+    BUG();                                                      \
+} while (0)
+#endif /* assert_run_failed */
+
 #ifdef CONFIG_ASSERTS
 #define ASSERT(p) \
     do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
@@ -55,9 +65,15 @@ do {                                    
 #define ASSERT_PRINTK(p, ...)                                   \
     do { if ( unlikely(!(p)) )                                  \
             assert_printk_failed(#p, __VA_ARGS__); } while (0)
+
+/* func expected to be void, taking a single void * argument */
+#define ASSERT_RUN(p, func, arg)                                \
+    do { if ( unlikely(!(p)) )                                  \
+            assert_run_failed(#p, func, arg); } while (0)
 #else
 #define ASSERT(p) do { if ( 0 && (p) ); } while (0)
 #define ASSERT_PRINTK(p, ...) do { if ( 0 && (p) ); } while (0)
+#define ASSERT_RUN(p, func, arg) do { if ( 0 && (p) ); } while (0)
 #endif
 
 #define ABS(_x) ({                              \

Re: [PATCH 0 of 3] Introduce more debugging flexibility with ASSERT() macros

[Keir Fraser]

On 08/10/2012 19:16, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

> The following three patches introduce several debugging macros I have
> been using for a long time while debugging issues in Xen.
> 
> ASSERT_PRINK() is hopefully obvious, and ASSERT_RUN() is useful when
> more complicated printing is required.

Are these going to get enough use to be worthwhile, rather than open-coding
them where necessary? In many places we may not care about being able to
disable the check-and-crash, so avoiding ifdefs is not necessarily a good
argument.

 -- Keir

> The final macro ASSERT_RUN_SINGLE() is not fit for upstream yet.  It is
> designed to force all other PCPUs into a wait loop in an NMI context, so
> the ASSERT()'ing processor can walk data structures without locks, and
> without fear that values are changing under its feet.  I will work on
> integrating this into the crash code (as it has a similar setup for the
> start of the kexec_crash() path), and upstream when I have time.
> 
> ~Andrew

Re: [PATCH 0 of 3] Introduce more debugging flexibility with ASSERT() macros

[Andrew Cooper]

On 08/10/12 19:31, Keir Fraser wrote:
> On 08/10/2012 19:16, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:
>
>> The following three patches introduce several debugging macros I have
>> been using for a long time while debugging issues in Xen.
>>
>> ASSERT_PRINK() is hopefully obvious, and ASSERT_RUN() is useful when
>> more complicated printing is required.
> Are these going to get enough use to be worthwhile, rather than open-coding
> them where necessary? In many places we may not care about being able to
> disable the check-and-crash, so avoiding ifdefs is not necessarily a good
> argument.
>
>  -- Keir

I hope that ASSERT_PRINTK() will start seeing quite a lot of use,
especially with compound conditional statements where is is impossible
from crash to see which of the individual conditionals failed.

ASSERT_RUN() perhaps not so, but having it available means one less
thing to opencode when debugging.


I had not considered the case for {WARN,BUG}_PRINTK() which have the
same argument as ASSERT_PRINTK().  Would you like to see them introduced
as well?

~Andrew

>
>> The final macro ASSERT_RUN_SINGLE() is not fit for upstream yet.  It is
>> designed to force all other PCPUs into a wait loop in an NMI context, so
>> the ASSERT()'ing processor can walk data structures without locks, and
>> without fear that values are changing under its feet.  I will work on
>> integrating this into the crash code (as it has a similar setup for the
>> start of the kexec_crash() path), and upstream when I have time.
>>
>> ~Andrew
>

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Re: [PATCH 2 of 3] xen/debug: Introduce ASSERT_PRINTK()

[Jan Beulich]

>>> On 08.10.12 at 20:16, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> This is a variant of ASSERT() which takes a predicate, and a variable
> number of arguments which get fed to prink() before the BUG().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> --
> This does use C99 varadic macros, but given that we use other C99
> features without #ifdef guards, I felt it not necessary to guard this as
> well.
> 
> diff -r 2927e18e9a7c -r 477ccdb9870e xen/include/xen/lib.h
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -38,11 +38,26 @@ do {                                    
>  } while (0)
>  #endif
>  
> +#ifndef assert_printk_failed
> +#define assert_printk_failed(p, ...)                            \
> +do {                                                            \
> +    printk("Assertion '%s' failed, line %d, file %s\n", p ,     \
> +                   __LINE__, __FILE__);                         \
> +    printk(__VA_ARGS__);                                        \

The first argument here necessarily is a format string, so it
should also be enforced that way. Which then opens the
question whether the two printk()-s shouldn't be folded (at the
price of requiring the format string to be a literal).

I wonder though whether we wouldn't be better off following
Linux'es WARN() et al infrastructure, rather than extending the
ASSERT() one.

Jan

> +    BUG();                                                      \
> +} while (0)
> +#endif /* assert_printk_failed */
> +
>  #ifdef CONFIG_ASSERTS
>  #define ASSERT(p) \
>      do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
> +
> +#define ASSERT_PRINTK(p, ...)                                   \
> +    do { if ( unlikely(!(p)) )                                  \
> +            assert_printk_failed(#p, __VA_ARGS__); } while (0)
>  #else
>  #define ASSERT(p) do { if ( 0 && (p) ); } while (0)
> +#define ASSERT_PRINTK(p, ...) do { if ( 0 && (p) ); } while (0)
>  #endif
>  
>  #define ABS(_x) ({                              \

Re: [PATCH 3 of 3] xen/debug: Introduce ASSERT_RUN()

[Jan Beulich]

>>> On 08.10.12 at 20:16, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> This is a variant of ASSERT() which takes a predicate, a function an
> argument for the function.  It is designed for debugging in situations
> where ASSERT_PRINTK() is perhaps not powerful enough.
> 
> It will run the given function with the given argument before the BUG()
> which kills Xen.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> diff -r 477ccdb9870e -r 0a1a3f35f56a xen/include/xen/lib.h
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -48,6 +48,16 @@ do {                                    
>  } while (0)
>  #endif /* assert_printk_failed */
>  
> +#ifndef assert_run_failed
> +#define assert_run_failed(p, func, arg)                         \
> +do {                                                            \
> +    printk("Assertion '%s' failed, line %d, file %s\n", p ,     \
> +                   __LINE__, __FILE__);                         \
> +    (func)((arg));                                              \

Quite a few pointless parentheses here.

> +    BUG();                                                      \
> +} while (0)
> +#endif /* assert_run_failed */
> +
>  #ifdef CONFIG_ASSERTS
>  #define ASSERT(p) \
>      do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
> @@ -55,9 +65,15 @@ do {                                    
>  #define ASSERT_PRINTK(p, ...)                                   \
>      do { if ( unlikely(!(p)) )                                  \
>              assert_printk_failed(#p, __VA_ARGS__); } while (0)
> +
> +/* func expected to be void, taking a single void * argument */
> +#define ASSERT_RUN(p, func, arg)                                \

Since the user of the construct specifies both func and arg, I don't
see the need to specify what type they are. Nor is it meaningful
here whether the function returns void (it would at most need to
be stated - I'd consider this mostly obvious - that an eventual
return value doesn't get used).

> +    do { if ( unlikely(!(p)) )                                  \
> +            assert_run_failed(#p, func, arg); } while (0)
>  #else
>  #define ASSERT(p) do { if ( 0 && (p) ); } while (0)
>  #define ASSERT_PRINTK(p, ...) do { if ( 0 && (p) ); } while (0)
> +#define ASSERT_RUN(p, func, arg) do { if ( 0 && (p) ); } while (0)

You shall evaluate func and arg (i.e. invoke func(arg)) in the
(dead) if() body, to both avoid the need for #ifdef-s at use
sites and check type compatibility even in non-debug (or non-
assert, following your earlier patch) builds.

Jan

>  #endif
>  
>  #define ABS(_x) ({                              \

Re: [PATCH 2 of 3] xen/debug: Introduce ASSERT_PRINTK()

[Andrew Cooper]

On 15/10/12 10:17, Jan Beulich wrote:
>>>> On 08.10.12 at 20:16, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> This is a variant of ASSERT() which takes a predicate, and a variable
>> number of arguments which get fed to prink() before the BUG().
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> --
>> This does use C99 varadic macros, but given that we use other C99
>> features without #ifdef guards, I felt it not necessary to guard this as
>> well.
>>
>> diff -r 2927e18e9a7c -r 477ccdb9870e xen/include/xen/lib.h
>> --- a/xen/include/xen/lib.h
>> +++ b/xen/include/xen/lib.h
>> @@ -38,11 +38,26 @@ do {                                    
>>  } while (0)
>>  #endif
>>  
>> +#ifndef assert_printk_failed
>> +#define assert_printk_failed(p, ...)                            \
>> +do {                                                            \
>> +    printk("Assertion '%s' failed, line %d, file %s\n", p ,     \
>> +                   __LINE__, __FILE__);                         \
>> +    printk(__VA_ARGS__);                                        \
> The first argument here necessarily is a format string, so it
> should also be enforced that way.

Except for the trailing comma issue present in C99 varadic macros, which
is why it is specified this way.

#define COMMA(fmt, ...) printf(fmt, _VA_ARGS__);

Calling COMMA("foobar") will expand to 'printf("foobar",);' leading to a
syntax error.  There is a GCCism which fixes this issue, but it is not
portable.


>  Which then opens the
> question whether the two printk()-s shouldn't be folded (at the
> price of requiring the format string to be a literal).

I would err away from that option if possible, just for flexibility sake.

>
> I wonder though whether we wouldn't be better off following
> Linux'es WARN() et al infrastructure, rather than extending the
> ASSERT() one.
>
> Jan

Keir implied that this might like to be extended to BUG()s and WARN()s,
which I am happy to do if that is the consensus.

~Andrew

>
>> +    BUG();                                                      \
>> +} while (0)
>> +#endif /* assert_printk_failed */
>> +
>>  #ifdef CONFIG_ASSERTS
>>  #define ASSERT(p) \
>>      do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
>> +
>> +#define ASSERT_PRINTK(p, ...)                                   \
>> +    do { if ( unlikely(!(p)) )                                  \
>> +            assert_printk_failed(#p, __VA_ARGS__); } while (0)
>>  #else
>>  #define ASSERT(p) do { if ( 0 && (p) ); } while (0)
>> +#define ASSERT_PRINTK(p, ...) do { if ( 0 && (p) ); } while (0)
>>  #endif
>>  
>>  #define ABS(_x) ({                              \
>
>

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Re: [PATCH 2 of 3] xen/debug: Introduce ASSERT_PRINTK()

[Jan Beulich]

>>> On 15.10.12 at 11:29, Andrew Cooper <andrew.cooper3@citrix.com> wrote:

> On 15/10/12 10:17, Jan Beulich wrote:
>>>>> On 08.10.12 at 20:16, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> This is a variant of ASSERT() which takes a predicate, and a variable
>>> number of arguments which get fed to prink() before the BUG().
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>
>>> --
>>> This does use C99 varadic macros, but given that we use other C99
>>> features without #ifdef guards, I felt it not necessary to guard this as
>>> well.
>>>
>>> diff -r 2927e18e9a7c -r 477ccdb9870e xen/include/xen/lib.h
>>> --- a/xen/include/xen/lib.h
>>> +++ b/xen/include/xen/lib.h
>>> @@ -38,11 +38,26 @@ do {                                    
>>>  } while (0)
>>>  #endif
>>>  
>>> +#ifndef assert_printk_failed
>>> +#define assert_printk_failed(p, ...)                            \
>>> +do {                                                            \
>>> +    printk("Assertion '%s' failed, line %d, file %s\n", p ,     \
>>> +                   __LINE__, __FILE__);                         \
>>> +    printk(__VA_ARGS__);                                        \
>> The first argument here necessarily is a format string, so it
>> should also be enforced that way.
> 
> Except for the trailing comma issue present in C99 varadic macros, which
> is why it is specified this way.
> 
> #define COMMA(fmt, ...) printf(fmt, _VA_ARGS__);
> 
> Calling COMMA("foobar") will expand to 'printf("foobar",);' leading to a
> syntax error.  There is a GCCism which fixes this issue, but it is not
> portable.

But this is not a public header, so gcc-isms are no problem.

Jan

Re: [PATCH 3 of 3] xen/debug: Introduce ASSERT_RUN()

[Andrew Cooper]

On 15/10/12 10:23, Jan Beulich wrote:
>>>> On 08.10.12 at 20:16, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> This is a variant of ASSERT() which takes a predicate, a function an
>> argument for the function.  It is designed for debugging in situations
>> where ASSERT_PRINTK() is perhaps not powerful enough.
>>
>> It will run the given function with the given argument before the BUG()
>> which kills Xen.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> diff -r 477ccdb9870e -r 0a1a3f35f56a xen/include/xen/lib.h
>> --- a/xen/include/xen/lib.h
>> +++ b/xen/include/xen/lib.h
>> @@ -48,6 +48,16 @@ do {                                    
>>  } while (0)
>>  #endif /* assert_printk_failed */
>>  
>> +#ifndef assert_run_failed
>> +#define assert_run_failed(p, func, arg)                         \
>> +do {                                                            \
>> +    printk("Assertion '%s' failed, line %d, file %s\n", p ,     \
>> +                   __LINE__, __FILE__);                         \
>> +    (func)((arg));                                              \
> Quite a few pointless parentheses here.

Ok

>
>> +    BUG();                                                      \
>> +} while (0)
>> +#endif /* assert_run_failed */
>> +
>>  #ifdef CONFIG_ASSERTS
>>  #define ASSERT(p) \
>>      do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
>> @@ -55,9 +65,15 @@ do {                                    
>>  #define ASSERT_PRINTK(p, ...)                                   \
>>      do { if ( unlikely(!(p)) )                                  \
>>              assert_printk_failed(#p, __VA_ARGS__); } while (0)
>> +
>> +/* func expected to be void, taking a single void * argument */
>> +#define ASSERT_RUN(p, func, arg)                                \
> Since the user of the construct specifies both func and arg, I don't
> see the need to specify what type they are. Nor is it meaningful
> here whether the function returns void (it would at most need to
> be stated - I'd consider this mostly obvious - that an eventual
> return value doesn't get used).

Good point

>
>> +    do { if ( unlikely(!(p)) )                                  \
>> +            assert_run_failed(#p, func, arg); } while (0)
>>  #else
>>  #define ASSERT(p) do { if ( 0 && (p) ); } while (0)
>>  #define ASSERT_PRINTK(p, ...) do { if ( 0 && (p) ); } while (0)
>> +#define ASSERT_RUN(p, func, arg) do { if ( 0 && (p) ); } while (0)
> You shall evaluate func and arg (i.e. invoke func(arg)) in the
> (dead) if() body, to both avoid the need for #ifdef-s at use
> sites and check type compatibility even in non-debug (or non-
> assert, following your earlier patch) builds.
>
> Jan

I presume that you mean I should?  Why would that prevent the need for
#ifdefs? I can see the argument for type compatibility.

~Andrew

>
>>  #endif
>>  
>>  #define ABS(_x) ({                              \
>
>

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Re: [PATCH 3 of 3] xen/debug: Introduce ASSERT_RUN()

[Jan Beulich]

>>> On 15.10.12 at 11:37, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> I presume that you mean I should?  Why would that prevent the need for
> #ifdefs? I can see the argument for type compatibility.

Because if func is a static function defined for use just in an
ASSERT_RUN(), the compiler would warn about it being unused
in non-debug builds.

Jan

Re: [PATCH 3 of 3] xen/debug: Introduce ASSERT_RUN()

[Andrew Cooper]

On 15/10/12 10:42, Jan Beulich wrote:
>>>> On 15.10.12 at 11:37, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> I presume that you mean I should?  Why would that prevent the need for
>> #ifdefs? I can see the argument for type compatibility.
> Because if func is a static function defined for use just in an
> ASSERT_RUN(), the compiler would warn about it being unused
> in non-debug builds.
>
> Jan
>

Ah yes.  I see now.

As I am going to respin these patches, would you like BUG and WARN
variants (of at least the PRINTK version) ?

~Andrew

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Re: [PATCH 3 of 3] xen/debug: Introduce ASSERT_RUN()

[Jan Beulich]

>>> On 15.10.12 at 11:52, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> As I am going to respin these patches, would you like BUG and WARN
> variants (of at least the PRINTK version) ?

For WARN, yes (but in its Linux incarnation, i.e. without any PRINTK
in the name). For a BUG() variant I'm not that sure of its usefulness.

Jan