Re: [PATCH] gdb: add linux_nat_debug_printf macro

[Joel Brobecker <brobecker@adacore.com>]

Hi Simon,

On Fri, Jul 10, 2020 at 09:55:40AM -0400, Simon Marchi wrote:
> On 2020-07-10 7:47 a.m., Gary Benson via Gdb-patches wrote:
> > Simon Marchi wrote:
> >> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> >> index fde360f5080a..3b5c803b83fd 100644
> >> --- a/gdb/linux-nat.c
> >> +++ b/gdb/linux-nat.c
> >> @@ -198,6 +198,27 @@ show_debug_linux_nat (struct ui_file *file, int from_tty,
> >>  		    value);
> >>  }
> >>
> >> +/* Print a debug statement.  Should be used through linux_nat_debug_printf.  */
> >> +
> >> +static void ATTRIBUTE_PRINTF (2, 3)
> >> +linux_nat_debug_printf_1 (const char *func_name, const char *fmt, ...)
> >> +{
> >> +  fprintf_unfiltered (gdb_stdlog, "[linux-nat] %s: ", func_name);
> >> +
> >> +  va_list ap;
> >> +  va_start (ap, fmt);
> >> +  vfprintf_unfiltered (gdb_stdlog, fmt, ap);
> >> +  va_end (ap);
> >> +
> >> +  fprintf_unfiltered (gdb_stdlog, "\n");
> >> +}
> > 
> > Would it be useful to use debug_printf and debug_vprintf here?
> > 
> > Thanks,
> > Gary
> 
> Indeed, I think the result would be the same, and it seems to exist for this purpose.  I've
> replaced locally the implementation of linux_nat_debug_printf_1 with:
> 
> static void ATTRIBUTE_PRINTF (2, 3)
> linux_nat_debug_printf_1 (const char *func_name, const char *fmt, ...)
> {
>   debug_printf ("[linux-nat] %s: ", func_name);
> 
>   va_list ap;
>   va_start (ap, fmt);
>   debug_vprintf (fmt, ap);
>   va_end (ap);
> 
>   debug_printf ("\n");
> }

I skimmed through the patch, and it looked good to me.

Thanks again for doing that. That's a nice improvement.

Thinking out loud (and certainly not a suggestion or request that
you take care of the implementation): I'm wondering if we should be
thinking about generalizing this approach, somehow. For instance,
if we a function that took not just a func name, but also the name
of the unit:

    unit_debug_vprintf (const char *module_name, const char *func_name,
                        const char *fmt, ...)

... or perhaps more generally:

    unit_debug_vprintf (const char *label, const char *func_name,
                        const char *fmt, ...)

... then all modules would have to do to have consistent logging
is to define one macro.

-- 
Joel