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

[Xen-devel] [PATCH] 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 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(-)

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


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

Re: [Xen-devel] [PATCH] xen: make CONFIG_DEBUG_LOCKS usable without CONFIG_DEBUG

[George Dunlap]

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

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

Re: [Xen-devel] [PATCH] xen: make CONFIG_DEBUG_LOCKS usable without CONFIG_DEBUG

[Jürgen Groß]

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


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

Re: [Xen-devel] [PATCH] xen: make CONFIG_DEBUG_LOCKS usable without CONFIG_DEBUG

[Jan Beulich]

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

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

Re: [Xen-devel] [PATCH] xen: make CONFIG_DEBUG_LOCKS usable without CONFIG_DEBUG

[Jan Beulich]

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

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

Re: [Xen-devel] [PATCH] xen: make CONFIG_DEBUG_LOCKS usable without CONFIG_DEBUG

[George Dunlap]

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

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

Re: [Xen-devel] [PATCH] xen: make CONFIG_DEBUG_LOCKS usable without CONFIG_DEBUG

[Jan Beulich]

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

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

Re: [Xen-devel] [PATCH] xen: make CONFIG_DEBUG_LOCKS usable without CONFIG_DEBUG

[Jürgen Groß]

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

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