Re: [Qemu-devel] [PATCH] translate-all: Enable locking debug in a debug build

[Pranith Kumar]

On Wed, Nov 16, 2016 at 10:57 AM, Alex Bennée <address@hidden> wrote:
>
> Pranith Kumar <address@hidden> writes:
>
>> Unconditionally enable locking checks in debug builds so that we get
>> wider testing. Using tcg_debug_assert() allows us to remove
>> DEBUG_LOCKING define.
>
> Interesting. The other option would be to add a debug build to
> .travis.yml that define this (and others) with -DFOO_DEBUG.
>
>>
>> Signed-off-by: Pranith Kumar <address@hidden>
>> ---
>>  translate-all.c | 50 +++++++++++++++++---------------------------------
>>  1 file changed, 17 insertions(+), 33 deletions(-)
>>
>> diff --git a/translate-all.c b/translate-all.c
>> index cf828aa..a03f323 100644
>> --- a/translate-all.c
>> +++ b/translate-all.c
>> @@ -60,7 +60,6 @@
>>
>>  /* #define DEBUG_TB_INVALIDATE */
>>  /* #define DEBUG_TB_FLUSH */
>> -/* #define DEBUG_LOCKING */
>>  /* make various TB consistency checks */
>>  /* #define DEBUG_TB_CHECK */
>
> So if we are enabling this for tcg_debug builds why not the other cases?

Ideally, we should enable all the debug checks in the debug build. I
didn't want to touch unrelated stuff in this patch. I can clean up all
these cases if you prefer.

>
>>
>> @@ -75,23 +74,13 @@
>>   * access to the memory related structures are protected with the
>>   * mmap_lock.
>>   */
>> -#ifdef DEBUG_LOCKING
>> -#define DEBUG_MEM_LOCKS 1
>> -#else
>> -#define DEBUG_MEM_LOCKS 0
>> -#endif
>> -
>
> In retrospect I should probably of had a comment in here about the roll
> of tb_lock in CONFIG_SOFTMMU versus the mmap_lock.
>
>>  #ifdef CONFIG_SOFTMMU
>>  #define assert_memory_lock() do {           \
>> -        if (DEBUG_MEM_LOCKS) {              \
>> -            g_assert(have_tb_lock);         \
>> -        }                                   \
>> +        tcg_debug_assert(have_tb_lock);     \
>>      } while (0)
>>  #else
>>  #define assert_memory_lock() do {               \
>> -        if (DEBUG_MEM_LOCKS) {                  \
>> -            g_assert(have_mmap_lock());         \
>> -        }                                       \
>> +        tcg_debug_assert(have_mmap_lock());     \
>>      } while (0)
>>  #endif
>>
>> @@ -172,16 +161,24 @@ static void page_table_config_init(void)
>>      assert(v_l2_levels >= 0);
>>  }
>>
>> +#define assert_tb_locked() do {             \
>> +        tcg_debug_assert(have_tb_lock);     \
>> +    } while (0)
>> +
>> +#define assert_tb_unlocked() do {           \
>> +        tcg_debug_assert(!have_tb_lock);    \
>> +    } while (0)
>> +
>
> I'm not sure we need all this multi-line stuff for a simple
> substitution? Richard?

OK, I will update this to a single line macro...

>
>>  void tb_lock(void)
>>  {
>> -    assert(!have_tb_lock);
>> +    assert_tb_unlocked();
>
> Hmm why introduce a helper for exactly one use?

...or can entirely remove the macro. I don't favour one over the other. :)


-- 
Pranith