Re: [PATCH 04/11 v5] Introduce and use debug_printf and debug_vprintf

[Doug Evans <dje@google.com>]

Gary Benson writes:
 > This introduces debug_vprintf (a function that clients of "common" are
 > expected to implement) and debug_printf (a wrapper for debug_vprintf).
 > A debug_vprintf function is written from scratch for GDB; gdbserver's
 > existing debug_printf is repurposed as debug_vprintf.
 > 
 > common/agent.c is changed to use debug_vprintf rather than
 > defining the macro DEBUG_AGENT depending on GDBSERVER.
 > 
 > nat/i386-dregs.c is changed to use the externally-implemented
 > debug_printf, rather than defining it itself.
 > 
 > gdb/
 > 2014-08-01  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* common/common-debug.h: New file.
 > 	* common/common-debug.c: Likewise.
 > 	* debug.c: Likewise.
 > 	* Makefile.in (HFILES_NO_SRCDIR): Add common/common-debug.h.
 > 	(COMMON_OBS): Add common-debug.o and debug.o.
 > 	(common-debug.o): New rule.
 > 	* common/common-defs.h: Include common-debug.h.
 > 	* common/agent.c (debug_agent_print): New function.
 > 	(DEBUG_AGENT): Redefine.
 > 	* nat/i386-dregs.c (debug_printf): Undefine.
 > 
 > gdb/gdbserver/
 > 2014-08-01  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* Makefile.in (SFILES): Add common/common-debug.c.
 > 	(OBS): Add common-debug.o.
 > 	(common-debug.o): New rule.
 > 	* debug.h (debug_printf): Don't declare.
 > 	* debug.c (debug_printf): Renamed and rewritten as...
 > 	(debug_vprintf): New function.
 > ---
 >  gdb/ChangeLog             |   14 ++++++++++++++
 >  gdb/Makefile.in           |    9 +++++++--
 >  gdb/common/agent.c        |   24 +++++++++++++++---------
 >  gdb/common/common-debug.c |   37 +++++++++++++++++++++++++++++++++++++
 >  gdb/common/common-debug.h |   35 +++++++++++++++++++++++++++++++++++
 >  gdb/common/common-defs.h  |    1 +
 >  gdb/debug.c               |   28 ++++++++++++++++++++++++++++
 >  gdb/gdbserver/ChangeLog   |   10 ++++++++++
 >  gdb/gdbserver/Makefile.in |   10 +++++++---
 >  gdb/gdbserver/debug.c     |   11 ++++-------
 >  gdb/gdbserver/debug.h     |    1 -
 >  gdb/nat/i386-dregs.c      |    4 ----
 >  12 files changed, 158 insertions(+), 26 deletions(-)
 >  create mode 100644 gdb/common/common-debug.c
 >  create mode 100644 gdb/common/common-debug.h
 >  create mode 100644 gdb/debug.c
 > 
 > diff --git a/gdb/Makefile.in b/gdb/Makefile.in
 > index 2fac59d..8429ebc 100644
 > --- a/gdb/Makefile.in
 > +++ b/gdb/Makefile.in
 > @@ -935,7 +935,8 @@ gdb_bfd.h sparc-ravenscar-thread.h ppc-ravenscar-thread.h nat/linux-btrace.h \
 >  ctf.h nat/i386-cpuid.h nat/i386-gcc-cpuid.h target/resume.h \
 >  target/wait.h target/waitstatus.h nat/linux-nat.h nat/linux-waitpid.h \
 >  common/print-utils.h common/rsp-low.h nat/i386-dregs.h x86-linux-nat.h \
 > -i386-linux-nat.h common/common-defs.h common/errors.h common/common-types.h
 > +i386-linux-nat.h common/common-defs.h common/errors.h common/common-types.h \
 > +common/common-debug.h
 >  
 >  # Header files that already have srcdir in them, or which are in objdir.
 >  
 > @@ -1034,7 +1035,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
 >  	gdb_vecs.o jit.o progspace.o skip.o probe.o \
 >  	common-utils.o buffer.o ptid.o gdb-dlfcn.o common-agent.o \
 >  	format.o registry.o btrace.o record-btrace.o waitstatus.o \
 > -	print-utils.o rsp-low.o errors.o
 > +	print-utils.o rsp-low.o errors.o common-debug.o debug.o
 >  
 >  TSOBS = inflow.o
 >  
 > @@ -2148,6 +2149,10 @@ errors.o: ${srcdir}/common/errors.c
 >  	$(COMPILE) $(srcdir)/common/errors.c
 >  	$(POSTCOMPILE)
 >  
 > +common-debug.o: ${srcdir}/common/common-debug.c
 > +	$(COMPILE) $(srcdir)/common/common-debug.c
 > +	$(POSTCOMPILE)
 > +
 >  #
 >  # gdb/target/ dependencies
 >  #
 > diff --git a/gdb/common/agent.c b/gdb/common/agent.c
 > index 3f868ba..7071ce6 100644
 > --- a/gdb/common/agent.c
 > +++ b/gdb/common/agent.c
 > @@ -31,15 +31,21 @@
 >  
 >  int debug_agent = 0;
 >  
 > -#ifdef GDBSERVER
 > -#define DEBUG_AGENT(fmt, args...)	\
 > -  if (debug_agent)			\
 > -    fprintf (stderr, fmt, ##args);
 > -#else
 > -#define DEBUG_AGENT(fmt, args...)	\
 > -  if (debug_agent)			\
 > -    fprintf_unfiltered (gdb_stdlog, fmt, ##args);
 > -#endif
 > +/* A stdarg wrapper for debug_vprintf.  */
 > +
 > +static void ATTRIBUTE_PRINTF (1, 2)
 > +debug_agent_print (const char *fmt, ...)

Nit: debug_agent_printf ?

 > +{
 > +  va_list ap;
 > +
 > +  if (!debug_agent)
 > +    return;
 > +  va_start (ap, fmt);
 > +  debug_vprintf (fmt, ap);
 > +  va_end (ap);
 > +}
 > +
 > +#define DEBUG_AGENT debug_agent_print
 >  
 >  /* Global flag to determine using agent or not.  */
 >  int use_agent = 0;
 > [...]
 > diff --git a/gdb/common/common-debug.h b/gdb/common/common-debug.h
 > new file mode 100644
 > index 0000000..0467610
 > --- /dev/null
 > +++ b/gdb/common/common-debug.h
 > @@ -0,0 +1,35 @@
 > +/* Declarations for debug printing functions.
 > +
 > +   Copyright (C) 2014 Free Software Foundation, Inc.
 > +
 > +   This file is part of GDB.
 > +
 > +   This program is free software; you can redistribute it and/or modify
 > +   it under the terms of the GNU General Public License as published by
 > +   the Free Software Foundation; either version 3 of the License, or
 > +   (at your option) any later version.
 > +
 > +   This program is distributed in the hope that it will be useful,
 > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
 > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 > +   GNU General Public License for more details.
 > +
 > +   You should have received a copy of the GNU General Public License
 > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 > +
 > +#ifndef COMMON_DEBUG_H
 > +#define COMMON_DEBUG_H
 > +
 > +/* Print a formatted message to the appropriate channel for debugging
 > +   output for the client.  */
 > +
 > +extern void debug_printf (const char *format, ...)
 > +     ATTRIBUTE_PRINTF (1, 2);
 > +
 > +/* Print a formatted message to the appropriate channel for debugging
 > +   output for the client.  */

IWBN to include mention that this function must be provided by the client.
[I'm not sure what other similar decls do, but IWBN if they all had
a similar comment.]

 > +
 > +extern void debug_vprintf (const char *format, va_list ap)
 > +     ATTRIBUTE_PRINTF (1, 0);
 > +
 > +#endif /* COMMON_DEBUG_H */
 > [...]

The rest of the patch LGTM.