Re: [PATCH 2/2] gdb: add SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simark@simark.ca> writes:

> On 12/9/22 12:42, Tom Tromey wrote:
>>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 
>> Andrew> After the previous commit converted symbol lookup debug to use the new
>> Andrew> debug scheme, this commit adds SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT.
>> 
>> Andrew> The previous commit updated 'set debug symbol-lookup' to use the new
>> Andrew> debug scheme, however SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT still does
>> Andrew> not exist.  The reason for this is that 'set debug symbol-lookup'
>> Andrew> controls an integer variable, rather than a bool, and, depending on
>> Andrew> the value of this variable, different amounts of debug will be
>> Andrew> produced.
>> 
>> Andrew> Currently the *_SCOPED_DEBUG_ENTER_EXIT mechanism will only work for
>> Andrew> control variables of type bool, this is because the underlying
>> Andrew> scoped_debug_start_end class can only handle variables of type bool.
>> 
>> Andrew> This commit templates scoped_debug_start_end so that the class can
>> Andrew> accept either a 'bool &' or an invokable object, e.g. a lambda
>> Andrew> function, or a function pointer.
>> 
>> Andrew> The existing scoped_debug_start_end and scoped_debug_enter_exit macros
>> Andrew> in common-debug.h are updated to support scoped_debug_enter_exit being
>> Andrew> templated, however, nothing outside of common-debug.h needs to change.
>> 
>> Andrew> I've then added SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT in symtab.h, and
>> Andrew> added a couple of token uses in symtab.c.  I didn't want to add too
>> Andrew> much in this first commit, this is really about updating
>> Andrew> common-debug.h to support this new ability.
>> 
>> This seems fine.  I suppose inlining will make it not cost too much... ?
>> 
>> Andrew> +#define SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT \
>> Andrew> +  scoped_debug_enter_exit ([&] () -> bool { \
>> Andrew> +    return symbol_lookup_debug > 0; \
>> Andrew> +  }, "symbol-lookup")
>> 
>> I wonder whether the [&] is needed here.  I couldn't see what it is
>> capturing.
>
> Same comment here.  Instead of a lambda, you could have single function:
>
> static bool
> symbol_lookup_debug_enabled ()
> {
>   return symbol_lookup_debug > 0;
> }

Thanks for the feedback.  I did originally have a free function here,
but wanted to check the code worked fine with a lambda too ... and it
just got left like that.

I'll switch it back to a free function before committing.

Thanks,
Andrew

>
> And then you could use it in symbol_lookup_debug_printf too, to factor
> the logic.  You could have symbol_lookup_debug_v_enabled too for
> symmetry if you wanted.
>
> Otherwise, it all LGTM:
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
>
> Simon