Andrew Cooper
2012-Oct-08 18:16 UTC
[PATCH 0 of 3] Introduce more debugging flexibility with ASSERT() macros
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
Andrew Cooper
2012-Oct-08 18:16 UTC
[PATCH 1 of 3] xen/debug: Allow ASSERT() to be enabled in a non-debug build
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
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) ({ \
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) ({ \
Keir Fraser
2012-Oct-08 18:31 UTC
Re: [PATCH 0 of 3] Introduce more debugging flexibility with ASSERT() macros
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
Andrew Cooper
2012-Oct-09 09:55 UTC
Re: [PATCH 0 of 3] Introduce more debugging flexibility with ASSERT() macros
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. > > -- KeirI 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
>>> 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) ({ \
>>> 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) ({ \
Andrew Cooper
2012-Oct-15 09:29 UTC
Re: [PATCH 2 of 3] xen/debug: Introduce ASSERT_PRINTK()
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. > > JanKeir 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
>>> 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
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. > > JanI 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
>>> 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
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
>>> 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