On Wed, Nov 16, 2016 at 10:57 AM, Alex Bennée <alex.ben...@linaro.org> wrote: > > Pranith Kumar <bobby.pr...@gmail.com> 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 <bobby.pr...@gmail.com> >> --- >> 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