[PATCH 00/11 v5] Common code cleanups

[PATCH 00/11 v5] Common code cleanups

[Gary Benson]

Hi all,

This series contains updated versions of the common code cleanups
patches I've posted previously.  These have been reworked (in some
cases recreated) to apply on top of my "Include some headers in
common-defs.h" series that is currently under review [1, 2].

Current gdb/master has 32 uses of "#ifdef GDBSERVER".  This series
reduces that count to three:

  - One in common/common-defs.h, to select the appropriate gnulib
    config.h.  This use can be removed when the build is modified
    to build only one copy of gnulib.

  - One in common/common-types.h, to define types based on BFD's
    types (for GDB) or otherwise (for gdbserver).  I'm not sure
    what form removing this use would take.

  - One in linux-waitpid.c to protect some gdbserver-specific
    thread debugging code which can be removed when the Linux
    thread_db code is unified and made shared.

All built and regtested on x86-64 RHEL6.5.  mips-linux-watch.h
changes checked by cross-building gdbserver with the Sourcery
CodeBench for MIPS GNU/Linux toolchain.

If this series is approved I will update the "Include Files" section
of the Coding Standards page [3] to insert the following line after
the existing first line:

  "All .c files under gdb/common/, gdb/nat/ and gdb/target/ must
  include common-defs.h as their first non-comment line."

I will also update the subsequent mentions of common-defs.h
accordingly.

Is this ok to commit?

Thanks,
Gary

--
[1] https://sourceware.org/ml/gdb-patches/2014-07/msg00736.html
[2] https://sourceware.org/ml/gdb-patches/2014-07/msg00820.html
[3] https://sourceware.org/gdb/wiki/Internals GDB-C-Coding-Standards

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

[Gary Benson]

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, ...)
+{
+  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.c b/gdb/common/common-debug.c
new file mode 100644
index 0000000..660fc70
--- /dev/null
+++ b/gdb/common/common-debug.c
@@ -0,0 +1,37 @@
+/* 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/>.  */
+
+#ifdef GDBSERVER
+#include "server.h"
+#else
+#include "defs.h"
+#endif
+#include "common-debug.h"
+
+/* See common/common-debug.h.  */
+
+void
+debug_printf (const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+  debug_vprintf (fmt, ap);
+  va_end (ap);
+}
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.  */
+
+extern void debug_vprintf (const char *format, va_list ap)
+     ATTRIBUTE_PRINTF (1, 0);
+
+#endif /* COMMON_DEBUG_H */
diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index 66c0d21..2d3444b 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -44,5 +44,6 @@
 #include "errors.h"
 #include "common-types.h"
 #include "print-utils.h"
+#include "common-debug.h"
 
 #endif /* COMMON_DEFS_H */
diff --git a/gdb/debug.c b/gdb/debug.c
new file mode 100644
index 0000000..95cf617
--- /dev/null
+++ b/gdb/debug.c
@@ -0,0 +1,28 @@
+/* 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/>.  */
+
+#include "defs.h"
+
+/* See common/common-debug.h.  */
+
+void
+debug_vprintf (const char *fmt, va_list ap)
+{
+  vfprintf_unfiltered (gdb_stdlog, fmt, ap);
+}
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 1faa00c..18486c6 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -169,7 +169,8 @@ SFILES=	$(srcdir)/gdbreplay.c $(srcdir)/inferiors.c $(srcdir)/dll.c \
 	$(srcdir)/common/buffer.c $(srcdir)/nat/linux-btrace.c \
 	$(srcdir)/common/filestuff.c $(srcdir)/target/waitstatus.c \
 	$(srcdir)/nat/mips-linux-watch.c $(srcdir)/common/print-utils.c \
-	$(srcdir)/common/rsp-low.c $(srcdir)/common/errors.c
+	$(srcdir)/common/rsp-low.c $(srcdir)/common/errors.c \
+	$(srcdir)/common/common-debug.c
 
 DEPFILES = @GDBSERVER_DEPFILES@
 
@@ -182,8 +183,8 @@ OBS = agent.o ax.o inferiors.o regcache.o remote-utils.o server.o signals.o \
       target.o waitstatus.o utils.o debug.o version.o vec.o gdb_vecs.o \
       mem-break.o hostio.o event-loop.o tracepoint.o xml-utils.o \
       common-utils.o ptid.o buffer.o format.o filestuff.o dll.o notif.o \
-      tdesc.o print-utils.o rsp-low.o errors.o $(XML_BUILTIN) $(DEPFILES) \
-      $(LIBOBJS)
+      tdesc.o print-utils.o rsp-low.o errors.o common-debug.o \
+      $(XML_BUILTIN) $(DEPFILES) $(LIBOBJS)
 GDBREPLAY_OBS = gdbreplay.o version.o
 GDBSERVER_LIBS = @GDBSERVER_LIBS@
 XM_CLIBS = @LIBS@
@@ -540,6 +541,9 @@ agent.o: ../common/agent.c
 errors.o: ../common/errors.c
 	$(COMPILE) $<
 	$(POSTCOMPILE)
+common-debug.o: ../common/common-debug.c
+	$(COMPILE) $<
+	$(POSTCOMPILE)
 waitstatus.o: ../target/waitstatus.c
 	$(COMPILE) $<
 	$(POSTCOMPILE)
diff --git a/gdb/gdbserver/debug.c b/gdb/gdbserver/debug.c
index c50af76..339e33c 100644
--- a/gdb/gdbserver/debug.c
+++ b/gdb/gdbserver/debug.c
@@ -33,9 +33,8 @@ int debug_timestamp;
    previous call ended with "\n".  */
 
 void
-debug_printf (const char *msg, ...)
+debug_vprintf (const char *format, va_list ap)
 {
-  va_list args;
 #if !defined (IN_PROCESS_AGENT)
   /* N.B. Not thread safe, and can't be used, as is, with IPA.  */
   static int new_line = 1;
@@ -53,13 +52,11 @@ debug_printf (const char *msg, ...)
     }
 #endif
 
-  va_start (args, msg);
-  vfprintf (stderr, msg, args);
-  va_end (args);
+  vfprintf (stderr, format, ap);
 
 #if !defined (IN_PROCESS_AGENT)
-  if (*msg)
-    new_line = msg[strlen (msg) - 1] == '\n';
+  if (*format)
+    new_line = format[strlen (format) - 1] == '\n';
 #endif
 }
 
diff --git a/gdb/gdbserver/debug.h b/gdb/gdbserver/debug.h
index 0f056ca..42a3f21 100644
--- a/gdb/gdbserver/debug.h
+++ b/gdb/gdbserver/debug.h
@@ -29,7 +29,6 @@
 extern int debug_threads;
 extern int debug_timestamp;
 
-void debug_printf (const char *msg, ...) ATTRIBUTE_PRINTF (1, 2);
 void debug_flush (void);
 void do_debug_enter (const char *function_name);
 void do_debug_exit (const char *function_name);
diff --git a/gdb/nat/i386-dregs.c b/gdb/nat/i386-dregs.c
index 1fa5c19..e3272cd 100644
--- a/gdb/nat/i386-dregs.c
+++ b/gdb/nat/i386-dregs.c
@@ -178,10 +178,6 @@ typedef enum { WP_INSERT, WP_REMOVE, WP_COUNT } i386_wp_op_t;
 #ifndef GDBSERVER
 /* Whether or not to print the mirrored debug registers.  */
 extern int debug_hw_points;
-
-/* Print debugging messages.  */
-#define debug_printf(fmt, args...) \
-  fprintf_unfiltered (gdb_stdlog, fmt, ##args);
 #endif
 
 /* Print the values of the mirrored debug registers.  */
-- 
1.7.1

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

[Doug Evans]

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.

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

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
> > +/* A stdarg wrapper for debug_vprintf.  */
> > +
> > +static void ATTRIBUTE_PRINTF (1, 2)
> > +debug_agent_print (const char *fmt, ...)
> 
> Nit: debug_agent_printf ?

I will change this.

>  > +/* 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.]

That's a good idea.  I'll add this here and everywhere else that needs
it.

Thanks,
Gary

-- 
http://gbenson.net/

[PATCH 03/11 v5] Move print-utils.h to common-defs.h

[Gary Benson]

This commit moves the inclusion of print-utils.h to common-defs.h
and removes all other inclusions.

gdb/
2014-08-01  Gary Benson  <gbenson@redhat.com>

	* common/common-defs.h: Include print-utils.h.
	* utils.h: Do not include print-utils.h.

gdb/gdbserver/
2014-08-01  Gary Benson  <gbenson@redhat.com>

	* utils.h: Do not include print-utils.h.
---
 gdb/ChangeLog            |    5 +++++
 gdb/common/common-defs.h |    1 +
 gdb/gdbserver/ChangeLog  |    4 ++++
 gdb/gdbserver/utils.h    |    2 --
 gdb/utils.h              |    1 -
 5 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index 5543e4c..66c0d21 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -43,5 +43,6 @@
 #include "gdb_assert.h"
 #include "errors.h"
 #include "common-types.h"
+#include "print-utils.h"
 
 #endif /* COMMON_DEFS_H */
diff --git a/gdb/gdbserver/utils.h b/gdb/gdbserver/utils.h
index a994f38..cdd80df 100644
--- a/gdb/gdbserver/utils.h
+++ b/gdb/gdbserver/utils.h
@@ -19,8 +19,6 @@
 #ifndef UTILS_H
 #define UTILS_H
 
-#include "print-utils.h"
-
 void fatal (const char *string,...) ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
 char *paddress (CORE_ADDR addr);
 char *pfildes (gdb_fildes_t fd);
diff --git a/gdb/utils.h b/gdb/utils.h
index 18a95a7..57a1c0f 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -23,7 +23,6 @@
 
 #include "cleanups.h"
 #include "exceptions.h"
-#include "print-utils.h"
 
 extern void initialize_utils (void);
 
-- 
1.7.1

Re: [PATCH 03/11 v5] Move print-utils.h to common-defs.h

[Doug Evans]

Gary Benson writes:
 > This commit moves the inclusion of print-utils.h to common-defs.h
 > and removes all other inclusions.
 > 
 > gdb/
 > 2014-08-01  Gary Benson  <gbenson@redhat.com>
 > 
 > 	* common/common-defs.h: Include print-utils.h.
 > 	* utils.h: Do not include print-utils.h.
 > 
 > gdb/gdbserver/
 > 2014-08-01  Gary Benson  <gbenson@redhat.com>
 > 
 > 	* utils.h: Do not include print-utils.h.
 > ---
 >  gdb/ChangeLog            |    5 +++++
 >  gdb/common/common-defs.h |    1 +
 >  gdb/gdbserver/ChangeLog  |    4 ++++
 >  gdb/gdbserver/utils.h    |    2 --
 >  gdb/utils.h              |    1 -
 >  5 files changed, 10 insertions(+), 3 deletions(-)
 > 
 > diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
 > index 5543e4c..66c0d21 100644
 > --- a/gdb/common/common-defs.h
 > +++ b/gdb/common/common-defs.h
 > @@ -43,5 +43,6 @@
 >  #include "gdb_assert.h"
 >  #include "errors.h"
 >  #include "common-types.h"
 > +#include "print-utils.h"
 >  
 >  #endif /* COMMON_DEFS_H */

It's a bit odd to see common-defs.h include print-utils.h and not
common-utils.h.  I see it gets included by gdb_assert.h, but
as a reader I'm still left with the question of wondering what's
going on and wanting to spend time digging into it.
I wonder if we can improve this somehow.
[No need to work on that in this patch set though.]

Re: [PATCH 03/11 v5] Move print-utils.h to common-defs.h

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
> > diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
> > index 5543e4c..66c0d21 100644
> > --- a/gdb/common/common-defs.h
> > +++ b/gdb/common/common-defs.h
> > @@ -43,5 +43,6 @@
> >  #include "gdb_assert.h"
> >  #include "errors.h"
> >  #include "common-types.h"
> > +#include "print-utils.h"
> >  
> >  #endif /* COMMON_DEFS_H */
> 
> It's a bit odd to see common-defs.h include print-utils.h and not
> common-utils.h.  I see it gets included by gdb_assert.h, but
> as a reader I'm still left with the question of wondering what's
> going on and wanting to spend time digging into it.
> I wonder if we can improve this somehow.
> [No need to work on that in this patch set though.]

This version of this series needs to be applied on top of this series:
https://sourceware.org/ml/gdb-patches/2014-07/msg00736.html
common-utils.h is moved into common-defs.h in patch 11 of that series
(and all other inclusions are removed, including the one in gdb_assert.h)

If it helps I have both series in a branch:
https://github.com/gbenson/binutils-gdb/tree/common-defs

Cheers,
Gary

-- 
http://gbenson.net/

[PATCH 02/11 v5] Introduce common-types.h

[Gary Benson]

This introduces common-types.h.  This file defines various standard
types used by gdb and gdbserver.

Currently these types are conditionally defined based on GDBSERVER.
The long term goal is to remove all such tests; however, this is
difficult as currently gdb uses definitions from BFD.  In the meantime
this is still a step in the right direction.

gdb/
2014-08-01  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* common/common-types.h: New file.
	* Makefile.in (HFILES_NO_SRCDIR): Add common/common-types.h.
	* common/common-defs.h: Include common-types.h.
	* defs.h (gdb_byte, CORE_ADDR, CORE_ADDR_MAX, LONGEST)
	(ULONGEST): Remove.

gdb/gdbserver/
2014-08-01  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* server.h: Add static assertion.
	(gdb_byte, CORE_ADDR, LONGEST, ULONGEST): Remove.
---
 gdb/ChangeLog             |    9 ++++++
 gdb/Makefile.in           |    2 +-
 gdb/common/common-defs.h  |    1 +
 gdb/common/common-types.h |   61 +++++++++++++++++++++++++++++++++++++++++++++
 gdb/defs.h                |   29 ---------------------
 gdb/gdbserver/ChangeLog   |    6 ++++
 gdb/gdbserver/server.h    |   13 +--------
 7 files changed, 80 insertions(+), 41 deletions(-)
 create mode 100644 gdb/common/common-types.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 4d3e7e5..2fac59d 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -935,7 +935,7 @@ 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
+i386-linux-nat.h common/common-defs.h common/errors.h common/common-types.h
 
 # Header files that already have srcdir in them, or which are in objdir.
 
diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index a15423c..5543e4c 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -42,5 +42,6 @@
 #include "common-utils.h"
 #include "gdb_assert.h"
 #include "errors.h"
+#include "common-types.h"
 
 #endif /* COMMON_DEFS_H */
diff --git a/gdb/common/common-types.h b/gdb/common/common-types.h
new file mode 100644
index 0000000..9fa1c24
--- /dev/null
+++ b/gdb/common/common-types.h
@@ -0,0 +1,61 @@
+/* Declarations for common types.
+
+   Copyright (C) 1986-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_TYPES_H
+#define COMMON_TYPES_H
+
+#ifdef GDBSERVER
+
+/* * A byte from the program being debugged.  */
+typedef unsigned char gdb_byte;
+
+typedef unsigned long long CORE_ADDR;
+
+typedef long long LONGEST;
+typedef unsigned long long ULONGEST;
+
+#else /* GDBSERVER */
+
+#include "bfd.h"
+
+/* * A byte from the program being debugged.  */
+typedef bfd_byte gdb_byte;
+
+/* * An address in the program being debugged.  Host byte order.  */
+typedef bfd_vma CORE_ADDR;
+
+/* This is to make sure that LONGEST is at least as big as CORE_ADDR.  */
+
+#ifdef BFD64
+
+typedef BFD_HOST_64_BIT LONGEST;
+typedef BFD_HOST_U_64_BIT ULONGEST;
+
+#else /* No BFD64 */
+
+typedef long long LONGEST;
+typedef unsigned long long ULONGEST;
+
+#endif /* No BFD64 */
+#endif /* GDBSERVER */
+
+/* * The largest CORE_ADDR value.  */
+#define CORE_ADDR_MAX (~ (CORE_ADDR) 0)
+
+#endif /* COMMON_TYPES_H */
diff --git a/gdb/defs.h b/gdb/defs.h
index b7271a7..8914512 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -76,35 +76,6 @@
 
 #include "hashtab.h"
 
-/* Rather than duplicate all the logic in BFD for figuring out what
-   types to use (which can be pretty complicated), symply define them
-   in terms of the corresponding type from BFD.  */
-
-#include "bfd.h"
-
-/* * A byte from the program being debugged.  */
-typedef bfd_byte gdb_byte;
-
-/* * An address in the program being debugged.  Host byte order.  */
-typedef bfd_vma CORE_ADDR;
-
-/* * The largest CORE_ADDR value.  */
-#define CORE_ADDR_MAX (~ (CORE_ADDR) 0)
-
-/* This is to make sure that LONGEST is at least as big as CORE_ADDR.  */
-
-#ifdef BFD64
-
-#define LONGEST BFD_HOST_64_BIT
-#define ULONGEST BFD_HOST_U_64_BIT
-
-#else /* No BFD64 */
-
-#define LONGEST long long
-#define ULONGEST unsigned long long
-
-#endif /* No BFD64 */
-
 #ifndef min
 #define min(a, b) ((a) < (b) ? (a) : (b))
 #endif
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index db6ddde..e6b2277 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -21,6 +21,8 @@
 
 #include "common-defs.h"
 
+gdb_static_assert (sizeof (CORE_ADDR) >= sizeof (void *));
+
 #ifdef __MINGW32CE__
 #include "wincecompat.h"
 #endif
@@ -63,19 +65,8 @@ int vsnprintf(char *str, size_t size, const char *format, va_list ap);
 #  define PROG "gdbserver"
 #endif
 
-/* A type used for binary buffers.  */
-typedef unsigned char gdb_byte;
-
 #include "buffer.h"
 #include "xml-utils.h"
-
-/* FIXME: This should probably be autoconf'd for.  It's an integer type at
-   least the size of a (void *).  */
-typedef unsigned long long CORE_ADDR;
-
-typedef long long LONGEST;
-typedef unsigned long long ULONGEST;
-
 #include "regcache.h"
 #include "gdb_signals.h"
 #include "target.h"
-- 
1.7.1

Re: [PATCH 02/11 v5] Introduce common-types.h

[Doug Evans]

Gary Benson writes:
 > This introduces common-types.h.  This file defines various standard
 > types used by gdb and gdbserver.

Side discussion: It's a bit odd to see common-types.h and then see
errors.h spelled as errors.h and not common-errors.h.
[Or vice versa.]

Is consistency in the file names a goal?
I realize common-utils.h needs to be something other than utils.h
because gdb and gdbserver each still have their own.
And similarly for common-defs.h - gdb still has defs.h.
But nothing else in common/ is spelled common-foo,
and I wonder if less randomness would be useful.
I'd be happy with #include "common/errors.h", and so on.
That way when I see it used I know where to look without having to
rely on memory or ls.

 > Currently these types are conditionally defined based on GDBSERVER.
 > The long term goal is to remove all such tests; however, this is
 > difficult as currently gdb uses definitions from BFD.  In the meantime
 > this is still a step in the right direction.
 > 
 > gdb/
 > 2014-08-01  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* common/common-types.h: New file.
 > 	* Makefile.in (HFILES_NO_SRCDIR): Add common/common-types.h.
 > 	* common/common-defs.h: Include common-types.h.
 > 	* defs.h (gdb_byte, CORE_ADDR, CORE_ADDR_MAX, LONGEST)
 > 	(ULONGEST): Remove.
 > 
 > gdb/gdbserver/
 > 2014-08-01  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* server.h: Add static assertion.
 > 	(gdb_byte, CORE_ADDR, LONGEST, ULONGEST): Remove.
 >[...]
 > diff --git a/gdb/common/common-types.h b/gdb/common/common-types.h
 > new file mode 100644
 > index 0000000..9fa1c24
 > --- /dev/null
 > +++ b/gdb/common/common-types.h
 > @@ -0,0 +1,61 @@
 > +/* Declarations for common types.
 > +
 > +   Copyright (C) 1986-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_TYPES_H
 > +#define COMMON_TYPES_H

Nit, and I do mean nit :-): errors.h uses COMMON_ERRORS_H,
so should this be COMMON_COMMON_TYPES_H?

[PATCH 09/11 v5] Remove GDBSERVER uses from linux-btrace.c

[Gary Benson]

This commit makes nat/linux-btrace.c include common-defs.h rather
than defs.h or server.h.  A couple of minor changes were required
to support this change.

gdb/
2014-08-01  Gary Benson  <gbenson@redhat.com>

	* nat/linux-btrace.c: Include common-defs.h.
	Don't include defs.h, server.h or gdbthread.h.
	* nat/linux-btrace.h (struct target_ops): New forward declaration.
---
 gdb/ChangeLog          |    6 ++++++
 gdb/nat/linux-btrace.c |    8 +-------
 gdb/nat/linux-btrace.h |    2 ++
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index b32d04c..9d9f8f9 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -19,15 +19,9 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
-
+#include "common-defs.h"
 #include "linux-btrace.h"
 #include "regcache.h"
-#include "gdbthread.h"
 #include "gdb_wait.h"
 #include "i386-cpuid.h"
 #include "common-regcache.h"
diff --git a/gdb/nat/linux-btrace.h b/gdb/nat/linux-btrace.h
index 28a7176..e4b2604 100644
--- a/gdb/nat/linux-btrace.h
+++ b/gdb/nat/linux-btrace.h
@@ -30,6 +30,8 @@
 #  include <linux/perf_event.h>
 #endif
 
+struct target_ops;
+
 /* Branch trace target information per thread.  */
 struct btrace_target_info
 {
-- 
1.7.1

Re: [PATCH 09/11 v5] Remove GDBSERVER uses from linux-btrace.c

[Doug Evans]

Gary Benson writes:
 > This commit makes nat/linux-btrace.c include common-defs.h rather
 > than defs.h or server.h.  A couple of minor changes were required
 > to support this change.
 > 
 > gdb/
 > 2014-08-01  Gary Benson  <gbenson@redhat.com>
 > 
 > 	* nat/linux-btrace.c: Include common-defs.h.
 > 	Don't include defs.h, server.h or gdbthread.h.
 > 	* nat/linux-btrace.h (struct target_ops): New forward declaration.

LGTM

[PATCH 07/11 v5] Introduce get_thread_regcache_for_ptid

[Gary Benson]

This introduces get_thread_regcache_for_ptid so that we can simplify
nat/linux-btrace.c.  A better long term solution would be unify the
regcache code, but this is sufficient for now.

gdb/
2014-08-01  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* common/common-regcache.h: New file.
	* Makefile.in (HFILES_NO_SRCDIR): Add common/common-regcache.h.
	* regcache.c: Include the above.
	(get_thread_regcache_for_ptid): New function.
	* nat/linux-btrace.c: Include common-regcache.h.
	(perf_event_read_bts): Use get_thread_regcache_for_ptid.

gdb/gdbserver/
2014-08-01  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* regcache.c: Include common-regcache.h.
	(get_thread_regcache_for_ptid): New function.
---
 gdb/ChangeLog                |   10 ++++++++++
 gdb/Makefile.in              |    2 +-
 gdb/common/common-regcache.h |   28 ++++++++++++++++++++++++++++
 gdb/gdbserver/ChangeLog      |    6 ++++++
 gdb/gdbserver/regcache.c     |   10 ++++++++++
 gdb/nat/linux-btrace.c       |    7 ++-----
 gdb/regcache.c               |    8 ++++++++
 7 files changed, 65 insertions(+), 6 deletions(-)
 create mode 100644 gdb/common/common-regcache.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index fea7550..c5f2854 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -936,7 +936,7 @@ 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 \
-common/common-debug.h target/target.h target/symbol.h
+common/common-debug.h target/target.h target/symbol.h common/common-regcache.h
 
 # Header files that already have srcdir in them, or which are in objdir.
 
diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
new file mode 100644
index 0000000..2ef7cb1
--- /dev/null
+++ b/gdb/common/common-regcache.h
@@ -0,0 +1,28 @@
+/* Cache and manage the values of registers
+
+   Copyright (C) 1986-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_REGCACHE_H
+#define COMMON_REGCACHE_H
+
+/* A hack until we have an independent regcache.  This must be
+   provided by the user.  */
+
+extern struct regcache *get_thread_regcache_for_ptid (ptid_t ptid);
+
+#endif /* COMMON_REGCACHE_H */
diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index db9dad9..ec4f6c7 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -21,6 +21,8 @@
 #include "gdbthread.h"
 #include "tdesc.h"
 #include "rsp-low.h"
+#include "common-regcache.h"
+
 #ifndef IN_PROCESS_AGENT
 
 struct regcache *
@@ -61,6 +63,14 @@ get_thread_regcache (struct thread_info *thread, int fetch)
   return regcache;
 }
 
+/* See common/linux-btrace.h.  */
+
+struct regcache *
+get_thread_regcache_for_ptid (ptid_t ptid)
+{
+  return get_thread_regcache (find_thread_ptid (ptid), 1);
+}
+
 void
 regcache_invalidate_thread (struct thread_info *thread)
 {
diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index f6fdbda..b32d04c 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -30,6 +30,7 @@
 #include "gdbthread.h"
 #include "gdb_wait.h"
 #include "i386-cpuid.h"
+#include "common-regcache.h"
 
 #ifdef HAVE_SYS_SYSCALL_H
 #include <sys/syscall.h>
@@ -180,11 +181,7 @@ perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
   gdb_assert (start <= end);
 
   /* The first block ends at the current pc.  */
-#ifdef GDBSERVER
-  regcache = get_thread_regcache (find_thread_ptid (tinfo->ptid), 1);
-#else
-  regcache = get_thread_regcache (tinfo->ptid);
-#endif
+  regcache = get_thread_regcache_for_ptid (tinfo->ptid);
   block.end = regcache_read_pc (regcache);
 
   /* The buffer may contain a partial record as its last entry (i.e. when the
diff --git a/gdb/regcache.c b/gdb/regcache.c
index f410dc2..2ffc121 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -28,6 +28,7 @@
 #include "exceptions.h"
 #include "remote.h"
 #include "valprint.h"
+#include "common-regcache.h"
 
 /*
  * DATA STRUCTURE
@@ -535,6 +536,13 @@ get_current_regcache (void)
   return get_thread_regcache (inferior_ptid);
 }
 
+/* See common/linux-btrace.h.  */
+
+struct regcache *
+get_thread_regcache_for_ptid (ptid_t ptid)
+{
+  return get_thread_regcache (ptid);
+}
 
 /* Observer for the target_changed event.  */
 
-- 
1.7.1

Re: [PATCH 07/11 v5] Introduce get_thread_regcache_for_ptid

[Doug Evans]

Gary Benson writes:
 > This introduces get_thread_regcache_for_ptid so that we can simplify
 > nat/linux-btrace.c.  A better long term solution would be unify the
 > regcache code, but this is sufficient for now.
 > 
 > gdb/
 > 2014-08-01  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* common/common-regcache.h: New file.
 > 	* Makefile.in (HFILES_NO_SRCDIR): Add common/common-regcache.h.
 > 	* regcache.c: Include the above.
 > 	(get_thread_regcache_for_ptid): New function.
 > 	* nat/linux-btrace.c: Include common-regcache.h.
 > 	(perf_event_read_bts): Use get_thread_regcache_for_ptid.
 > 
 > gdb/gdbserver/
 > 2014-08-01  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* regcache.c: Include common-regcache.h.
 > 	(get_thread_regcache_for_ptid): New function.
 > [...]
 > diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
 > new file mode 100644
 > index 0000000..2ef7cb1
 > --- /dev/null
 > +++ b/gdb/common/common-regcache.h
 > @@ -0,0 +1,28 @@
 > +/* Cache and manage the values of registers
 > +
 > +   Copyright (C) 1986-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_REGCACHE_H
 > +#define COMMON_REGCACHE_H
 > +
 > +/* A hack until we have an independent regcache.  This must be
 > +   provided by the user.  */

This should still say what the function does,
what the inputs/outputs are, etc.

 > +
 > +extern struct regcache *get_thread_regcache_for_ptid (ptid_t ptid);
 > +
 > +#endif /* COMMON_REGCACHE_H */
 > diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
 > index db9dad9..ec4f6c7 100644
 > --- a/gdb/gdbserver/regcache.c
 > +++ b/gdb/gdbserver/regcache.c
 > @@ -21,6 +21,8 @@
 >  #include "gdbthread.h"
 >  #include "tdesc.h"
 >  #include "rsp-low.h"
 > +#include "common-regcache.h"
 > +
 >  #ifndef IN_PROCESS_AGENT
 >  
 >  struct regcache *
 > @@ -61,6 +63,14 @@ get_thread_regcache (struct thread_info *thread, int fetch)
 >    return regcache;
 >  }
 >  
 > +/* See common/linux-btrace.h.  */

It's nat/linux-btrace.h, but I think you mean common/common-regcache.h.
[setting aside the #include "common/regcache.h" discussion]

 > +
 > +struct regcache *
 > +get_thread_regcache_for_ptid (ptid_t ptid)
 > +{
 > +  return get_thread_regcache (find_thread_ptid (ptid), 1);
 > +}
 > +
 >  void
 >  regcache_invalidate_thread (struct thread_info *thread)
 >  {
 > diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
 > index f6fdbda..b32d04c 100644
 > --- a/gdb/nat/linux-btrace.c
 > +++ b/gdb/nat/linux-btrace.c
 > @@ -30,6 +30,7 @@
 >  #include "gdbthread.h"
 >  #include "gdb_wait.h"
 >  #include "i386-cpuid.h"
 > +#include "common-regcache.h"
 >  
 >  #ifdef HAVE_SYS_SYSCALL_H
 >  #include <sys/syscall.h>
 > @@ -180,11 +181,7 @@ perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
 >    gdb_assert (start <= end);
 >  
 >    /* The first block ends at the current pc.  */
 > -#ifdef GDBSERVER
 > -  regcache = get_thread_regcache (find_thread_ptid (tinfo->ptid), 1);
 > -#else
 > -  regcache = get_thread_regcache (tinfo->ptid);
 > -#endif
 > +  regcache = get_thread_regcache_for_ptid (tinfo->ptid);
 >    block.end = regcache_read_pc (regcache);
 >  
 >    /* The buffer may contain a partial record as its last entry (i.e. when the
 > diff --git a/gdb/regcache.c b/gdb/regcache.c
 > index f410dc2..2ffc121 100644
 > --- a/gdb/regcache.c
 > +++ b/gdb/regcache.c
 > @@ -28,6 +28,7 @@
 >  #include "exceptions.h"
 >  #include "remote.h"
 >  #include "valprint.h"
 > +#include "common-regcache.h"
 >  
 >  /*
 >   * DATA STRUCTURE
 > @@ -535,6 +536,13 @@ get_current_regcache (void)
 >    return get_thread_regcache (inferior_ptid);
 >  }
 >  
 > +/* See common/linux-btrace.h.  */

Similarly, common/common-regcache.h.

 > +
 > +struct regcache *
 > +get_thread_regcache_for_ptid (ptid_t ptid)
 > +{
 > +  return get_thread_regcache (ptid);
 > +}
 >  
 >  /* Observer for the target_changed event.  */
 >  
 > -- 
 > 1.7.1
 > 

Ok with those nits fixed.

[PATCH 10/11 v5] Remove GDBSERVER uses from i386-dregs.c

[Gary Benson]

This commit makes nat/i386-dregs.c include common-defs.h rather than
defs.h or server.h.  A couple of minor changes were required in order
to support this change.

gdb/
2014-08-01  Gary Benson  <gbenson@redhat.com>

	* nat/i386-dregs.c: Include common-defs.h and break-common.h.
	Don't include defs.h or server.h.
	(debug_hw_points): Declare unconditionally.
---
 gdb/ChangeLog        |    6 ++++++
 gdb/nat/i386-dregs.c |   10 ++--------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdb/nat/i386-dregs.c b/gdb/nat/i386-dregs.c
index e3272cd..7905c44 100644
--- a/gdb/nat/i386-dregs.c
+++ b/gdb/nat/i386-dregs.c
@@ -17,13 +17,9 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#include "inferior.h"
-#endif
+#include "common-defs.h"
 #include "i386-dregs.h"
+#include "break-common.h"
 
 /* Support for hardware watchpoints and breakpoints using the i386
    debug registers.
@@ -175,10 +171,8 @@
 /* Types of operations supported by i386_handle_nonaligned_watchpoint.  */
 typedef enum { WP_INSERT, WP_REMOVE, WP_COUNT } i386_wp_op_t;
 
-#ifndef GDBSERVER
 /* Whether or not to print the mirrored debug registers.  */
 extern int debug_hw_points;
-#endif
 
 /* Print the values of the mirrored debug registers.  */
 
-- 
1.7.1

Re: [PATCH 10/11 v5] Remove GDBSERVER uses from i386-dregs.c

[Doug Evans]

Gary Benson writes:
 > This commit makes nat/i386-dregs.c include common-defs.h rather than
 > defs.h or server.h.  A couple of minor changes were required in order
 > to support this change.
 > 
 > gdb/
 > 2014-08-01  Gary Benson  <gbenson@redhat.com>
 > 
 > 	* nat/i386-dregs.c: Include common-defs.h and break-common.h.
 > 	Don't include defs.h or server.h.
 > 	(debug_hw_points): Declare unconditionally.
 > ---
 >  gdb/ChangeLog        |    6 ++++++
 >  gdb/nat/i386-dregs.c |   10 ++--------
 >  2 files changed, 8 insertions(+), 8 deletions(-)
 > 
 > diff --git a/gdb/nat/i386-dregs.c b/gdb/nat/i386-dregs.c
 > index e3272cd..7905c44 100644
 > --- a/gdb/nat/i386-dregs.c
 > +++ b/gdb/nat/i386-dregs.c
 > @@ -17,13 +17,9 @@
 >     You should have received a copy of the GNU General Public License
 >     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 >  
 > -#ifdef GDBSERVER
 > -#include "server.h"
 > -#else
 > -#include "defs.h"
 > -#include "inferior.h"
 > -#endif
 > +#include "common-defs.h"
 >  #include "i386-dregs.h"
 > +#include "break-common.h"
 >  
 >  /* Support for hardware watchpoints and breakpoints using the i386
 >     debug registers.
 > @@ -175,10 +171,8 @@
 >  /* Types of operations supported by i386_handle_nonaligned_watchpoint.  */
 >  typedef enum { WP_INSERT, WP_REMOVE, WP_COUNT } i386_wp_op_t;
 >  
 > -#ifndef GDBSERVER
 >  /* Whether or not to print the mirrored debug registers.  */
 >  extern int debug_hw_points;
 > -#endif

Since externs should live in headers, and since this is a cleanup patch,
can I impose on you a request to move debug_hw_points to a header.
Seems like i386-dregs.h is the best place.

The variable could even arguably be defined in i386-dregs.c,
though I'm not sure if/how people would want i386-nat.c to change.
Plus add x86 or some such to the variable's name.
I'd leave this part to another pass.

 >  
 >  /* Print the values of the mirrored debug registers.  */
 >  
 > -- 
 > 1.7.1
 > 

Re: [PATCH 10/11 v5] Remove GDBSERVER uses from i386-dregs.c

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
> > -#ifndef GDBSERVER
> >  /* Whether or not to print the mirrored debug registers.  */
> >  extern int debug_hw_points;
> > -#endif
> 
> Since externs should live in headers, and since this is a cleanup
> patch, can I impose on you a request to move debug_hw_points to a
> header.  Seems like i386-dregs.h is the best place.
> 
> The variable could even arguably be defined in i386-dregs.c, though
> I'm not sure if/how people would want i386-nat.c to change.  Plus
> add x86 or some such to the variable's name.  I'd leave this part to
> another pass.

i386-dregs doesn't work for debug_hw_points, as it's also used by
the aarch64-linux code.  It's handled fairly messily at present
(it's global in gdbserver, in server.[ch], but local to some ports
in GDB) which I guess I why I left it why it was when I did the
i386-dregs work.

Since it's global in gdbserver it probably should just be global.
I'll move the extern to common-debug.h, and the definition to
common-debug.c.

Thanks,
Gary

-- 
http://gbenson.net/

[PATCH 08/11 v5] Include common-defs.h instead of defs.h/server.h in shared code

[Gary Benson]

This commit makes 19 of the 22 shared .c files in common, nat and
target include common-defs.h instead of defs.h/server.h.  The
remaining three files need slight extra work and are dealt with
in separate commits.

gdb/
2014-08-01  Gary Benson  <gbenson@redhat.com>

	* common/agent.c: Include common-defs.h.
	Don't include defs.h or server.h.
	* common/buffer.c: Likewise.
	* common/common-debug.c: Likewise.
	* common/common-utils.c: Likewise.
	* common/errors.c: Likewise.
	* common/filestuff.c: Likewise.
	* common/format.c: Likewise.
	* common/gdb_vecs.c: Likewise.
	* common/print-utils.c: Likewise.
	* common/ptid.c: Likewise.
	* common/rsp-low.c: Likewise.
	* common/signals.c: Likewise.
	* common/vec.c: Likewise.
	* common/xml-utils.c: Likewise.
	* nat/linux-osdata.c: Likewise.
	* nat/linux-procfs.c: Likewise.
	* nat/linux-ptrace.c: Likewise.
	* nat/mips-linux-watch.c: Likewise.
	* target/waitstatus.c: Likewise.
---
 gdb/ChangeLog              |   23 +++++++++++++++++++++++
 gdb/common/agent.c         |    6 +-----
 gdb/common/buffer.c        |    7 +------
 gdb/common/common-debug.c  |    6 +-----
 gdb/common/common-utils.c  |    6 +-----
 gdb/common/errors.c        |    6 +-----
 gdb/common/filestuff.c     |    6 +-----
 gdb/common/format.c        |    7 +------
 gdb/common/gdb_vecs.c      |    7 +------
 gdb/common/print-utils.c   |    7 +------
 gdb/common/ptid.c          |    6 +-----
 gdb/common/rsp-low.c       |    7 +------
 gdb/common/signals.c       |    6 +-----
 gdb/common/vec.c           |    7 +------
 gdb/common/xml-utils.c     |    7 +------
 gdb/nat/linux-osdata.c     |    7 +------
 gdb/nat/linux-procfs.c     |    7 +------
 gdb/nat/linux-ptrace.c     |    7 +------
 gdb/nat/mips-linux-watch.c |    6 +-----
 gdb/target/waitstatus.c    |    7 +------
 20 files changed, 42 insertions(+), 106 deletions(-)

diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index d68e50e..8b90c4d 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -17,11 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
+#include "common-defs.h"
 #include "target/target.h"
 #include "target/symbol.h"
 #include <unistd.h>
diff --git a/gdb/common/buffer.c b/gdb/common/buffer.c
index 4a213b3..d6afb6a 100644
--- a/gdb/common/buffer.c
+++ b/gdb/common/buffer.c
@@ -17,12 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
-
+#include "common-defs.h"
 #include "xml-utils.h"
 #include "buffer.h"
 #include "inttypes.h"
diff --git a/gdb/common/common-debug.c b/gdb/common/common-debug.c
index 660fc70..22f6a8a 100644
--- a/gdb/common/common-debug.c
+++ b/gdb/common/common-debug.c
@@ -17,11 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
+#include "common-defs.h"
 #include "common-debug.h"
 
 /* See common/common-debug.h.  */
diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index a905d1d..3b8237e 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -17,11 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
+#include "common-defs.h"
 
 /* The xmalloc() (libiberty.h) family of memory management routines.
 
diff --git a/gdb/common/errors.c b/gdb/common/errors.c
index 7d0bb6e..6da2666 100644
--- a/gdb/common/errors.c
+++ b/gdb/common/errors.c
@@ -17,11 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
+#include "common-defs.h"
 #include "errors.h"
 
 /* See common/errors.h.  */
diff --git a/gdb/common/filestuff.c b/gdb/common/filestuff.c
index a31ecd7..7ee9c5a 100644
--- a/gdb/common/filestuff.c
+++ b/gdb/common/filestuff.c
@@ -16,11 +16,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
+#include "common-defs.h"
 #include "filestuff.h"
 #include "gdb_vecs.h"
 #include <fcntl.h>
diff --git a/gdb/common/format.c b/gdb/common/format.c
index 247aaff..b989dc7 100644
--- a/gdb/common/format.c
+++ b/gdb/common/format.c
@@ -17,12 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
-
+#include "common-defs.h"
 #include "format.h"
 
 struct format_piece *
diff --git a/gdb/common/gdb_vecs.c b/gdb/common/gdb_vecs.c
index 4a3330f..ae11cc6 100644
--- a/gdb/common/gdb_vecs.c
+++ b/gdb/common/gdb_vecs.c
@@ -17,12 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
-
+#include "common-defs.h"
 #include "gdb_vecs.h"
 #include "host-defs.h"
 
diff --git a/gdb/common/print-utils.c b/gdb/common/print-utils.c
index f5bef0a..820ade0 100644
--- a/gdb/common/print-utils.c
+++ b/gdb/common/print-utils.c
@@ -17,12 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
-
+#include "common-defs.h"
 #include "print-utils.h"
 #include <stdint.h>
 
diff --git a/gdb/common/ptid.c b/gdb/common/ptid.c
index 04fd118..84e4aa7 100644
--- a/gdb/common/ptid.c
+++ b/gdb/common/ptid.c
@@ -17,11 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
+#include "common-defs.h"
 #include "ptid.h"
 
 /* See ptid.h for these.  */
diff --git a/gdb/common/rsp-low.c b/gdb/common/rsp-low.c
index 0263005..e88799a 100644
--- a/gdb/common/rsp-low.c
+++ b/gdb/common/rsp-low.c
@@ -17,12 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
-
+#include "common-defs.h"
 #include "rsp-low.h"
 
 /* See rsp-low.h.  */
diff --git a/gdb/common/signals.c b/gdb/common/signals.c
index 13d1e2c..ebe2761 100644
--- a/gdb/common/signals.c
+++ b/gdb/common/signals.c
@@ -17,11 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
+#include "common-defs.h"
 
 #ifdef HAVE_SIGNAL_H
 #include <signal.h>
diff --git a/gdb/common/vec.c b/gdb/common/vec.c
index 4611e5f..9fc6915 100644
--- a/gdb/common/vec.c
+++ b/gdb/common/vec.c
@@ -17,12 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
-
+#include "common-defs.h"
 #include "vec.h"
 
 struct vec_prefix
diff --git a/gdb/common/xml-utils.c b/gdb/common/xml-utils.c
index 0f81390..b90dd21 100644
--- a/gdb/common/xml-utils.c
+++ b/gdb/common/xml-utils.c
@@ -17,12 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
-
+#include "common-defs.h"
 #include "xml-utils.h"
 
 /* Return a malloc allocated string with special characters from TEXT
diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
index 887e518..3f72883 100644
--- a/gdb/nat/linux-osdata.c
+++ b/gdb/nat/linux-osdata.c
@@ -17,12 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
-
+#include "common-defs.h"
 #include "linux-osdata.h"
 
 #include <sys/types.h>
diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
index 84fc890..30797da 100644
--- a/gdb/nat/linux-procfs.c
+++ b/gdb/nat/linux-procfs.c
@@ -16,12 +16,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
-
+#include "common-defs.h"
 #include "linux-procfs.h"
 #include "filestuff.h"
 
diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
index b4db862..6275516 100644
--- a/gdb/nat/linux-ptrace.c
+++ b/gdb/nat/linux-ptrace.c
@@ -16,12 +16,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
-
+#include "common-defs.h"
 #include "linux-ptrace.h"
 #include "linux-procfs.h"
 #include "linux-waitpid.h"
diff --git a/gdb/nat/mips-linux-watch.c b/gdb/nat/mips-linux-watch.c
index ea6e02d..afa3d78 100644
--- a/gdb/nat/mips-linux-watch.c
+++ b/gdb/nat/mips-linux-watch.c
@@ -15,11 +15,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
+#include "common-defs.h"
 #include <sys/ptrace.h>
 #include "mips-linux-watch.h"
 
diff --git a/gdb/target/waitstatus.c b/gdb/target/waitstatus.c
index 4493555..717f47a 100644
--- a/gdb/target/waitstatus.c
+++ b/gdb/target/waitstatus.c
@@ -17,12 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
-
+#include "common-defs.h"
 #include "waitstatus.h"
 
 /* Return a pretty printed form of target_waitstatus.
-- 
1.7.1

Re: [PATCH 08/11 v5] Include common-defs.h instead of defs.h/server.h in shared code

[Doug Evans]

Gary Benson writes:
 > This commit makes 19 of the 22 shared .c files in common, nat and
 > target include common-defs.h instead of defs.h/server.h.  The
 > remaining three files need slight extra work and are dealt with
 > in separate commits.
 > 
 > gdb/
 > 2014-08-01  Gary Benson  <gbenson@redhat.com>
 > 
 > 	* common/agent.c: Include common-defs.h.
 > 	Don't include defs.h or server.h.
 > 	* common/buffer.c: Likewise.
 > 	* common/common-debug.c: Likewise.
 > 	* common/common-utils.c: Likewise.
 > 	* common/errors.c: Likewise.
 > 	* common/filestuff.c: Likewise.
 > 	* common/format.c: Likewise.
 > 	* common/gdb_vecs.c: Likewise.
 > 	* common/print-utils.c: Likewise.
 > 	* common/ptid.c: Likewise.
 > 	* common/rsp-low.c: Likewise.
 > 	* common/signals.c: Likewise.
 > 	* common/vec.c: Likewise.
 > 	* common/xml-utils.c: Likewise.
 > 	* nat/linux-osdata.c: Likewise.
 > 	* nat/linux-procfs.c: Likewise.
 > 	* nat/linux-ptrace.c: Likewise.
 > 	* nat/mips-linux-watch.c: Likewise.
 > 	* target/waitstatus.c: Likewise.

LGTM

[PATCH 06/11 v5] Add target/symbol.h

[Gary Benson]

This adds target/symbol.h.  This file declares a function that the
shared code can use and that the clients must implement.  It also
changes some shared code to use these functions.

gdb/
2014-08-01  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* target/symbol.h: New file.
	* Makefile.in (HFILES_NO_SRCDIR): Add target/symbol.h.
	* target.h: Include target/symbol.h.
	* target.c (target_look_up_symbol): New function.
	* common/agent.c: Include target/symbol.h.
	[!GDBSERVER]: Don't include objfiles.h.
	(agent_look_up_symbols): Use target_look_up_symbol.

gdb/gdbserver/
2014-08-01  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* target.c: Include target/symbol.h.
	(target_look_up_symbol): New function.
---
 gdb/ChangeLog           |   11 +++++++++++
 gdb/Makefile.in         |    2 +-
 gdb/common/agent.c      |   14 ++------------
 gdb/gdbserver/ChangeLog |    6 ++++++
 gdb/gdbserver/target.c  |   12 ++++++++++++
 gdb/target.c            |   14 ++++++++++++++
 gdb/target.h            |    1 +
 gdb/target/symbol.h     |   36 ++++++++++++++++++++++++++++++++++++
 8 files changed, 83 insertions(+), 13 deletions(-)
 create mode 100644 gdb/target/symbol.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 090c912..fea7550 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -936,7 +936,7 @@ 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 \
-common/common-debug.h target/target.h
+common/common-debug.h target/target.h target/symbol.h
 
 # Header files that already have srcdir in them, or which are in objdir.
 
diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index 5c19454..d68e50e 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -21,9 +21,9 @@
 #include "server.h"
 #else
 #include "defs.h"
-#include "objfiles.h"
 #endif
 #include "target/target.h"
+#include "target/symbol.h"
 #include <unistd.h>
 #include "agent.h"
 #include "filestuff.h"
@@ -98,18 +98,8 @@ agent_look_up_symbols (void *arg)
     {
       CORE_ADDR *addrp =
 	(CORE_ADDR *) ((char *) &ipa_sym_addrs + symbol_list[i].offset);
-#ifdef GDBSERVER
-
-      if (look_up_one_symbol (symbol_list[i].name, addrp, 1) == 0)
-#else
-      struct bound_minimal_symbol sym =
-	lookup_minimal_symbol (symbol_list[i].name, NULL,
-			       (struct objfile *) arg);
 
-      if (sym.minsym != NULL)
-	*addrp = BMSYMBOL_VALUE_ADDRESS (sym);
-      else
-#endif
+      if (target_look_up_symbol (symbol_list[i].name, addrp, arg) == 0)
 	{
 	  DEBUG_AGENT ("symbol `%s' not found\n", symbol_list[i].name);
 	  return -1;
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index 6ba375c..ad249a0 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -20,6 +20,7 @@
 
 #include "server.h"
 #include "tracepoint.h"
+#include "target/symbol.h"
 
 struct target_ops *the_target;
 
@@ -168,6 +169,17 @@ target_resume (ptid_t ptid, int step, enum gdb_signal signal)
   (*the_target->resume) (&resume_info, 1);
 }
 
+/* See target/symbol.h.  */
+
+int
+target_look_up_symbol (const char *name, CORE_ADDR *addr,
+		       struct objfile *objfile)
+{
+  gdb_assert (objfile == NULL);
+
+  return look_up_one_symbol (name, addr, 1);
+}
+
 int
 start_non_stop (int nonstop)
 {
diff --git a/gdb/target.c b/gdb/target.c
index 1773eff..fd5a806 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1292,6 +1292,20 @@ target_read_uint32 (CORE_ADDR memaddr, unsigned int *result)
   return 0;
 }
 
+/* See target/symbol.h.  */
+
+int
+target_look_up_symbol (const char *name, CORE_ADDR *addr,
+		       struct objfile *objfile)
+{
+  struct bound_minimal_symbol sym
+    = lookup_minimal_symbol (name, NULL, objfile);
+
+  if (sym.minsym != NULL)
+    *addr = BMSYMBOL_VALUE_ADDRESS (sym);
+  return sym.minsym != NULL;
+}
+
 /* Like target_read_memory, but specify explicitly that this is a read
    from the target's raw memory.  That is, this read bypasses the
    dcache, breakpoint shadowing, etc.  */
diff --git a/gdb/target.h b/gdb/target.h
index 1a10744..f16c84c 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -62,6 +62,7 @@ struct dcache_struct;
 #include "target/resume.h"
 #include "target/wait.h"
 #include "target/waitstatus.h"
+#include "target/symbol.h"
 #include "bfd.h"
 #include "symtab.h"
 #include "memattr.h"
diff --git a/gdb/target/symbol.h b/gdb/target/symbol.h
new file mode 100644
index 0000000..bb37b72
--- /dev/null
+++ b/gdb/target/symbol.h
@@ -0,0 +1,36 @@
+/* Declarations of target symbol functions.
+
+   Copyright (C) 1986-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 TARGET_SYMBOL_H
+#define TARGET_SYMBOL_H
+
+struct objfile;
+
+/* Find a symbol that matches NAME.  Limit the search to OBJFILE if
+   OBJFILE is non-NULL and the implementation supports limiting the
+   search to specific object files.  If a match is found, store the
+   matching symbol's address in ADDR and return nonzero.  Return zero
+   if no symbol matching NAME is found.  Raise an exception if OBJFILE
+   is non-NULL and the implementation does not support limiting
+   searches to specific object files.  */
+
+extern int target_look_up_symbol (const char *name, CORE_ADDR *addr,
+				  struct objfile *objfile);
+
+#endif /* TARGET_SYMBOL_H */
-- 
1.7.1

Re: [PATCH 06/11 v5] Add target/symbol.h

[Doug Evans]

Gary Benson writes:
 > This adds target/symbol.h.  This file declares a function that the
 > shared code can use and that the clients must implement.  It also
 > changes some shared code to use these functions.
 > 
 > gdb/
 > 2014-08-01  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* target/symbol.h: New file.
 > 	* Makefile.in (HFILES_NO_SRCDIR): Add target/symbol.h.
 > 	* target.h: Include target/symbol.h.
 > 	* target.c (target_look_up_symbol): New function.
 > 	* common/agent.c: Include target/symbol.h.
 > 	[!GDBSERVER]: Don't include objfiles.h.
 > 	(agent_look_up_symbols): Use target_look_up_symbol.
 > 
 > gdb/gdbserver/
 > 2014-08-01  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* target.c: Include target/symbol.h.
 > 	(target_look_up_symbol): New function.
 > [...]
 > diff --git a/gdb/target/symbol.h b/gdb/target/symbol.h
 > new file mode 100644
 > index 0000000..bb37b72
 > --- /dev/null
 > +++ b/gdb/target/symbol.h
 > @@ -0,0 +1,36 @@
 > +/* Declarations of target symbol functions.
 > +
 > +   Copyright (C) 1986-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 TARGET_SYMBOL_H
 > +#define TARGET_SYMBOL_H
 > +
 > +struct objfile;
 > +
 > +/* Find a symbol that matches NAME.  Limit the search to OBJFILE if
 > +   OBJFILE is non-NULL and the implementation supports limiting the
 > +   search to specific object files.  If a match is found, store the
 > +   matching symbol's address in ADDR and return nonzero.  Return zero
 > +   if no symbol matching NAME is found.  Raise an exception if OBJFILE
 > +   is non-NULL and the implementation does not support limiting
 > +   searches to specific object files.  */
 > +
 > +extern int target_look_up_symbol (const char *name, CORE_ADDR *addr,
 > +				  struct objfile *objfile);
 > +
 > +#endif /* TARGET_SYMBOL_H */
 > -- 
 > 1.7.1

Can this comment spell out that either a mangled or demangled
form of the symbol is allowed for NAME?
[assuming that that is indeed the case]

Also, the target/target.h memory routines return zero for success
and a non-zero error code for failure.  E.g.,

+/* Read LEN bytes of target memory at address MEMADDR, placing the
+   results in GDB's memory at MYADDR.  Return zero for success,
+   nonzero if any error occurs.  Implementations of this function may
+   define and use their own error codes, but functions in the common,
+   nat and target directories must treat the return code as opaque.
+   No guarantee is made about the contents of the data at MYADDR if
+   any error occurs.  */
+
+extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
+			       ssize_t len);

Do we want to be consistent here, and have the same results for
target_look_up_symbol? [and throughout the target API]

Re: [PATCH 06/11 v5] Add target/symbol.h

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
> > +/* Find a symbol that matches NAME.  Limit the search to OBJFILE if
> > +   OBJFILE is non-NULL and the implementation supports limiting the
> > +   search to specific object files.  If a match is found, store the
> > +   matching symbol's address in ADDR and return nonzero.  Return zero
> > +   if no symbol matching NAME is found.  Raise an exception if OBJFILE
> > +   is non-NULL and the implementation does not support limiting
> > +   searches to specific object files.  */
> > +
> > +extern int target_look_up_symbol (const char *name, CORE_ADDR *addr,
> > +				  struct objfile *objfile);
> 
> Can this comment spell out that either a mangled or demangled
> form of the symbol is allowed for NAME?
> [assuming that that is indeed the case]

It is the case.  I'll update the comment.

> Also, the target/target.h memory routines return zero for success
> and a non-zero error code for failure.  E.g.,
> 
> +/* Read LEN bytes of target memory at address MEMADDR, placing the
> +   results in GDB's memory at MYADDR.  Return zero for success,
> +   nonzero if any error occurs.  Implementations of this function may
> +   define and use their own error codes, but functions in the common,
> +   nat and target directories must treat the return code as opaque.
> +   No guarantee is made about the contents of the data at MYADDR if
> +   any error occurs.  */
> +
> +extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
> +			       ssize_t len);
> 
> Do we want to be consistent here, and have the same results for
> target_look_up_symbol? [and throughout the target API]

That seems reasonable to me, I'll make the change.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 06/11 v5] Add target/symbol.h

[Pedro Alves]

On 08/01/2014 11:19 AM, Gary Benson wrote:
> This adds target/symbol.h.  This file declares a function that the
> shared code can use and that the clients must implement.  It also
> changes some shared code to use these functions.

A small parens:

I have to say that calling this new method target_foo looks kind of
awkward to me.  Unlike other target methods and helpers, that extract
info out of the target or tell the target to do something,
this goes in the other direction -- this is the target/backend/server
calling back to the client/symbol side for something.  Put another way,
seems like this method would never ultimately go through target_ops.

Thanks,
Pedro Alves

Re: [PATCH 06/11 v5] Add target/symbol.h

[Gary Benson]

Pedro Alves wrote:
> On 08/01/2014 11:19 AM, Gary Benson wrote:
> > This adds target/symbol.h.  This file declares a function that the
> > shared code can use and that the clients must implement.  It also
> > changes some shared code to use these functions.
> 
> A small parens:
> 
> I have to say that calling this new method target_foo looks kind
> of awkward to me.  Unlike other target methods and helpers, that
> extract info out of the target or tell the target to do something,
> this goes in the other direction -- this is the target/backend/
> server calling back to the client/symbol side for something.  Put
> another way, seems like this method would never ultimately go
> through target_ops.

Can you suggest a more suitable name?

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 06/11 v5] Add target/symbol.h

[Pedro Alves]

On 08/20/2014 01:14 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 08/01/2014 11:19 AM, Gary Benson wrote:
>>> This adds target/symbol.h.  This file declares a function that the
>>> shared code can use and that the clients must implement.  It also
>>> changes some shared code to use these functions.
>>
>> A small parens:
>>
>> I have to say that calling this new method target_foo looks kind
>> of awkward to me.  Unlike other target methods and helpers, that
>> extract info out of the target or tell the target to do something,
>> this goes in the other direction -- this is the target/backend/
>> server calling back to the client/symbol side for something.  Put
>> another way, seems like this method would never ultimately go
>> through target_ops.
> 
> Can you suggest a more suitable name?

find_minimal_symbol_address ?

I'd suggest moving this out of target.c too.  E.g., to a
new gdbserver/symbol.c or some such.

-- 
Thanks,
Pedro Alves

[PATCH 05/11 v5] Add target/target.h

[Gary Benson]

This adds target/target.h.  This file declares some functions that
the shared code can use and that the clients must implement.  It
also changes some shared code to use these functions.

gdb/
2014-08-01  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* target/target.h: New file.
	* Makefile.in (HFILES_NO_SRCDIR): Add target/target.h.
	* target.h: Include target/target.h.
	(target_resume, target_wait, target_stop, target_read_memory)
	(target_write_memory, non_stop): Don't declare.
	* target.c (target_read_uint32): New function.
	* common/agent.c: Include target/target.h.
	[!GDBSERVER]: Don't include target.h or infrun.h.
	(agent_get_helper_thread_id): Use target_read_uint32.
	(agent_run_command): Always use target_resume, target_stop,
	target_wait, target_write_memory, and target_read_memory.
	(agent_capability_check): Use target_read_uint32.

gdb/gdbserver/
2014-08-01  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* target.c (target_wait, target_stop, target_resume)
	(target_read_memory, target_read_uint32)
	(target_write_memory): New functions.
---
 gdb/ChangeLog           |   16 ++++++++
 gdb/Makefile.in         |    2 +-
 gdb/common/agent.c      |   76 +++---------------------------------
 gdb/gdbserver/ChangeLog |    7 +++
 gdb/gdbserver/target.c  |   58 ++++++++++++++++++++++++++++
 gdb/target.c            |   16 ++++++++
 gdb/target.h            |   38 +------------------
 gdb/target/target.h     |   98 +++++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 204 insertions(+), 107 deletions(-)
 create mode 100644 gdb/target/target.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 8429ebc..090c912 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -936,7 +936,7 @@ 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 \
-common/common-debug.h
+common/common-debug.h target/target.h
 
 # Header files that already have srcdir in them, or which are in objdir.
 
diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index 7071ce6..5c19454 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -21,10 +21,9 @@
 #include "server.h"
 #else
 #include "defs.h"
-#include "target.h"
-#include "infrun.h"
 #include "objfiles.h"
 #endif
+#include "target/target.h"
 #include <unistd.h>
 #include "agent.h"
 #include "filestuff.h"
@@ -126,23 +125,9 @@ agent_get_helper_thread_id (void)
 {
   if  (helper_thread_id == 0)
     {
-#ifdef GDBSERVER
-      if (read_inferior_memory (ipa_sym_addrs.addr_helper_thread_id,
-				(unsigned char *) &helper_thread_id,
-				sizeof helper_thread_id))
-#else
-      enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch ());
-      gdb_byte buf[4];
-
-      if (target_read_memory (ipa_sym_addrs.addr_helper_thread_id,
-			      buf, sizeof buf) == 0)
-	helper_thread_id = extract_unsigned_integer (buf, sizeof buf,
-						     byte_order);
-      else
-#endif
-	{
-	  warning (_("Error reading helper thread's id in lib"));
-	}
+      if (target_read_uint32 (ipa_sym_addrs.addr_helper_thread_id,
+			      &helper_thread_id))
+	warning (_("Error reading helper thread's id in lib"));
     }
 
   return helper_thread_id;
@@ -220,13 +205,8 @@ agent_run_command (int pid, const char *cmd, int len)
   int tid = agent_get_helper_thread_id ();
   ptid_t ptid = ptid_build (pid, tid, 0);
 
-#ifdef GDBSERVER
-  int ret = write_inferior_memory (ipa_sym_addrs.addr_cmd_buf,
-				   (const unsigned char *) cmd, len);
-#else
   int ret = target_write_memory (ipa_sym_addrs.addr_cmd_buf,
 				 (gdb_byte *) cmd, len);
-#endif
 
   if (ret != 0)
     {
@@ -237,18 +217,7 @@ agent_run_command (int pid, const char *cmd, int len)
   DEBUG_AGENT ("agent: resumed helper thread\n");
 
   /* Resume helper thread.  */
-#ifdef GDBSERVER
-{
-  struct thread_resume resume_info;
-
-  resume_info.thread = ptid;
-  resume_info.kind = resume_continue;
-  resume_info.sig = GDB_SIGNAL_0;
-  (*the_target->resume) (&resume_info, 1);
-}
-#else
- target_resume (ptid, 0, GDB_SIGNAL_0);
-#endif
+  target_resume (ptid, 0, GDB_SIGNAL_0);
 
   fd = gdb_connect_sync_socket (pid);
   if (fd >= 0)
@@ -284,37 +253,18 @@ agent_run_command (int pid, const char *cmd, int len)
       int was_non_stop = non_stop;
       /* Stop thread PTID.  */
       DEBUG_AGENT ("agent: stop helper thread\n");
-#ifdef GDBSERVER
-      {
-	struct thread_resume resume_info;
-
-	resume_info.thread = ptid;
-	resume_info.kind = resume_stop;
-	resume_info.sig = GDB_SIGNAL_0;
-	(*the_target->resume) (&resume_info, 1);
-      }
-
-      non_stop = 1;
-      mywait (ptid, &status, 0, 0);
-#else
       non_stop = 1;
       target_stop (ptid);
 
       memset (&status, 0, sizeof (status));
       target_wait (ptid, &status, 0);
-#endif
       non_stop = was_non_stop;
     }
 
   if (fd >= 0)
     {
-#ifdef GDBSERVER
-      if (read_inferior_memory (ipa_sym_addrs.addr_cmd_buf,
-				(unsigned char *) cmd, IPA_CMD_BUF_SIZE))
-#else
       if (target_read_memory (ipa_sym_addrs.addr_cmd_buf, (gdb_byte *) cmd,
 			      IPA_CMD_BUF_SIZE))
-#endif
 	{
 	  warning (_("Error reading command response"));
 	  return -1;
@@ -334,20 +284,8 @@ agent_capability_check (enum agent_capa agent_capa)
 {
   if (agent_capability == 0)
     {
-#ifdef GDBSERVER
-      if (read_inferior_memory (ipa_sym_addrs.addr_capability,
-				(unsigned char *) &agent_capability,
-				sizeof agent_capability))
-#else
-      enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch ());
-      gdb_byte buf[4];
-
-      if (target_read_memory (ipa_sym_addrs.addr_capability,
-			      buf, sizeof buf) == 0)
-	agent_capability = extract_unsigned_integer (buf, sizeof buf,
-						     byte_order);
-      else
-#endif
+      if (target_read_uint32 (ipa_sym_addrs.addr_capability,
+			      &agent_capability))
 	warning (_("Error reading capability of agent"));
     }
   return agent_capability & agent_capa;
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index dcad5c9..6ba375c 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -48,6 +48,22 @@ read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
   return res;
 }
 
+/* See target/target.h.  */
+
+int
+target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
+{
+  return read_inferior_memory (memaddr, myaddr, len);
+}
+
+/* See target/target.h.  */
+
+int
+target_read_uint32 (CORE_ADDR memaddr, unsigned int *result)
+{
+  return read_inferior_memory (memaddr, (gdb_byte *) result, sizeof (*result));
+}
+
 int
 write_inferior_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
 		       int len)
@@ -71,6 +87,14 @@ write_inferior_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
   return res;
 }
 
+/* See target/target.h.  */
+
+int
+target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
+{
+  return write_inferior_memory (memaddr, myaddr, len);
+}
+
 ptid_t
 mywait (ptid_t ptid, struct target_waitstatus *ourstatus, int options,
 	int connected_wait)
@@ -110,6 +134,40 @@ mywait (ptid_t ptid, struct target_waitstatus *ourstatus, int options,
   return ret;
 }
 
+/* See target/target.h.  */
+
+ptid_t
+target_wait (ptid_t ptid, struct target_waitstatus *status, int options)
+{
+  return mywait (ptid, status, options, 0);
+}
+
+/* See target/target.h.  */
+
+void
+target_stop (ptid_t ptid)
+{
+  struct thread_resume resume_info;
+
+  resume_info.thread = ptid;
+  resume_info.kind = resume_stop;
+  resume_info.sig = GDB_SIGNAL_0;
+  (*the_target->resume) (&resume_info, 1);
+}
+
+/* See target/target.h.  */
+
+void
+target_resume (ptid_t ptid, int step, enum gdb_signal signal)
+{
+  struct thread_resume resume_info;
+
+  resume_info.thread = ptid;
+  resume_info.kind = step ? resume_step : resume_continue;
+  resume_info.sig = signal;
+  (*the_target->resume) (&resume_info, 1);
+}
+
 int
 start_non_stop (int nonstop)
 {
diff --git a/gdb/target.c b/gdb/target.c
index 2c1d35b..1773eff 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1276,6 +1276,22 @@ target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
     return TARGET_XFER_E_IO;
 }
 
+/* See target/target.h.  */
+
+int
+target_read_uint32 (CORE_ADDR memaddr, unsigned int *result)
+{
+  gdb_byte buf[4];
+  int r;
+
+  r = target_read_memory (memaddr, buf, sizeof buf);
+  if (r != 0)
+    return r;
+  *result = extract_unsigned_integer (buf, sizeof buf,
+				      gdbarch_byte_order (target_gdbarch ()));
+  return 0;
+}
+
 /* Like target_read_memory, but specify explicitly that this is a read
    from the target's raw memory.  That is, this read bypasses the
    dcache, breakpoint shadowing, etc.  */
diff --git a/gdb/target.h b/gdb/target.h
index 4d91b6b..1a10744 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -58,6 +58,7 @@ struct dcache_struct;
    it goes into the file stratum, which is always below the process
    stratum.  */
 
+#include "target/target.h"
 #include "target/resume.h"
 #include "target/wait.h"
 #include "target/waitstatus.h"
@@ -1206,31 +1207,6 @@ extern void target_detach (const char *, int);
 
 extern void target_disconnect (const char *, int);
 
-/* Resume execution of the target process PTID (or a group of
-   threads).  STEP says whether to single-step or to run free; SIGGNAL
-   is the signal to be given to the target, or GDB_SIGNAL_0 for no
-   signal.  The caller may not pass GDB_SIGNAL_DEFAULT.  A specific
-   PTID means `step/resume only this process id'.  A wildcard PTID
-   (all threads, or all threads of process) means `step/resume
-   INFERIOR_PTID, and let other threads (for which the wildcard PTID
-   matches) resume with their 'thread->suspend.stop_signal' signal
-   (usually GDB_SIGNAL_0) if it is in "pass" state, or with no signal
-   if in "no pass" state.  */
-
-extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
-
-/* Wait for process pid to do something.  PTID = -1 to wait for any
-   pid to do something.  Return pid of child, or -1 in case of error;
-   store status through argument pointer STATUS.  Note that it is
-   _NOT_ OK to throw_exception() out of target_wait() without popping
-   the debugging target from the stack; GDB isn't prepared to get back
-   to the prompt with a debugging target but without the frame cache,
-   stop_pc, etc., set up.  OPTIONS is a bitwise OR of TARGET_W*
-   options.  */
-
-extern ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status,
-			   int options);
-
 /* Fetch at least register REGNO, or all regs if regno == -1.  No result.  */
 
 extern void target_fetch_registers (struct regcache *regcache, int regno);
@@ -1294,9 +1270,6 @@ int target_supports_disable_randomization (void);
 
 extern int target_read_string (CORE_ADDR, char **, int, int *);
 
-extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
-			       ssize_t len);
-
 extern int target_read_raw_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
 				   ssize_t len);
 
@@ -1304,9 +1277,6 @@ extern int target_read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len);
 
 extern int target_read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len);
 
-extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
-				ssize_t len);
-
 extern int target_write_raw_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
 				    ssize_t len);
 
@@ -1583,12 +1553,6 @@ extern int target_thread_alive (ptid_t ptid);
 
 extern void target_find_new_threads (void);
 
-/* Make target stop in a continuable fashion.  (For instance, under
-   Unix, this should act like SIGSTOP).  This function is normally
-   used by GUIs to implement a stop button.  */
-
-extern void target_stop (ptid_t ptid);
-
 /* Send the specified COMMAND to the target's monitor
    (shell,interpreter) for execution.  The result of the query is
    placed in OUTBUF.  */
diff --git a/gdb/target/target.h b/gdb/target/target.h
new file mode 100644
index 0000000..d9a4b86
--- /dev/null
+++ b/gdb/target/target.h
@@ -0,0 +1,98 @@
+/* Declarations for common target functions.
+
+   Copyright (C) 1986-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 TARGET_COMMON_H
+#define TARGET_COMMON_H
+
+#include "target/waitstatus.h"
+
+/* This header is a stopgap until more code is shared.  */
+
+/* Resume execution of the target process PTID (or a group of
+   threads).  STEP says whether to single-step or to run free; SIGGNAL
+   is the signal to be given to the target, or GDB_SIGNAL_0 for no
+   signal.  The caller may not pass GDB_SIGNAL_DEFAULT.  A specific
+   PTID means `step/resume only this process id'.  A wildcard PTID
+   (all threads, or all threads of process) means `step/resume
+   INFERIOR_PTID, and let other threads (for which the wildcard PTID
+   matches) resume with their 'thread->suspend.stop_signal' signal
+   (usually GDB_SIGNAL_0) if it is in "pass" state, or with no signal
+   if in "no pass" state.  */
+
+extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
+
+/* Wait for process pid to do something.  PTID = -1 to wait for any
+   pid to do something.  Return pid of child, or -1 in case of error;
+   store status through argument pointer STATUS.  Note that it is
+   _NOT_ OK to throw_exception() out of target_wait() without popping
+   the debugging target from the stack; GDB isn't prepared to get back
+   to the prompt with a debugging target but without the frame cache,
+   stop_pc, etc., set up.  OPTIONS is a bitwise OR of TARGET_W*
+   options.  */
+
+extern ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status,
+			   int options);
+
+/* Make target stop in a continuable fashion.  (For instance, under
+   Unix, this should act like SIGSTOP).  This function is normally
+   used by GUIs to implement a stop button.  */
+
+extern void target_stop (ptid_t ptid);
+
+/* Read LEN bytes of target memory at address MEMADDR, placing the
+   results in GDB's memory at MYADDR.  Return zero for success,
+   nonzero if any error occurs.  Implementations of this function may
+   define and use their own error codes, but functions in the common,
+   nat and target directories must treat the return code as opaque.
+   No guarantee is made about the contents of the data at MYADDR if
+   any error occurs.  */
+
+extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
+			       ssize_t len);
+
+/* Read an unsigned 32-bit integer in the target's format from target
+   memory at address MEMADDR, storing the result in GDB's format in
+   GDB's memory at RESULT.  Return zero for success, nonzero if any
+   error occurs.  Implementations of this function may define and use
+   their own error codes, but functions in the common, nat and target
+   directories must treat the return code as opaque.  No guarantee is
+   made about the contents of the data at RESULT if any error
+   occurs.  */
+
+extern int target_read_uint32 (CORE_ADDR memaddr, unsigned int *result);
+
+/* Write LEN bytes from MYADDR to target memory at address MEMADDR.
+   Return zero for success, nonzero if any error occurs.
+   Implementations of this function may define and use their own error
+   codes, but functions in the common, nat and target directories must
+   treat the return code as opaque.  No guarantee is made about the
+   contents of the data at MEMADDR if any error occurs.  */
+
+extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
+				ssize_t len);
+
+/* If set, the inferior should be controlled in non-stop mode.  In
+   this mode, each thread is controlled independently.  Execution
+   commands apply only to the selected thread by default, and stop
+   events stop only the thread that had the event -- the other threads
+   are kept running freely.  */
+
+extern int non_stop;
+
+#endif /* TARGET_COMMON_H */
-- 
1.7.1

Re: [PATCH 05/11 v5] Add target/target.h

[Doug Evans]

Gary Benson writes:
 > This adds target/target.h.  This file declares some functions that
 > the shared code can use and that the clients must implement.  It
 > also changes some shared code to use these functions.
 > 
 > gdb/
 > 2014-08-01  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* target/target.h: New file.
 > 	* Makefile.in (HFILES_NO_SRCDIR): Add target/target.h.
 > 	* target.h: Include target/target.h.
 > 	(target_resume, target_wait, target_stop, target_read_memory)
 > 	(target_write_memory, non_stop): Don't declare.
 > 	* target.c (target_read_uint32): New function.
 > 	* common/agent.c: Include target/target.h.
 > 	[!GDBSERVER]: Don't include target.h or infrun.h.
 > 	(agent_get_helper_thread_id): Use target_read_uint32.
 > 	(agent_run_command): Always use target_resume, target_stop,
 > 	target_wait, target_write_memory, and target_read_memory.
 > 	(agent_capability_check): Use target_read_uint32.
 > 
 > gdb/gdbserver/
 > 2014-08-01  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* target.c (target_wait, target_stop, target_resume)
 > 	(target_read_memory, target_read_uint32)
 > 	(target_write_memory): New functions.
 > ---
 >  gdb/ChangeLog           |   16 ++++++++
 >  gdb/Makefile.in         |    2 +-
 >  gdb/common/agent.c      |   76 +++---------------------------------
 >  gdb/gdbserver/ChangeLog |    7 +++
 >  gdb/gdbserver/target.c  |   58 ++++++++++++++++++++++++++++
 >  gdb/target.c            |   16 ++++++++
 >  gdb/target.h            |   38 +------------------
 >  gdb/target/target.h     |   98 +++++++++++++++++++++++++++++++++++++++++++++++
 >  8 files changed, 204 insertions(+), 107 deletions(-)
 >  create mode 100644 gdb/target/target.h
 > 
 > diff --git a/gdb/Makefile.in b/gdb/Makefile.in
 > index 8429ebc..090c912 100644
 > --- a/gdb/Makefile.in
 > +++ b/gdb/Makefile.in
 > @@ -936,7 +936,7 @@ 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 \
 > -common/common-debug.h
 > +common/common-debug.h target/target.h
 >  
 >  # Header files that already have srcdir in them, or which are in objdir.
 >  
 > diff --git a/gdb/common/agent.c b/gdb/common/agent.c
 > index 7071ce6..5c19454 100644
 > --- a/gdb/common/agent.c
 > +++ b/gdb/common/agent.c
 > @@ -21,10 +21,9 @@
 >  #include "server.h"
 >  #else
 >  #include "defs.h"
 > -#include "target.h"
 > -#include "infrun.h"
 >  #include "objfiles.h"
 >  #endif
 > +#include "target/target.h"
 >  #include <unistd.h>
 >  #include "agent.h"
 >  #include "filestuff.h"
 > @@ -126,23 +125,9 @@ agent_get_helper_thread_id (void)
 >  {
 >    if  (helper_thread_id == 0)
 >      {
 > -#ifdef GDBSERVER
 > -      if (read_inferior_memory (ipa_sym_addrs.addr_helper_thread_id,
 > -				(unsigned char *) &helper_thread_id,
 > -				sizeof helper_thread_id))
 > -#else
 > -      enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch ());
 > -      gdb_byte buf[4];
 > -
 > -      if (target_read_memory (ipa_sym_addrs.addr_helper_thread_id,
 > -			      buf, sizeof buf) == 0)
 > -	helper_thread_id = extract_unsigned_integer (buf, sizeof buf,
 > -						     byte_order);
 > -      else
 > -#endif
 > -	{
 > -	  warning (_("Error reading helper thread's id in lib"));
 > -	}
 > +      if (target_read_uint32 (ipa_sym_addrs.addr_helper_thread_id,
 > +			      &helper_thread_id))
 > +	warning (_("Error reading helper thread's id in lib"));
 >      }
 >  
 >    return helper_thread_id;
 > @@ -220,13 +205,8 @@ agent_run_command (int pid, const char *cmd, int len)
 >    int tid = agent_get_helper_thread_id ();
 >    ptid_t ptid = ptid_build (pid, tid, 0);
 >  
 > -#ifdef GDBSERVER
 > -  int ret = write_inferior_memory (ipa_sym_addrs.addr_cmd_buf,
 > -				   (const unsigned char *) cmd, len);
 > -#else
 >    int ret = target_write_memory (ipa_sym_addrs.addr_cmd_buf,
 >  				 (gdb_byte *) cmd, len);
 > -#endif
 >  
 >    if (ret != 0)
 >      {
 > @@ -237,18 +217,7 @@ agent_run_command (int pid, const char *cmd, int len)
 >    DEBUG_AGENT ("agent: resumed helper thread\n");
 >  
 >    /* Resume helper thread.  */
 > -#ifdef GDBSERVER
 > -{
 > -  struct thread_resume resume_info;
 > -
 > -  resume_info.thread = ptid;
 > -  resume_info.kind = resume_continue;
 > -  resume_info.sig = GDB_SIGNAL_0;
 > -  (*the_target->resume) (&resume_info, 1);
 > -}
 > -#else
 > - target_resume (ptid, 0, GDB_SIGNAL_0);
 > -#endif
 > +  target_resume (ptid, 0, GDB_SIGNAL_0);
 >  
 >    fd = gdb_connect_sync_socket (pid);
 >    if (fd >= 0)
 > @@ -284,37 +253,18 @@ agent_run_command (int pid, const char *cmd, int len)
 >        int was_non_stop = non_stop;
 >        /* Stop thread PTID.  */
 >        DEBUG_AGENT ("agent: stop helper thread\n");
 > -#ifdef GDBSERVER
 > -      {
 > -	struct thread_resume resume_info;
 > -
 > -	resume_info.thread = ptid;
 > -	resume_info.kind = resume_stop;
 > -	resume_info.sig = GDB_SIGNAL_0;
 > -	(*the_target->resume) (&resume_info, 1);

I certainly welcome replacing that with target_stop(). :-)

 > -      }
 > -
 > -      non_stop = 1;
 > -      mywait (ptid, &status, 0, 0);

The old gdbserver code set non_stop = 1 *after* asking
the target to stop, whereas now it'll be done before (right?).
Just checking that that's ok.
E.g., I see a test for non_stop in linux_resume (which feels weird to be
using in this context given that we're talking about target_stop :-)).

 > -#else
 >        non_stop = 1;
 >        target_stop (ptid);
 >  
 >        memset (&status, 0, sizeof (status));
 >        target_wait (ptid, &status, 0);
 > -#endif
 >        non_stop = was_non_stop;
 >      }
 >  
 >    if (fd >= 0)
 >      {
 > -#ifdef GDBSERVER
 > -      if (read_inferior_memory (ipa_sym_addrs.addr_cmd_buf,
 > -				(unsigned char *) cmd, IPA_CMD_BUF_SIZE))
 > -#else
 >        if (target_read_memory (ipa_sym_addrs.addr_cmd_buf, (gdb_byte *) cmd,
 >  			      IPA_CMD_BUF_SIZE))
 > -#endif
 >  	{
 >  	  warning (_("Error reading command response"));
 >  	  return -1;
 > @@ -334,20 +284,8 @@ agent_capability_check (enum agent_capa agent_capa)
 >  {
 >    if (agent_capability == 0)
 >      {
 > -#ifdef GDBSERVER
 > -      if (read_inferior_memory (ipa_sym_addrs.addr_capability,
 > -				(unsigned char *) &agent_capability,
 > -				sizeof agent_capability))
 > -#else
 > -      enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch ());
 > -      gdb_byte buf[4];
 > -
 > -      if (target_read_memory (ipa_sym_addrs.addr_capability,
 > -			      buf, sizeof buf) == 0)
 > -	agent_capability = extract_unsigned_integer (buf, sizeof buf,
 > -						     byte_order);
 > -      else
 > -#endif
 > +      if (target_read_uint32 (ipa_sym_addrs.addr_capability,
 > +			      &agent_capability))
 >  	warning (_("Error reading capability of agent"));
 >      }
 >    return agent_capability & agent_capa;
 > [...]

LGTM with the above question resolved.

Re: [PATCH 05/11 v5] Add target/target.h

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
> > @@ -284,37 +253,18 @@ agent_run_command (int pid, const char *cmd,
> >        int was_non_stop = non_stop;
> >        /* Stop thread PTID.  */
> >        DEBUG_AGENT ("agent: stop helper thread\n");
> > -#ifdef GDBSERVER
> > -      {
> > -        struct thread_resume resume_info;
> > -
> > -        resume_info.thread = ptid;
> > -        resume_info.kind = resume_stop;
> > -        resume_info.sig = GDB_SIGNAL_0;
> > -        (*the_target->resume) (&resume_info, 1);
> > -      }
> > -
> > -      non_stop = 1;
> > -      mywait (ptid, &status, 0, 0);
> > -#else
> >        non_stop = 1;
> >        target_stop (ptid);
> >  
> >        memset (&status, 0, sizeof (status));
> >        target_wait (ptid, &status, 0);
> > -#endif
> >        non_stop = was_non_stop;
> 
> The old gdbserver code set non_stop = 1 *after* asking the target to
> stop, whereas now it'll be done before (right?).  Just checking that
> that's ok.
> E.g., I see a test for non_stop in linux_resume (which feels weird to be
> using in this context given that we're talking about target_stop :-)).

Good catch!  I did not notice that change.  I also don't know if it's
ok.

In the gdbserver case forcing non_stop to 1 causes need_step_over
in linux_resume to become maybe set.  If non_stop had been 0
need_step_over would definitely be NULL.  So forcing non_stop to 1
beforehand like this patch does means a step over might take place
that would otherwise not have.

In the GDB case forcing non_stop to 1 before target_stop forces GDB
to send a SIGSTOP to each LWP.  If non_stop had been 0 linux_nat_stop
would have fallen back to inf_ptrace_stop which sends one SIGINT to
the process group.

I don't know enough about this to know which is the best solution.
Pedro would know, but he's away for the next two weeks.  If what is
happening currently is correct in both cases then maybe a new target
method "target_stop_all" is required to encompass the whole block of
code.

In the interests of keeping things moving would you be ok for me to
commit the following until Pedro is back and able to advise?

      /* FIXME: This conditional code needs removing, either by
         setting non_stop in the same place for both cases or by
	 implementing a new client method for this whole block
	 (less the printing code) from "int was_non_stop = non_stop;"
	 to "non_stop = was_non_stop;".  */
#ifdef GDBSERVER
      target_stop (ptid);
      non_stop = 1;
#else
      non_stop = 1;
      target_stop (ptid);
#endif

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 05/11 v5] Add target/target.h

[Pedro Alves]

On 08/07/2014 02:48 PM, Gary Benson wrote:
> Doug Evans wrote:
>> Gary Benson writes:
>>> @@ -284,37 +253,18 @@ agent_run_command (int pid, const char *cmd,
>>>        int was_non_stop = non_stop;
>>>        /* Stop thread PTID.  */
>>>        DEBUG_AGENT ("agent: stop helper thread\n");
>>> -#ifdef GDBSERVER
>>> -      {
>>> -        struct thread_resume resume_info;
>>> -
>>> -        resume_info.thread = ptid;
>>> -        resume_info.kind = resume_stop;
>>> -        resume_info.sig = GDB_SIGNAL_0;
>>> -        (*the_target->resume) (&resume_info, 1);
>>> -      }
>>> -
>>> -      non_stop = 1;
>>> -      mywait (ptid, &status, 0, 0);
>>> -#else
>>>        non_stop = 1;
>>>        target_stop (ptid);
>>>  
>>>        memset (&status, 0, sizeof (status));
>>>        target_wait (ptid, &status, 0);
>>> -#endif
>>>        non_stop = was_non_stop;
>>
>> The old gdbserver code set non_stop = 1 *after* asking the target to
>> stop, whereas now it'll be done before (right?).  Just checking that
>> that's ok.
>> E.g., I see a test for non_stop in linux_resume (which feels weird to be
>> using in this context given that we're talking about target_stop :-)).
> 
> Good catch!  I did not notice that change.  I also don't know if it's
> ok.
> 
> In the gdbserver case forcing non_stop to 1 causes need_step_over
> in linux_resume to become maybe set.  

> If non_stop had been 0
> need_step_over would definitely be NULL. 

That isn't really true, see:

  any_pending = 0;
  if (!non_stop)
    find_inferior (&all_threads, resume_status_pending_p, &any_pending); #1
...
  if (!any_pending && supports_breakpoints ())
    need_step_over
      = (struct thread_info *) find_inferior (&all_threads,
					      need_step_over_p, NULL);

If non_stop is 0, then we execute #1 above, true.  But, that may well
return with ANY_PENDING still clear/0, and so 'need_step_over' may end
up set anyway.

So looks fine to me.

> So forcing non_stop to 1
> beforehand like this patch does means a step over might take place
> that would otherwise not have.

See above.

> 
> In the GDB case forcing non_stop to 1 before target_stop forces GDB
> to send a SIGSTOP to each LWP. 

Note we're just really just stopping one LWP here, the agent helper
thread, specified in PTID, not all threads.

> If non_stop had been 0 linux_nat_stop
> would have fallen back to inf_ptrace_stop which sends one SIGINT to
> the process group.
> 

Yeah, we definitely want SIGSTOP, not SIGINT here.  Really, GDB_SIGNAL_0:
SIGSTOP is how we implement "quiesce with no signal" on Linux -- the
SIGSTOP is not visible to the target_wait caller.  Unfortunately we have
a mixup of "interrupt/ctrl-c" vs "quiesce" in the interface.

> I don't know enough about this to know which is the best solution.
> Pedro would know, but he's away for the next two weeks.  If what is
> happening currently is correct in both cases then maybe a new target
> method "target_stop_all" is required to encompass the whole block of
> code.
> 
> In the interests of keeping things moving would you be ok for me to
> commit the following until Pedro is back and able to advise?
> 
>       /* FIXME: This conditional code needs removing, either by
>          setting non_stop in the same place for both cases or by
> 	 implementing a new client method for this whole block
> 	 (less the printing code) from "int was_non_stop = non_stop;"
> 	 to "non_stop = was_non_stop;".  */
> #ifdef GDBSERVER
>       target_stop (ptid);
>       non_stop = 1;
> #else
>       non_stop = 1;
>       target_stop (ptid);
> #endif

Thanks,
Pedro Alves

Re: [PATCH 05/11 v5] Add target/target.h

[Gary Benson]

Pedro Alves wrote:
> On 08/07/2014 02:48 PM, Gary Benson wrote:
> > Doug Evans wrote:
> > > Gary Benson writes:
> > > > @@ -284,37 +253,18 @@ agent_run_command (int pid, const char *cmd,
> > > >        int was_non_stop = non_stop;
> > > >        /* Stop thread PTID.  */
> > > >        DEBUG_AGENT ("agent: stop helper thread\n");
> > > > -#ifdef GDBSERVER
> > > > -      {
> > > > -        struct thread_resume resume_info;
> > > > -
> > > > -        resume_info.thread = ptid;
> > > > -        resume_info.kind = resume_stop;
> > > > -        resume_info.sig = GDB_SIGNAL_0;
> > > > -        (*the_target->resume) (&resume_info, 1);
> > > > -      }
> > > > -
> > > > -      non_stop = 1;
> > > > -      mywait (ptid, &status, 0, 0);
> > > > -#else
> > > >        non_stop = 1;
> > > >        target_stop (ptid);
> > > >  
> > > >        memset (&status, 0, sizeof (status));
> > > >        target_wait (ptid, &status, 0);
> > > > -#endif
> > > >        non_stop = was_non_stop;
> > >
> > > The old gdbserver code set non_stop = 1 *after* asking the target to
> > > stop, whereas now it'll be done before (right?).  Just checking that
> > > that's ok.
> > > E.g., I see a test for non_stop in linux_resume (which feels weird to be
> > > using in this context given that we're talking about target_stop :-)).
> > 
> > Good catch!  I did not notice that change.  I also don't know if it's
> > ok.
> > 
> > In the gdbserver case forcing non_stop to 1 causes need_step_over
> > in linux_resume to become maybe set.  
> 
> > If non_stop had been 0
> > need_step_over would definitely be NULL. 
> 
> That isn't really true, see:
> 
>   any_pending = 0;
>   if (!non_stop)
>     find_inferior (&all_threads, resume_status_pending_p, &any_pending); #1
> ...
>   if (!any_pending && supports_breakpoints ())
>     need_step_over
>       = (struct thread_info *) find_inferior (&all_threads,
> 					      need_step_over_p, NULL);
> 
> If non_stop is 0, then we execute #1 above, true.  But, that may well
> return with ANY_PENDING still clear/0, and so 'need_step_over' may end
> up set anyway.
> 
> So looks fine to me.
> 
> > So forcing non_stop to 1
> > beforehand like this patch does means a step over might take place
> > that would otherwise not have.
> 
> See above.
> 
> > In the GDB case forcing non_stop to 1 before target_stop forces GDB
> > to send a SIGSTOP to each LWP. 
> 
> Note we're just really just stopping one LWP here, the agent helper
> thread, specified in PTID, not all threads.
> 
> > If non_stop had been 0 linux_nat_stop
> > would have fallen back to inf_ptrace_stop which sends one SIGINT to
> > the process group.
> 
> Yeah, we definitely want SIGSTOP, not SIGINT here.  Really, GDB_SIGNAL_0:
> SIGSTOP is how we implement "quiesce with no signal" on Linux -- the
> SIGSTOP is not visible to the target_wait caller.  Unfortunately we have
> a mixup of "interrupt/ctrl-c" vs "quiesce" in the interface.

Pedro, to mirror your new 'target_continue_ptid (ptid_t ptid)' function
suggestion, I thought I might make a new 'target_stop_ptid (ptid_t ptid)'
that would handle the stop/wait combination and the non_stop fiddling.
That way everything will stay exactly as it is.  Does that sound ok to
you?

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 05/11 v5] Add target/target.h

[Pedro Alves]

Hi Gary,

On 08/20/2014 04:01 PM, Gary Benson wrote:

> Pedro, to mirror your new 'target_continue_ptid (ptid_t ptid)' function
> suggestion, I thought I might make a new 'target_stop_ptid (ptid_t ptid)'
> that would handle the stop/wait combination and the non_stop fiddling.
> That way everything will stay exactly as it is.  Does that sound ok to
> you?

That sounds OK to me.

Thanks,
Pedro Alves

Re: [PATCH 05/11 v5] Add target/target.h

[Pedro Alves]

(Reviewing in pieces)

On 08/01/2014 11:19 AM, Gary Benson wrote:

> +/* See target/target.h.  */
> +
> +int
> +target_read_uint32 (CORE_ADDR memaddr, unsigned int *result)
> +{

The type of 'result' should be uint32_t then.

> +
> +/* Resume execution of the target process PTID (or a group of
> +   threads).  STEP says whether to single-step or to run free; SIGGNAL
> +   is the signal to be given to the target, or GDB_SIGNAL_0 for no
> +   signal.  The caller may not pass GDB_SIGNAL_DEFAULT.  A specific
> +   PTID means `step/resume only this process id'.  A wildcard PTID
> +   (all threads, or all threads of process) means `step/resume
> +   INFERIOR_PTID, and let other threads (for which the wildcard PTID
> +   matches) resume with their 'thread->suspend.stop_signal' signal
> +   (usually GDB_SIGNAL_0) if it is in "pass" state, or with no signal
> +   if in "no pass" state.  */

Most of this comment doesn't make any sense for gdbserver, and I don't
ever will.  (GDBserver's resume interface is more flexible than gdb's.)
There's no inferior_ptid in gdbserver.  There's no thread->suspend.stop_signal.
In the gdbserver implemention this adds:

> +  resume_info.thread = ptid;
> +  resume_info.kind = step ? resume_step : resume_continue;
> +  resume_info.sig = signal;
> +  (*the_target->resume) (&resume_info, 1);

... when STEP is true and PTID is a wildcard, what this actually does
is tell the target to step each thread in the wildcard, all in parallel.

I think we should instead add a new 'target_continue_ptid (ptid_t ptid)'
method, that then consumes/calls gdb's target_resume in gdb's implementation,
and gdbserver's '(*the_target->resume) (&resume_info, 1);' in the
gdbserver implementation?

> +
> +extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);

Thanks,
Pedro Alves

Re: [PATCH 05/11 v5] Add target/target.h

[Pedro Alves]

On 08/01/2014 11:19 AM, Gary Benson wrote:

> diff --git a/gdb/target.h b/gdb/target.h
> index 4d91b6b..1a10744 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -58,6 +58,7 @@ struct dcache_struct;
>     it goes into the file stratum, which is always below the process
>     stratum.  */
>  
> +#include "target/target.h"
>  #include "target/resume.h"
>  #include "target/wait.h"
>  #include "target/waitstatus.h"
> @@ -1206,31 +1207,6 @@ extern void target_detach (const char *, int);
>  
>  extern void target_disconnect (const char *, int);
>  
> -/* Resume execution of the target process PTID (or a group of
> -   threads).  STEP says whether to single-step or to run free; SIGGNAL
> -   is the signal to be given to the target, or GDB_SIGNAL_0 for no
> -   signal.  The caller may not pass GDB_SIGNAL_DEFAULT.  A specific
> -   PTID means `step/resume only this process id'.  A wildcard PTID
> -   (all threads, or all threads of process) means `step/resume
> -   INFERIOR_PTID, and let other threads (for which the wildcard PTID
> -   matches) resume with their 'thread->suspend.stop_signal' signal
> -   (usually GDB_SIGNAL_0) if it is in "pass" state, or with no signal
> -   if in "no pass" state.  */
> -
> -extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
> -
> -/* Wait for process pid to do something.  PTID = -1 to wait for any
> -   pid to do something.  Return pid of child, or -1 in case of error;
> -   store status through argument pointer STATUS.  Note that it is
> -   _NOT_ OK to throw_exception() out of target_wait() without popping
> -   the debugging target from the stack; GDB isn't prepared to get back
> -   to the prompt with a debugging target but without the frame cache,
> -   stop_pc, etc., set up.  OPTIONS is a bitwise OR of TARGET_W*
> -   options.  */
> -
> -extern ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status,
> -			   int options);
> -
>  /* Fetch at least register REGNO, or all regs if regno == -1.  No result.  */
>  
>  extern void target_fetch_registers (struct regcache *regcache, int regno);
> @@ -1294,9 +1270,6 @@ int target_supports_disable_randomization (void);
>  
>  extern int target_read_string (CORE_ADDR, char **, int, int *);
>  
> -extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
> -			       ssize_t len);
> -
>  extern int target_read_raw_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
>  				   ssize_t len);
>  
> @@ -1304,9 +1277,6 @@ extern int target_read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len);
>  
>  extern int target_read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len);
>  
> -extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
> -				ssize_t len);
> -
>  extern int target_write_raw_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
>  				    ssize_t len);
>  
> @@ -1583,12 +1553,6 @@ extern int target_thread_alive (ptid_t ptid);
>  
>  extern void target_find_new_threads (void);
>  
> -/* Make target stop in a continuable fashion.  (For instance, under
> -   Unix, this should act like SIGSTOP).  This function is normally
> -   used by GUIs to implement a stop button.  */
> -
> -extern void target_stop (ptid_t ptid);
> -
>  /* Send the specified COMMAND to the target's monitor
>     (shell,interpreter) for execution.  The result of the query is
>     placed in OUTBUF.  */

I worry about the fragmentation that moving pieces of target
interfaces to target/target.h generates ends up confusing
people.  Did you consider leaving a marker in place, like:

 -extern ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status,
 -			   int options);
 + /* For target_wait see target/target.h.  */

 -extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
 -				ssize_t len);
 + /* For target_write_memory see target/target.h.  */

?

Thanks,
Pedro Alves

Re: [PATCH 05/11 v5] Add target/target.h

[Gary Benson]

Pedro Alves wrote:
> I worry about the fragmentation that moving pieces of target
> interfaces to target/target.h generates ends up confusing
> people.  Did you consider leaving a marker in place, like:
> 
>  -extern ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status,
>  -			   int options);
>  + /* For target_wait see target/target.h.  */
> 
>  -extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
>  -				ssize_t len);
>  + /* For target_write_memory see target/target.h.  */
> 
> ?

I can certainly do that.  Will include it in the next revision.

Cheers,
Gary

-- 
http://gbenson.net/

[PATCH 01/11 v5] Introduce common/errors.h

[Gary Benson]

This introduces common/errors.h.  This holds some error- and
warning-related declarations that can be used by the code in common.
Clients of the "common" code must provide definitions for these
functions.

gdb/
2014-08-01  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* common/errors.h: New file.
	* common/errors.c: Likewise.
	* Makefile.in (HFILES_NO_SRCDIR): Add common/errors.h.
	(COMMON_OBS): Add errors.o.
	(errors.o): New rule.
	* common/common-defs.h: Include errors.h.
	* utils.h (perror_with_name, error, verror, warning, vwarning):
	Don't declare.
	* common/common-utils.h: (malloc_failure, internal_error):
	Likewise.

gdb/gdbserver/
2014-08-01  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* Makefile.in (SFILES): Add common/errors.c.
	(OBS): Add errors.o.
	(IPA_OBS): Add errors-ipa.o.
	(errors.o): New rule.
	(errors-ipa.o): Likewise.
	* utils.h (perror_with_name, error, warning): Don't declare.
	* utils.c (warning): Renamed and rewritten as...
	(vwarning): New function.
	(error): Renamed and rewritten as...
	(verror): New function.
	(internal_error): Renamed and rewritten as...
	(internal_verror): New function.
---
 gdb/ChangeLog             |   14 +++++++++
 gdb/Makefile.in           |    8 ++++-
 gdb/common/common-defs.h  |    1 +
 gdb/common/common-utils.h |    4 --
 gdb/common/errors.c       |   61 ++++++++++++++++++++++++++++++++++++++
 gdb/common/errors.h       |   72 +++++++++++++++++++++++++++++++++++++++++++++
 gdb/gdbserver/ChangeLog   |   16 ++++++++++
 gdb/gdbserver/Makefile.in |   16 ++++++++--
 gdb/gdbserver/utils.c     |   23 ++++----------
 gdb/gdbserver/utils.h     |    3 --
 gdb/utils.c               |   36 ----------------------
 gdb/utils.h               |   15 ---------
 12 files changed, 190 insertions(+), 79 deletions(-)
 create mode 100644 gdb/common/errors.c
 create mode 100644 gdb/common/errors.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 8361030..4d3e7e5 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -935,7 +935,7 @@ 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
+i386-linux-nat.h common/common-defs.h common/errors.h
 
 # Header files that already have srcdir in them, or which are in objdir.
 
@@ -1034,7 +1034,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
+	print-utils.o rsp-low.o errors.o
 
 TSOBS = inflow.o
 
@@ -2144,6 +2144,10 @@ rsp-low.o: ${srcdir}/common/rsp-low.c
 	$(COMPILE) $(srcdir)/common/rsp-low.c
 	$(POSTCOMPILE)
 
+errors.o: ${srcdir}/common/errors.c
+	$(COMPILE) $(srcdir)/common/errors.c
+	$(POSTCOMPILE)
+
 #
 # gdb/target/ dependencies
 #
diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index 82290dc..a15423c 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -41,5 +41,6 @@
 #include "ptid.h"
 #include "common-utils.h"
 #include "gdb_assert.h"
+#include "errors.h"
 
 #endif /* COMMON_DEFS_H */
diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 9038bc2..67615ba 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -38,10 +38,6 @@
 #endif
 #endif
 
-extern void malloc_failure (long size) ATTRIBUTE_NORETURN;
-extern void internal_error (const char *file, int line, const char *, ...)
-     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 4);
-
 /* xmalloc(), xrealloc() and xcalloc() have already been declared in
    "libiberty.h". */
 
diff --git a/gdb/common/errors.c b/gdb/common/errors.c
new file mode 100644
index 0000000..7d0bb6e
--- /dev/null
+++ b/gdb/common/errors.c
@@ -0,0 +1,61 @@
+/* Error reporting facilities.
+
+   Copyright (C) 1986-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/>.  */
+
+#ifdef GDBSERVER
+#include "server.h"
+#else
+#include "defs.h"
+#endif
+#include "errors.h"
+
+/* See common/errors.h.  */
+
+void
+warning (const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+  vwarning (fmt, ap);
+  va_end (ap);
+}
+
+/* See common/errors.h.  */
+
+void
+error (const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+  verror (fmt, ap);
+  va_end (ap);
+}
+
+/* See common/errors.h.  */
+
+void
+internal_error (const char *file, int line, const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+  internal_verror (file, line, fmt, ap);
+  va_end (ap);
+}
diff --git a/gdb/common/errors.h b/gdb/common/errors.h
new file mode 100644
index 0000000..ff4e1e3
--- /dev/null
+++ b/gdb/common/errors.h
@@ -0,0 +1,72 @@
+/* Declarations for error-reporting facilities.
+
+   Copyright (C) 1986-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_ERRORS_H
+#define COMMON_ERRORS_H
+
+/* A problem was detected, but the requested operation can still
+   proceed.  A warning message is constructed using a printf- or
+   vprintf-style argument list.  */
+
+extern void warning (const char *fmt, ...)
+     ATTRIBUTE_PRINTF (1, 2);
+
+extern void vwarning (const char *fmt, va_list args)
+     ATTRIBUTE_PRINTF (1, 0);
+
+/* A non-predictable, non-fatal error was detected.  The requested
+   operation cannot proceed.  An error message is constructed using
+   a printf- or vprintf-style argument list.  These functions do not
+   return.  */
+
+extern void error (const char *fmt, ...)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
+
+extern void verror (const char *fmt, va_list args)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
+
+/* An internal error was detected.  Internal errors indicate
+   programming errors such as assertion failures, as opposed to
+   more general errors beyond the application's control.  These
+   functions do not return.  An error message is constructed using
+   a printf- or vprintf-style argument list.  FILE and LINE
+   indicate the file and line number where the programming error
+   was detected.  */
+
+extern void internal_error (const char *file, int line,
+			    const char *fmt, ...)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 4);
+
+extern void internal_verror (const char *file, int line,
+			     const char *fmt, va_list args)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 0);
+\f
+
+/* Like "error", but the error message is constructed by combining
+   STRING with the system error message for errno.  This function
+   does not return.  */
+
+extern void perror_with_name (const char *string) ATTRIBUTE_NORETURN;
+
+/* Call this function to handle memory allocation failures.
+   This function does not return.   */
+
+extern void malloc_failure (long size) ATTRIBUTE_NORETURN;
+
+#endif /* COMMON_ERRORS_H */
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index f9a2f17..1faa00c 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -169,7 +169,7 @@ SFILES=	$(srcdir)/gdbreplay.c $(srcdir)/inferiors.c $(srcdir)/dll.c \
 	$(srcdir)/common/buffer.c $(srcdir)/nat/linux-btrace.c \
 	$(srcdir)/common/filestuff.c $(srcdir)/target/waitstatus.c \
 	$(srcdir)/nat/mips-linux-watch.c $(srcdir)/common/print-utils.c \
-	$(srcdir)/common/rsp-low.c
+	$(srcdir)/common/rsp-low.c $(srcdir)/common/errors.c
 
 DEPFILES = @GDBSERVER_DEPFILES@
 
@@ -182,7 +182,8 @@ OBS = agent.o ax.o inferiors.o regcache.o remote-utils.o server.o signals.o \
       target.o waitstatus.o utils.o debug.o version.o vec.o gdb_vecs.o \
       mem-break.o hostio.o event-loop.o tracepoint.o xml-utils.o \
       common-utils.o ptid.o buffer.o format.o filestuff.o dll.o notif.o \
-      tdesc.o print-utils.o rsp-low.o $(XML_BUILTIN) $(DEPFILES) $(LIBOBJS)
+      tdesc.o print-utils.o rsp-low.o errors.o $(XML_BUILTIN) $(DEPFILES) \
+      $(LIBOBJS)
 GDBREPLAY_OBS = gdbreplay.o version.o
 GDBSERVER_LIBS = @GDBSERVER_LIBS@
 XM_CLIBS = @LIBS@
@@ -300,7 +301,10 @@ gdbreplay$(EXEEXT): $(GDBREPLAY_OBS) $(LIBGNU) $(LIBIBERTY)
 	${CC-LD} $(INTERNAL_CFLAGS) $(INTERNAL_LDFLAGS) -o gdbreplay$(EXEEXT) $(GDBREPLAY_OBS) \
 	  $(XM_CLIBS) $(LIBGNU) $(LIBIBERTY)
 
-IPA_OBJS=ax-ipa.o tracepoint-ipa.o format-ipa.o utils-ipa.o regcache-ipa.o remote-utils-ipa.o common-utils-ipa.o tdesc-ipa.o print-utils-ipa.o rsp-low-ipa.o ${IPA_DEPFILES}
+IPA_OBJS=ax-ipa.o tracepoint-ipa.o format-ipa.o utils-ipa.o \
+	regcache-ipa.o remote-utils-ipa.o common-utils-ipa.o \
+	tdesc-ipa.o print-utils-ipa.o rsp-low-ipa.o errors-ipa.o \
+	${IPA_DEPFILES}
 
 IPA_LIB=libinproctrace.so
 
@@ -489,6 +493,9 @@ print-utils-ipa.o: ../common/print-utils.c
 rsp-low-ipa.o: ../common/rsp-low.c
 	$(IPAGENT_COMPILE) $<
 	$(POSTCOMPILE)
+errors-ipa.o: ../common/errors.c
+	$(IPAGENT_COMPILE) $<
+	$(POSTCOMPILE)
 
 ax.o: ax.c
 	$(COMPILE) $(WARN_CFLAGS_NO_FORMAT) $<
@@ -530,6 +537,9 @@ filestuff.o: ../common/filestuff.c
 agent.o: ../common/agent.c
 	$(COMPILE) $<
 	$(POSTCOMPILE)
+errors.o: ../common/errors.c
+	$(COMPILE) $<
+	$(POSTCOMPILE)
 waitstatus.o: ../target/waitstatus.c
 	$(COMPILE) $<
 	$(POSTCOMPILE)
diff --git a/gdb/gdbserver/utils.c b/gdb/gdbserver/utils.c
index b011637..3aac3cd 100644
--- a/gdb/gdbserver/utils.c
+++ b/gdb/gdbserver/utils.c
@@ -71,18 +71,15 @@ perror_with_name (const char *string)
   error ("%s.", combined);
 }
 
-/* Print an error message and return to command level.
-   STRING is the error message, used as a fprintf string,
-   and ARG is passed as an argument to it.  */
+/* Print an error message and return to top level.  */
 
 void
-error (const char *string,...)
+verror (const char *string, va_list args)
 {
 #ifndef IN_PROCESS_AGENT
   extern jmp_buf toplevel;
 #endif
-  va_list args;
-  va_start (args, string);
+
   fflush (stdout);
   vfprintf (stderr, string, args);
   fprintf (stderr, "\n");
@@ -110,31 +107,25 @@ fatal (const char *string,...)
   exit (1);
 }
 
-/* VARARGS */
+/* Print a warning message.  */
+
 void
-warning (const char *string,...)
+vwarning (const char *string, va_list args)
 {
-  va_list args;
-  va_start (args, string);
   fprintf (stderr, PREFIX);
   vfprintf (stderr, string, args);
   fprintf (stderr, "\n");
-  va_end (args);
 }
 
 /* Report a problem internal to GDBserver, and exit.  */
 
 void
-internal_error (const char *file, int line, const char *fmt, ...)
+internal_verror (const char *file, int line, const char *fmt, va_list args)
 {
-  va_list args;
-  va_start (args, fmt);
-
   fprintf (stderr,  "\
 %s:%d: A problem internal to " TOOLNAME " has been detected.\n", file, line);
   vfprintf (stderr, fmt, args);
   fprintf (stderr, "\n");
-  va_end (args);
   exit (1);
 }
 
diff --git a/gdb/gdbserver/utils.h b/gdb/gdbserver/utils.h
index 906924b..a994f38 100644
--- a/gdb/gdbserver/utils.h
+++ b/gdb/gdbserver/utils.h
@@ -21,10 +21,7 @@
 
 #include "print-utils.h"
 
-void perror_with_name (const char *string) ATTRIBUTE_NORETURN;
-void error (const char *string,...) ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
 void fatal (const char *string,...) ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
-void warning (const char *string,...) ATTRIBUTE_PRINTF (1, 2);
 char *paddress (CORE_ADDR addr);
 char *pfildes (gdb_fildes_t fd);
 
diff --git a/gdb/utils.c b/gdb/utils.c
index 3849128..ab62084 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -531,22 +531,6 @@ vwarning (const char *string, va_list args)
     }
 }
 
-/* Print a warning message.
-   The first argument STRING is the warning message, used as a fprintf string,
-   and the remaining args are passed as arguments to it.
-   The primary difference between warnings and errors is that a warning
-   does not force the return to command level.  */
-
-void
-warning (const char *string, ...)
-{
-  va_list args;
-
-  va_start (args, string);
-  vwarning (string, args);
-  va_end (args);
-}
-
 /* Print an error message and return to command level.
    The first argument STRING is the error message, used as a fprintf string,
    and the remaining args are passed as arguments to it.  */
@@ -558,16 +542,6 @@ verror (const char *string, va_list args)
 }
 
 void
-error (const char *string, ...)
-{
-  va_list args;
-
-  va_start (args, string);
-  throw_verror (GENERIC_ERROR, string, args);
-  va_end (args);
-}
-
-void
 error_stream (struct ui_file *stream)
 {
   char *message = ui_file_xstrdup (stream, NULL);
@@ -814,16 +788,6 @@ internal_verror (const char *file, int line, const char *fmt, va_list ap)
   throw_quit (_("Command aborted."));
 }
 
-void
-internal_error (const char *file, int line, const char *string, ...)
-{
-  va_list ap;
-
-  va_start (ap, string);
-  internal_verror (file, line, string, ap);
-  va_end (ap);
-}
-
 static struct internal_problem internal_warning_problem = {
   "internal-warning", 1, internal_problem_ask, 1, internal_problem_ask
 };
diff --git a/gdb/utils.h b/gdb/utils.h
index cc79562..18a95a7 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -269,7 +269,6 @@ extern void fprintf_symbol_filtered (struct ui_file *, const char *,
 
 extern void throw_perror_with_name (enum errors errcode, const char *string)
   ATTRIBUTE_NORETURN;
-extern void perror_with_name (const char *) ATTRIBUTE_NORETURN;
 
 extern void perror_warning_with_name (const char *string);
 
@@ -283,18 +282,8 @@ extern void (*deprecated_error_begin_hook) (void);
 
 extern char *warning_pre_print;
 
-extern void verror (const char *fmt, va_list ap)
-     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
-
-extern void error (const char *fmt, ...)
-     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
-
 extern void error_stream (struct ui_file *) ATTRIBUTE_NORETURN;
 
-extern void internal_verror (const char *file, int line, const char *,
-			     va_list ap)
-     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 0);
-
 extern void internal_vwarning (const char *file, int line,
 			       const char *, va_list ap)
      ATTRIBUTE_PRINTF (3, 0);
@@ -302,10 +291,6 @@ extern void internal_vwarning (const char *file, int line,
 extern void internal_warning (const char *file, int line,
 			      const char *, ...) ATTRIBUTE_PRINTF (3, 4);
 
-extern void warning (const char *, ...) ATTRIBUTE_PRINTF (1, 2);
-
-extern void vwarning (const char *, va_list args) ATTRIBUTE_PRINTF (1, 0);
-
 extern void demangler_vwarning (const char *file, int line,
 			       const char *, va_list ap)
      ATTRIBUTE_PRINTF (3, 0);
-- 
1.7.1

Re: [PATCH 01/11 v5] Introduce common/errors.h

[Doug Evans]

Gary Benson writes:
 > This introduces common/errors.h.  This holds some error- and
 > warning-related declarations that can be used by the code in common.
 > Clients of the "common" code must provide definitions for these
 > functions.
 > 
 > gdb/
 > 2014-08-01  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* common/errors.h: New file.
 > 	* common/errors.c: Likewise.
 > 	* Makefile.in (HFILES_NO_SRCDIR): Add common/errors.h.
 > 	(COMMON_OBS): Add errors.o.
 > 	(errors.o): New rule.
 > 	* common/common-defs.h: Include errors.h.
 > 	* utils.h (perror_with_name, error, verror, warning, vwarning):
 > 	Don't declare.
 > 	* common/common-utils.h: (malloc_failure, internal_error):
 > 	Likewise.
 > 
 > gdb/gdbserver/
 > 2014-08-01  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* Makefile.in (SFILES): Add common/errors.c.
 > 	(OBS): Add errors.o.
 > 	(IPA_OBS): Add errors-ipa.o.
 > 	(errors.o): New rule.
 > 	(errors-ipa.o): Likewise.
 > 	* utils.h (perror_with_name, error, warning): Don't declare.
 > 	* utils.c (warning): Renamed and rewritten as...
 > 	(vwarning): New function.
 > 	(error): Renamed and rewritten as...
 > 	(verror): New function.
 > 	(internal_error): Renamed and rewritten as...
 > 	(internal_verror): New function.
 > [...]
 > diff --git a/gdb/common/errors.c b/gdb/common/errors.c
 > new file mode 100644
 > index 0000000..7d0bb6e
 > --- /dev/null
 > +++ b/gdb/common/errors.c
 > @@ -0,0 +1,61 @@
 > +/* Error reporting facilities.
 > +
 > +   Copyright (C) 1986-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/>.  */
 > +
 > +#ifdef GDBSERVER
 > +#include "server.h"
 > +#else
 > +#include "defs.h"
 > +#endif
 > +#include "errors.h"

Nit: The introductory email to this patch series didn't mention
this instance of #ifdef GDBSERVER.  Needed?

Re: [PATCH 01/11 v5] Introduce common/errors.h

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
> > +#ifdef GDBSERVER
> > +#include "server.h"
> > +#else
> > +#include "defs.h"
> > +#endif
> > +#include "errors.h"
> 
> Nit: The introductory email to this patch series didn't mention
> this instance of #ifdef GDBSERVER.  Needed?

It's removed in patch 8 of the series.
It's only here to ensure the series builds at every step.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 01/11 v5] Introduce common/errors.h

[Doug Evans]

On Wed, Aug 6, 2014 at 9:29 AM, Gary Benson <gbenson@redhat.com> wrote:
> Doug Evans wrote:
>> Gary Benson writes:
>> > +#ifdef GDBSERVER
>> > +#include "server.h"
>> > +#else
>> > +#include "defs.h"
>> > +#endif
>> > +#include "errors.h"
>>
>> Nit: The introductory email to this patch series didn't mention
>> this instance of #ifdef GDBSERVER.  Needed?
>
> It's removed in patch 8 of the series.
> It's only here to ensure the series builds at every step.

[Sometimes I wonder if we're way too anal in this regard.
There's so much work to do, and we bog ourselves down with such
pedantic administrivia.
Sometimes there's a good reason to have a patch series buildable at each step.
But I think the pendulum is tilted far too much towards the pedantic side.
/$0.02]

No worries though.  Thanks for the pointer.

[PATCH 11/11 v5] Remove one GDBSERVER use from linux-waitpid.c

[Gary Benson]

This commit makes nat/linux-waitpid.c include common-defs.h rather
than defs.h or server.h.  A use of GDBSERVER had to be left in to
support some some thread_db debug code.  This hack will be removed
when the Linux thread_db code is unified and made shared.

gdb/
2014-08-01  Gary Benson  <gbenson@redhat.com>

	* nat/linux-waitpid.c: Include common-defs.h.
	Don't include defs.h or server.h.
	(linux_debug) [GDBSERVER]: Declare debug_threads.
---
 gdb/ChangeLog           |    6 ++++++
 gdb/nat/linux-waitpid.c |   10 +++-------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/gdb/nat/linux-waitpid.c b/gdb/nat/linux-waitpid.c
index 53847ac..d04a3af 100644
--- a/gdb/nat/linux-waitpid.c
+++ b/gdb/nat/linux-waitpid.c
@@ -17,13 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#include "signal.h"
-#endif
-
+#include "common-defs.h"
 #include "linux-nat.h"
 #include "linux-waitpid.h"
 #include "gdb_wait.h"
@@ -35,6 +29,8 @@ static inline void
 linux_debug (const char *format, ...)
 {
 #ifdef GDBSERVER
+  extern int debug_threads;
+
   if (debug_threads)
     {
       va_list args;
-- 
1.7.1

Re: [PATCH 11/11 v5] Remove one GDBSERVER use from linux-waitpid.c

[Doug Evans]

Gary Benson writes:
 > This commit makes nat/linux-waitpid.c include common-defs.h rather
 > than defs.h or server.h.  A use of GDBSERVER had to be left in to
 > support some some thread_db debug code.  This hack will be removed
 > when the Linux thread_db code is unified and made shared.
 > 
 > gdb/
 > 2014-08-01  Gary Benson  <gbenson@redhat.com>
 > 
 > 	* nat/linux-waitpid.c: Include common-defs.h.
 > 	Don't include defs.h or server.h.
 > 	(linux_debug) [GDBSERVER]: Declare debug_threads.
 > ---
 >  gdb/ChangeLog           |    6 ++++++
 >  gdb/nat/linux-waitpid.c |   10 +++-------
 >  2 files changed, 9 insertions(+), 7 deletions(-)
 > 
 > diff --git a/gdb/nat/linux-waitpid.c b/gdb/nat/linux-waitpid.c
 > index 53847ac..d04a3af 100644
 > --- a/gdb/nat/linux-waitpid.c
 > +++ b/gdb/nat/linux-waitpid.c
 > @@ -17,13 +17,7 @@
 >     You should have received a copy of the GNU General Public License
 >     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 >  
 > -#ifdef GDBSERVER
 > -#include "server.h"
 > -#else
 > -#include "defs.h"
 > -#include "signal.h"
 > -#endif
 > -
 > +#include "common-defs.h"
 >  #include "linux-nat.h"
 >  #include "linux-waitpid.h"
 >  #include "gdb_wait.h"
 > @@ -35,6 +29,8 @@ static inline void
 >  linux_debug (const char *format, ...)
 >  {
 >  #ifdef GDBSERVER
 > +  extern int debug_threads;
 > +

Recognizing that this is a temporary hack, ok by me.
Can I ask you to add a FIXME here though?

 >    if (debug_threads)
 >      {
 >        va_list args;
 > -- 
 > 1.7.1
 > 

-- 
/dje

Re: [PATCH 11/11 v5] Remove one GDBSERVER use from linux-waitpid.c

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
>  > @@ -35,6 +29,8 @@ static inline void
>  >  linux_debug (const char *format, ...)
>  >  {
>  >  #ifdef GDBSERVER
>  > +  extern int debug_threads;
>  > +
> 
> Recognizing that this is a temporary hack, ok by me.
> Can I ask you to add a FIXME here though?

I added:

  /* FIXME: This conditional code and hacky extern are necessary now,
      but should be removed when GDB's and gdbserver's Linux thread_db
      implementations are unified and moved into the nat directory.  */
  
Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 11/11 v5] Remove one GDBSERVER use from linux-waitpid.c

[Pedro Alves]

On 08/07/2014 01:38 PM, Gary Benson wrote:
> Doug Evans wrote:
>> Gary Benson writes:
>>  > @@ -35,6 +29,8 @@ static inline void
>>  >  linux_debug (const char *format, ...)
>>  >  {
>>  >  #ifdef GDBSERVER
>>  > +  extern int debug_threads;
>>  > +
>>
>> Recognizing that this is a temporary hack, ok by me.
>> Can I ask you to add a FIXME here though?
> 
> I added:
> 
>   /* FIXME: This conditional code and hacky extern are necessary now,
>       but should be removed when GDB's and gdbserver's Linux thread_db
>       implementations are unified and moved into the nat directory.  */

No.

This file (linux-waitpid.c) really has nothing to do with thread_db
(glibc interface).  It's part of the kernel/ptrace layer.
If we wanted this particular debug output enabled on GDB, then it'd
be guarded by 'debug_linux_nat' (set debug lin-lwp 1).  There's really
no good reason for GDB and GDBserver to be different here, other than
the history of this waitpid wrapper and how it ended up used in gdb
as well.

The "threads" in "debug_threads" in gdbserver is just a historic wart.
That global really means "global/generic debug traces enabled".
It was the only debug flag that existed in gdbserver for a long
while, and nobody ever renamed it.

-- 
Thanks,
Pedro Alves