| Message ID | 20200109054031.18455-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: make CONFIG_DEBUG_LOCKS usable without CONFIG_DEBUG | expand |
On 1/9/20 5:40 AM, Juergen Gross wrote: > 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. Any reason not to use BUG_ON() in that case? Having two different asserts is almost certainly going to cause bugs. -George
On 09.01.20 11:07, George Dunlap wrote: > On 1/9/20 5:40 AM, Juergen Gross wrote: >> 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. > > Any reason not to use BUG_ON() in that case? The main reason is the missing message which condition failed. A rename ("BUG_ASSERT"?) could be an alternative to just dropping the message. Both would be fine with me. > > Having two different asserts is almost certainly going to cause bugs. Obviously having only one is enough for bugs already. ;-) Juergen
On 09.01.2020 11:07, George Dunlap wrote: > On 1/9/20 5:40 AM, Juergen Gross wrote: >> 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. > > Any reason not to use BUG_ON() in that case? > > Having two different asserts is almost certainly going to cause bugs. Furthermore, assert() is a C standard library construct, mandated to be controlled by NDEBUG. I.e. if anything at all, ASSERT() could be made behave differently, but of course this would be quite big a change to the code base. +1 to (continue) using BUG_ON() anywhere we want more than just debug build checking. Jan
On 09.01.2020 11:15, Jürgen Groß wrote: > On 09.01.20 11:07, George Dunlap wrote: >> On 1/9/20 5:40 AM, Juergen Gross wrote: >>> 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. >> >> Any reason not to use BUG_ON() in that case? > > The main reason is the missing message which condition failed. > > A rename ("BUG_ASSERT"?) could be an alternative to just dropping > the message. Both would be fine with me. How about if ( ... ) { printk(...); BUG(); } ? Jan
On 1/9/20 10:30 AM, Jan Beulich wrote: > On 09.01.2020 11:15, Jürgen Groß wrote: >> On 09.01.20 11:07, George Dunlap wrote: >>> On 1/9/20 5:40 AM, Juergen Gross wrote: >>>> 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. >>> >>> Any reason not to use BUG_ON() in that case? >> >> The main reason is the missing message which condition failed. >> >> A rename ("BUG_ASSERT"?) could be an alternative to just dropping >> the message. Both would be fine with me. > > How about > > if ( ... ) > { > printk(...); > BUG(); > } Is there a reason we can't make BUG_ON() print the condition? -George
On 09.01.2020 11:39, George Dunlap wrote: > On 1/9/20 10:30 AM, Jan Beulich wrote: >> On 09.01.2020 11:15, Jürgen Groß wrote: >>> On 09.01.20 11:07, George Dunlap wrote: >>>> On 1/9/20 5:40 AM, Juergen Gross wrote: >>>>> 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. >>>> >>>> Any reason not to use BUG_ON() in that case? >>> >>> The main reason is the missing message which condition failed. >>> >>> A rename ("BUG_ASSERT"?) could be an alternative to just dropping >>> the message. Both would be fine with me. >> >> How about >> >> if ( ... ) >> { >> printk(...); >> BUG(); >> } > > Is there a reason we can't make BUG_ON() print the condition? Of course we could, in principle, at the price of a meaningful growth of the .rodata section. If we do this, perhaps we'd want something like Linux'es CONFIG_DEBUG_BUGVERBOSE to control this. Jan
On 09.01.20 11:45, Jan Beulich wrote: > On 09.01.2020 11:39, George Dunlap wrote: >> On 1/9/20 10:30 AM, Jan Beulich wrote: >>> On 09.01.2020 11:15, Jürgen Groß wrote: >>>> On 09.01.20 11:07, George Dunlap wrote: >>>>> On 1/9/20 5:40 AM, Juergen Gross wrote: >>>>>> 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. >>>>> >>>>> Any reason not to use BUG_ON() in that case? >>>> >>>> The main reason is the missing message which condition failed. >>>> >>>> A rename ("BUG_ASSERT"?) could be an alternative to just dropping >>>> the message. Both would be fine with me. >>> >>> How about >>> >>> if ( ... ) >>> { >>> printk(...); >>> BUG(); >>> } >> >> Is there a reason we can't make BUG_ON() print the condition? > > Of course we could, in principle, at the price of a meaningful > growth of the .rodata section. If we do this, perhaps we'd want > something like Linux'es CONFIG_DEBUG_BUGVERBOSE to control this. In case nobody objects I'll modify my patch to do that (well, split it to introduce that option and then use it). Juergen
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c index 286f916bca..8f54580d24 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()); + assert(debug->cpu == smp_processor_id()); debug->cpu = SPINLOCK_NO_CPU; } diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index 8fbe84032d..000ea677d0 100644 --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -32,9 +32,11 @@ #define gcov_string "" #endif -#ifndef NDEBUG -#define ASSERT(p) \ +#define assert(p) \ do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0) + +#ifndef NDEBUG +#define ASSERT(p) assert(p) #define ASSERT_UNREACHABLE() assert_failed("unreachable") #define debug_build() 1 #else
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 introducing assert() doing the same as ASSERT(), but being available in non-debug builds, too, and use that in spinlock debug code. Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/common/spinlock.c | 2 +- xen/include/xen/lib.h | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-)