[PATCH 00/15] Common code cleanups

[PATCH 00/15] Common code cleanups

[Gary Benson]

Hi all,

The directories "common", "nat" and "target" contain code shared
between GDB and gdbserver.  Each C file is built at least twice,
once for GDB and once for gdbserver, and many of the files use
"#ifdef GDBSERVER" to source headers from GDB or gdbserver as
appropriate.  This means that while the code is shared, the
supporting definitions are not, and in some cases these are
different in non-trivial ways.  In other places, GDBSERVER
checks are used to cope where GDB and gdbserver do the same thing
in different ways.

This series reduces the number of GDBSERVER checks from 34 to 11.
Tom Tromey started this work back in January, and I've updated it
and extended it a little.  Most of the remaining checks are to
select the correct gnulib config header.

Built and regtested on x86-64 RHEL6.5.  gdbserver cross-built on
linux-mips to check the mips-linux-watch.h changes.

Ok to commit?

Thanks,
Gary

--
http://gbenson.net/

[PATCH 03/15] Make gdbserver CORE_ADDR unsigned

[Gary Benson]

gdbserver defines CORE_ADDR to be signed.  This seems erroneous to
me; and furthermore likely to cause problems in common/, as it is
different from gdb's definition.

gdb/gdbserver/
2014-07-09  Tom Tromey  <tromey@redhat.com>

	* defs.h (CORE_ADDR): Now unsigned.
---
 gdb/gdbserver/ChangeLog |    4 ++++
 gdb/gdbserver/server.h  |    2 +-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 6eb1a16..2d55513 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -87,7 +87,7 @@ typedef unsigned char gdb_byte;
 
 /* FIXME: This should probably be autoconf'd for.  It's an integer type at
    least the size of a (void *).  */
-typedef long long CORE_ADDR;
+typedef unsigned long long CORE_ADDR;
 
 typedef long long LONGEST;
 typedef unsigned long long ULONGEST;
-- 
1.7.1

Re: [PATCH 03/15] Make gdbserver CORE_ADDR unsigned

[Doug Evans]

Gary Benson writes:
 > gdbserver defines CORE_ADDR to be signed.  This seems erroneous to
 > me; and furthermore likely to cause problems in common/, as it is
 > different from gdb's definition.
 > 
 > gdb/gdbserver/
 > 2014-07-09  Tom Tromey  <tromey@redhat.com>
 > 
 > 	* defs.h (CORE_ADDR): Now unsigned.
 > ---
 >  gdb/gdbserver/ChangeLog |    4 ++++
 >  gdb/gdbserver/server.h  |    2 +-
 >  2 files changed, 5 insertions(+), 1 deletions(-)
 > 
 > diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
 > index 6eb1a16..2d55513 100644
 > --- a/gdb/gdbserver/server.h
 > +++ b/gdb/gdbserver/server.h
 > @@ -87,7 +87,7 @@ typedef unsigned char gdb_byte;
 >  
 >  /* FIXME: This should probably be autoconf'd for.  It's an integer type at
 >     least the size of a (void *).  */
 > -typedef long long CORE_ADDR;
 > +typedef unsigned long long CORE_ADDR;
 >  
 >  typedef long long LONGEST;
 >  typedef unsigned long long ULONGEST;
 > -- 
 > 1.7.1
 > 

LGTM, modulo file name spelled wrong in changelog entry.

[PATCH 04/15] 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-07-09  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* common/common-types.h: New file.
	* nat/linux-ptrace.c: Include common-types.h.
	* defs.h: Include common-types.h.
	(gdb_byte, CORE_ADDR, CORE_ADDR_MAX, LONGEST, ULONGEST): Remove.

gdb/gdbserver/
2014-07-09  Tom Tromey  <tromey@redhat.com>

	* server.h: Include common-types.h.  Move gdb_assert.h include
	earlier.  Add static assertion.
	(gdb_byte, CORE_ADDR, LONGEST, ULONGEST): Remove.
---
 gdb/ChangeLog             |    8 ++++++
 gdb/common/common-types.h |   61 +++++++++++++++++++++++++++++++++++++++++++++
 gdb/defs.h                |   30 +---------------------
 gdb/gdbserver/ChangeLog   |    6 ++++
 gdb/gdbserver/server.h    |   14 ++--------
 gdb/nat/linux-ptrace.c    |    1 +
 6 files changed, 80 insertions(+), 40 deletions(-)
 create mode 100644 gdb/common/common-types.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 511279a..d2a9447 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -96,35 +96,7 @@
 
 #include "libiberty.h"
 #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 */
+#include "common-types.h"
 
 #ifndef min
 #define min(a, b) ((a) < (b) ? (a) : (b))
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 2d55513..67214fa 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -77,20 +77,14 @@ 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 "ptid.h"
 #include "buffer.h"
 #include "xml-utils.h"
 #include "gdb_locale.h"
+#include "common-types.h"
+#include "gdb_assert.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;
+gdb_static_assert (sizeof (CORE_ADDR) >= sizeof (void *));
 
 #include "regcache.h"
 #include "gdb/signals.h"
@@ -147,8 +141,6 @@ extern int handle_target_event (int err, gdb_client_data client_data);
 #include "utils.h"
 #include "debug.h"
 
-#include "gdb_assert.h"
-
 /* Maximum number of bytes to read/write at once.  The value here
    is chosen to fill up a packet (the headers account for the 32).  */
 #define MAXBUFBYTES(N) (((N)-32)/2)
diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
index 59c9bfa..e3462ec 100644
--- a/gdb/nat/linux-ptrace.c
+++ b/gdb/nat/linux-ptrace.c
@@ -29,6 +29,7 @@
 #include "buffer.h"
 #include "gdb_assert.h"
 #include "gdb_wait.h"
+#include "common-types.h"
 
 #include <stdint.h>
 
-- 
1.7.1

[PATCH 01/15] 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-07-09  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* common/errors.h: New file.
	* utils.h: Include errors.h.
	(perror_with_name, error, fatal, warning): Don't declare.
	* common/common-utils.h: Include errors.h.
	(malloc_failure, internal_error): Don't declare.

gdb/gdbserver/
2014-07-09  Tom Tromey  <tromey@redhat.com>

	* utils.h: Include errors.h.
	(perror_with_name, error, fatal, warning): Don't declare.
---
 gdb/ChangeLog             |    9 +++++++
 gdb/common/common-utils.h |    5 +---
 gdb/common/errors.h       |   55 +++++++++++++++++++++++++++++++++++++++++++++
 gdb/gdbserver/ChangeLog   |    5 ++++
 gdb/gdbserver/utils.h     |    5 +---
 gdb/utils.h               |   10 +-------
 6 files changed, 72 insertions(+), 17 deletions(-)
 create mode 100644 gdb/common/errors.h

diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 063698d..53be2f8 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -24,6 +24,7 @@
 #include "ansidecl.h"
 #include <stddef.h>
 #include <stdarg.h>
+#include "errors.h"
 
 /* If possible, define FUNCTION_NAME, a macro containing the name of
    the function being defined.  Since this macro may not always be
@@ -43,10 +44,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.h b/gdb/common/errors.h
new file mode 100644
index 0000000..5f998e3
--- /dev/null
+++ b/gdb/common/errors.h
@@ -0,0 +1,55 @@
+/* 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
+
+/* The declarations in this file are, for the time being, separately
+   implemented by gdb and gdbserver.  However they share a common
+   definition so that they can be used by code in common/.  */
+
+/* Like "perror" but throws an exception with the appropriate
+   text.  */
+
+extern void perror_with_name (const char *) ATTRIBUTE_NORETURN;
+
+/* Throw an exception.  */
+
+extern void error (const char *fmt, ...)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
+
+/* Cause a fatal error.  */
+
+extern void fatal (const char *fmt, ...)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
+
+/* Issue a warning.  */
+
+extern void warning (const char *, ...) ATTRIBUTE_PRINTF (1, 2);
+
+/* Internal error.  */
+
+extern void internal_error (const char *file, int line, const char *, ...)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 4);
+
+/* Memory allocation failure.  */
+
+extern void malloc_failure (long size) ATTRIBUTE_NORETURN;
+
+#endif /* COMMON_ERRORS_H */
diff --git a/gdb/gdbserver/utils.h b/gdb/gdbserver/utils.h
index 906924b..819fa35 100644
--- a/gdb/gdbserver/utils.h
+++ b/gdb/gdbserver/utils.h
@@ -20,11 +20,8 @@
 #define UTILS_H
 
 #include "print-utils.h"
+#include "errors.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.h b/gdb/utils.h
index a91f551..d9fe7e3 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -24,6 +24,7 @@
 #include "cleanups.h"
 #include "exceptions.h"
 #include "print-utils.h"
+#include "errors.h"
 
 extern void initialize_utils (void);
 
@@ -269,7 +270,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);
 
@@ -286,17 +286,11 @@ 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 vfatal (const char *fmt, va_list ap)
      ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
 
-extern void fatal (const char *fmt, ...)
-     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
-
 extern void internal_verror (const char *file, int line, const char *,
 			     va_list ap)
      ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 0);
@@ -308,8 +302,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,
-- 
1.7.1

[PATCH 02/15] Remove some GDBSERVER checks from linux-ptrace

[Gary Benson]

This patch removes some GDBSERVER checks from nat/linux-ptrace.c.
Currently the code uses a compile-time check to decide whether some
flags should be used.  This changes the code to instead let users of
the module specify an additional set of flags; and then changes gdb's
linux-nat.c to call this function.  At some later date, when the back
ends are fully merged, we will be able to remove this function again.

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

	* nat/linux-ptrace.c (additional_flags): New global.
	(linux_test_for_tracesysgood, linux_test_for_tracefork): Use
	additional_flags; don't check GDBSERVER.
	(linux_ptrace_set_additional_flags): New function.
	* nat/linux-ptrace.h (linux_ptrace_set_additional_flags):
	Declare.
	* linux-nat.c (_initialize_linux_nat): Call
	linux_ptrace_set_additional_flags.
---
 gdb/ChangeLog          |   12 +++++++++
 gdb/linux-nat.c        |    6 ++++
 gdb/nat/linux-ptrace.c |   64 ++++++++++++++++++++++++-----------------------
 gdb/nat/linux-ptrace.h |    1 +
 4 files changed, 52 insertions(+), 31 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 0ab0362..5c7a21a 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -5025,6 +5025,12 @@ Enables printf debugging output."),
   sigdelset (&suspend_mask, SIGCHLD);
 
   sigemptyset (&blocked_mask);
+
+  linux_ptrace_set_additional_flags (PTRACE_O_TRACESYSGOOD
+				     | PTRACE_O_TRACEVFORKDONE
+				     | PTRACE_O_TRACEVFORK
+				     | PTRACE_O_TRACEFORK
+				     | PTRACE_O_TRACEEXEC);
 }
 \f
 
diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
index 3ad2113..59c9bfa 100644
--- a/gdb/nat/linux-ptrace.c
+++ b/gdb/nat/linux-ptrace.c
@@ -37,6 +37,10 @@
    there are no supported features.  */
 static int current_ptrace_options = -1;
 
+/* Additional flags to test.  */
+
+static int additional_flags;
+
 /* Find all possible reasons we could fail to attach PID and append
    these as strings to the already initialized BUFFER.  '\0'
    termination of BUFFER must be done by the caller.  */
@@ -359,16 +363,15 @@ linux_check_ptrace_features (void)
 static void
 linux_test_for_tracesysgood (int child_pid)
 {
-#ifdef GDBSERVER
-  /* gdbserver does not support PTRACE_O_TRACESYSGOOD.  */
-#else
-  int ret;
+  if ((additional_flags & PTRACE_O_TRACESYSGOOD) != 0)
+    {
+      int ret;
 
-  ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
-		(PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
-  if (ret == 0)
-    current_ptrace_options |= PTRACE_O_TRACESYSGOOD;
-#endif
+      ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
+		    (PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
+      if (ret != 0)
+	additional_flags &= ~PTRACE_O_TRACESYSGOOD;
+    }
 }
 
 /* Determine if PTRACE_O_TRACEFORK can be used to follow fork
@@ -388,16 +391,15 @@ linux_test_for_tracefork (int child_pid)
   if (ret != 0)
     return;
 
-#ifdef GDBSERVER
-  /* gdbserver does not support PTRACE_O_TRACEVFORKDONE yet.  */
-#else
-  /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
-  ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
-		(PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
-				    | PTRACE_O_TRACEVFORKDONE));
-  if (ret == 0)
-    current_ptrace_options |= PTRACE_O_TRACEVFORKDONE;
-#endif
+  if ((additional_flags & PTRACE_O_TRACEVFORKDONE) != 0)
+    {
+      /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
+      ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
+		    (PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
+					| PTRACE_O_TRACEVFORKDONE));
+      if (ret != 0)
+	additional_flags &= ~PTRACE_O_TRACEVFORKDONE;
+    }
 
   /* Setting PTRACE_O_TRACEFORK did not cause an error, however we
      don't know for sure that the feature is available; old
@@ -433,18 +435,7 @@ linux_test_for_tracefork (int child_pid)
 
 	  /* We got the PID from the grandchild, which means fork
 	     tracing is supported.  */
-#ifdef GDBSERVER
-	  /* Do not enable all the options for now since gdbserver does not
-	     properly support them.  This restriction will be lifted when
-	     gdbserver is augmented to support them.  */
-	  current_ptrace_options |= PTRACE_O_TRACECLONE;
-#else
-	  current_ptrace_options |= PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK
-	    | PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC;
-
-	  /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to
-	     support read-only process state.  */
-#endif
+	  current_ptrace_options |= PTRACE_O_TRACECLONE | additional_flags;
 
 	  /* Do some cleanup and kill the grandchild.  */
 	  my_waitpid (second_pid, &second_status, 0);
@@ -551,3 +542,14 @@ linux_ptrace_init_warnings (void)
 
   linux_ptrace_test_ret_to_nx ();
 }
+
+/* Set additional ptrace flags to use.  Some such flags may be checked
+   by the implementation above.  This function must be called before
+   any other function in this file; otherwise the flags may not take
+   effect appropriately.  */
+
+void
+linux_ptrace_set_additional_flags (int flags)
+{
+  additional_flags = flags;
+}
diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
index cffb5ce..41b3198 100644
--- a/gdb/nat/linux-ptrace.h
+++ b/gdb/nat/linux-ptrace.h
@@ -91,5 +91,6 @@ extern int linux_supports_tracefork (void);
 extern int linux_supports_traceclone (void);
 extern int linux_supports_tracevforkdone (void);
 extern int linux_supports_tracesysgood (void);
+extern void linux_ptrace_set_additional_flags (int);
 
 #endif /* COMMON_LINUX_PTRACE_H */
-- 
1.7.1

Re: [PATCH 02/15] Remove some GDBSERVER checks from linux-ptrace

[Breazeal, Don]

Hi,

On 7/9/2014 3:37 AM, Gary Benson wrote:
> This patch removes some GDBSERVER checks from nat/linux-ptrace.c.
> Currently the code uses a compile-time check to decide whether some
> flags should be used.  This changes the code to instead let users of
> the module specify an additional set of flags; and then changes gdb's
> linux-nat.c to call this function.  At some later date, when the back
> ends are fully merged, we will be able to remove this function again.

I just want to point out that I'm doing something similar as part of my
work on follow-fork for extended remote, based on Tom's suggestion here:

https://sourceware.org/ml/gdb-patches/2014-05/msg00486.html

In my case it adds "base" options and the capability to set the ptrace
options to the "additional" options at the caller's discretion, such as
when gdbserver receives the enable-extended-mode packet.  So, no
objection, just wanted to let you know that I hope to submit somewhat
different changes to the same code soon.

--Don
> 
> gdb/
> 2014-07-09  Tom Tromey  <tromey@redhat.com>
> 	    Gary Benson  <gbenson@redhat.com>
> 
> 	* nat/linux-ptrace.c (additional_flags): New global.
> 	(linux_test_for_tracesysgood, linux_test_for_tracefork): Use
> 	additional_flags; don't check GDBSERVER.
> 	(linux_ptrace_set_additional_flags): New function.
> 	* nat/linux-ptrace.h (linux_ptrace_set_additional_flags):
> 	Declare.
> 	* linux-nat.c (_initialize_linux_nat): Call
> 	linux_ptrace_set_additional_flags.
> ---
>  gdb/ChangeLog          |   12 +++++++++
>  gdb/linux-nat.c        |    6 ++++
>  gdb/nat/linux-ptrace.c |   64 ++++++++++++++++++++++++-----------------------
>  gdb/nat/linux-ptrace.h |    1 +
>  4 files changed, 52 insertions(+), 31 deletions(-)
> 
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 0ab0362..5c7a21a 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -5025,6 +5025,12 @@ Enables printf debugging output."),
>    sigdelset (&suspend_mask, SIGCHLD);
>  
>    sigemptyset (&blocked_mask);
> +
> +  linux_ptrace_set_additional_flags (PTRACE_O_TRACESYSGOOD
> +				     | PTRACE_O_TRACEVFORKDONE
> +				     | PTRACE_O_TRACEVFORK
> +				     | PTRACE_O_TRACEFORK
> +				     | PTRACE_O_TRACEEXEC);
>  }
>  \f
>  
> diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
> index 3ad2113..59c9bfa 100644
> --- a/gdb/nat/linux-ptrace.c
> +++ b/gdb/nat/linux-ptrace.c
> @@ -37,6 +37,10 @@
>     there are no supported features.  */
>  static int current_ptrace_options = -1;
>  
> +/* Additional flags to test.  */
> +
> +static int additional_flags;
> +
>  /* Find all possible reasons we could fail to attach PID and append
>     these as strings to the already initialized BUFFER.  '\0'
>     termination of BUFFER must be done by the caller.  */
> @@ -359,16 +363,15 @@ linux_check_ptrace_features (void)
>  static void
>  linux_test_for_tracesysgood (int child_pid)
>  {
> -#ifdef GDBSERVER
> -  /* gdbserver does not support PTRACE_O_TRACESYSGOOD.  */
> -#else
> -  int ret;
> +  if ((additional_flags & PTRACE_O_TRACESYSGOOD) != 0)
> +    {
> +      int ret;
>  
> -  ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
> -		(PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
> -  if (ret == 0)
> -    current_ptrace_options |= PTRACE_O_TRACESYSGOOD;
> -#endif
> +      ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
> +		    (PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
> +      if (ret != 0)
> +	additional_flags &= ~PTRACE_O_TRACESYSGOOD;
> +    }
>  }
>  
>  /* Determine if PTRACE_O_TRACEFORK can be used to follow fork
> @@ -388,16 +391,15 @@ linux_test_for_tracefork (int child_pid)
>    if (ret != 0)
>      return;
>  
> -#ifdef GDBSERVER
> -  /* gdbserver does not support PTRACE_O_TRACEVFORKDONE yet.  */
> -#else
> -  /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
> -  ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
> -		(PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
> -				    | PTRACE_O_TRACEVFORKDONE));
> -  if (ret == 0)
> -    current_ptrace_options |= PTRACE_O_TRACEVFORKDONE;
> -#endif
> +  if ((additional_flags & PTRACE_O_TRACEVFORKDONE) != 0)
> +    {
> +      /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
> +      ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
> +		    (PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
> +					| PTRACE_O_TRACEVFORKDONE));
> +      if (ret != 0)
> +	additional_flags &= ~PTRACE_O_TRACEVFORKDONE;
> +    }
>  
>    /* Setting PTRACE_O_TRACEFORK did not cause an error, however we
>       don't know for sure that the feature is available; old
> @@ -433,18 +435,7 @@ linux_test_for_tracefork (int child_pid)
>  
>  	  /* We got the PID from the grandchild, which means fork
>  	     tracing is supported.  */
> -#ifdef GDBSERVER
> -	  /* Do not enable all the options for now since gdbserver does not
> -	     properly support them.  This restriction will be lifted when
> -	     gdbserver is augmented to support them.  */
> -	  current_ptrace_options |= PTRACE_O_TRACECLONE;
> -#else
> -	  current_ptrace_options |= PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK
> -	    | PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC;
> -
> -	  /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to
> -	     support read-only process state.  */
> -#endif
> +	  current_ptrace_options |= PTRACE_O_TRACECLONE | additional_flags;
>  
>  	  /* Do some cleanup and kill the grandchild.  */
>  	  my_waitpid (second_pid, &second_status, 0);
> @@ -551,3 +542,14 @@ linux_ptrace_init_warnings (void)
>  
>    linux_ptrace_test_ret_to_nx ();
>  }
> +
> +/* Set additional ptrace flags to use.  Some such flags may be checked
> +   by the implementation above.  This function must be called before
> +   any other function in this file; otherwise the flags may not take
> +   effect appropriately.  */
> +
> +void
> +linux_ptrace_set_additional_flags (int flags)
> +{
> +  additional_flags = flags;
> +}
> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
> index cffb5ce..41b3198 100644
> --- a/gdb/nat/linux-ptrace.h
> +++ b/gdb/nat/linux-ptrace.h
> @@ -91,5 +91,6 @@ extern int linux_supports_tracefork (void);
>  extern int linux_supports_traceclone (void);
>  extern int linux_supports_tracevforkdone (void);
>  extern int linux_supports_tracesysgood (void);
> +extern void linux_ptrace_set_additional_flags (int);
>  
>  #endif /* COMMON_LINUX_PTRACE_H */
> 

Re: [PATCH 02/15] Remove some GDBSERVER checks from linux-ptrace

[Gary Benson]

Breazeal, Don wrote:
> On 7/9/2014 3:37 AM, Gary Benson wrote:
> > This patch removes some GDBSERVER checks from nat/linux-ptrace.c.
> > Currently the code uses a compile-time check to decide whether
> > some flags should be used.  This changes the code to instead let
> > users of the module specify an additional set of flags; and then
> > changes gdb's linux-nat.c to call this function.  At some later
> > date, when the back ends are fully merged, we will be able to
> > remove this function again.
> 
> I just want to point out that I'm doing something similar as part
> of my work on follow-fork for extended remote, based on Tom's
> suggestion here:
> 
> https://sourceware.org/ml/gdb-patches/2014-05/msg00486.html
> 
> In my case it adds "base" options and the capability to set the
> ptrace options to the "additional" options at the caller's
> discretion, such as when gdbserver receives the enable-extended-mode
> packet.  So, no objection, just wanted to let you know that I hope
> to submit somewhat different changes to the same code soon.

The patch I posted is basically an updated version of the patch Tom
posted there--the only change IIRC is that linux-ptrace.c is now in
"nat" rather than "common".

Do you need me to change anything about this parch, or is it ok for
you to work with?

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 02/15] Remove some GDBSERVER checks from linux-ptrace

[Breazeal, Don]

On 7/9/2014 11:20 AM, Gary Benson wrote:
> Breazeal, Don wrote:
>> On 7/9/2014 3:37 AM, Gary Benson wrote:
>>> This patch removes some GDBSERVER checks from nat/linux-ptrace.c.
>>> Currently the code uses a compile-time check to decide whether
>>> some flags should be used.  This changes the code to instead let
>>> users of the module specify an additional set of flags; and then
>>> changes gdb's linux-nat.c to call this function.  At some later
>>> date, when the back ends are fully merged, we will be able to
>>> remove this function again.
>>
>> I just want to point out that I'm doing something similar as part
>> of my work on follow-fork for extended remote, based on Tom's
>> suggestion here:
>>
>> https://sourceware.org/ml/gdb-patches/2014-05/msg00486.html
>>
>> In my case it adds "base" options and the capability to set the
>> ptrace options to the "additional" options at the caller's
>> discretion, such as when gdbserver receives the enable-extended-mode
>> packet.  So, no objection, just wanted to let you know that I hope
>> to submit somewhat different changes to the same code soon.
> 
> The patch I posted is basically an updated version of the patch Tom
> posted there--the only change IIRC is that linux-ptrace.c is now in
> "nat" rather than "common".
> 
> Do you need me to change anything about this parch, or is it ok for
> you to work with?

It is OK for me.
thanks
--Don

> 
> Thanks,
> Gary
> 

Re: [PATCH 02/15] Remove some GDBSERVER checks from linux-ptrace

[Doug Evans]

Gary Benson writes:
 > This patch removes some GDBSERVER checks from nat/linux-ptrace.c.
 > Currently the code uses a compile-time check to decide whether some
 > flags should be used.  This changes the code to instead let users of
 > the module specify an additional set of flags; and then changes gdb's
 > linux-nat.c to call this function.  At some later date, when the back
 > ends are fully merged, we will be able to remove this function again.
 > 
 > gdb/
 > 2014-07-09  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* nat/linux-ptrace.c (additional_flags): New global.
 > 	(linux_test_for_tracesysgood, linux_test_for_tracefork): Use
 > 	additional_flags; don't check GDBSERVER.
 > 	(linux_ptrace_set_additional_flags): New function.
 > 	* nat/linux-ptrace.h (linux_ptrace_set_additional_flags):
 > 	Declare.
 > 	* linux-nat.c (_initialize_linux_nat): Call
 > 	linux_ptrace_set_additional_flags.

Hi.  Recognizing that this is a temporary hack, the patch is ok with me.
One nit below.

 > @@ -433,18 +435,7 @@ linux_test_for_tracefork (int child_pid)
 >  
 >  	  /* We got the PID from the grandchild, which means fork
 >  	     tracing is supported.  */
 > -#ifdef GDBSERVER
 > -	  /* Do not enable all the options for now since gdbserver does not
 > -	     properly support them.  This restriction will be lifted when
 > -	     gdbserver is augmented to support them.  */
 > -	  current_ptrace_options |= PTRACE_O_TRACECLONE;
 > -#else
 > -	  current_ptrace_options |= PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK
 > -	    | PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC;
 > -
 > -	  /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to
 > -	     support read-only process state.  */
 > -#endif
 > +	  current_ptrace_options |= PTRACE_O_TRACECLONE | additional_flags;
 >  
 >  	  /* Do some cleanup and kill the grandchild.  */
 >  	  my_waitpid (second_pid, &second_status, 0);

I can't tell if the PTRACE_O_TRACEEXIT comment was accidentally
or intentionally dropped.
I'm not sure it's important enough to keep, but it would be
good to verify its deletion was intentional.

Re: [PATCH 02/15] Remove some GDBSERVER checks from linux-ptrace

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
>  > @@ -433,18 +435,7 @@ linux_test_for_tracefork (int child_pid)
>  >  
>  >  	  /* We got the PID from the grandchild, which means fork
>  >  	     tracing is supported.  */
>  > -#ifdef GDBSERVER
>  > -	  /* Do not enable all the options for now since gdbserver does not
>  > -	     properly support them.  This restriction will be lifted when
>  > -	     gdbserver is augmented to support them.  */
>  > -	  current_ptrace_options |= PTRACE_O_TRACECLONE;
>  > -#else
>  > -	  current_ptrace_options |= PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK
>  > -	    | PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC;
>  > -
>  > -	  /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to
>  > -	     support read-only process state.  */
>  > -#endif
>  > +	  current_ptrace_options |= PTRACE_O_TRACECLONE | additional_flags;
>  >  
>  >  	  /* Do some cleanup and kill the grandchild.  */
>  >  	  my_waitpid (second_pid, &second_status, 0);
> 
> I can't tell if the PTRACE_O_TRACEEXIT comment was accidentally or
> intentionally dropped.  I'm not sure it's important enough to keep,
> but it would be good to verify its deletion was intentional.

I don't know, so unless Tom has any objections I will reinstate
the comment before the call to linux_ptrace_set_additional_flags
in linux-nat.c.

Thanks,
Gary

-- 
http://gbenson.net/

[PATCH 11/15] More target unification

[Gary Benson]

This unifies a few more top-level target functions -- target_resume,
target_wait, and target_stop -- and the declaration of the variable
"non_stop".  This follows the usual pattern, where clients of "common"
are expected to define the objects appropriately, thus simplifying
common/agent.c a bit more.

gdb/
2014-07-09  Tom Tromey  <tromey@redhat.com>

	* target/target.h (target_resume, target_wait, target_stop)
	(non_stop): Moved from target.h.
	* target.h (target_resume, target_wait, target_stop, non_stop):
	Move to target/target.h.
	* common/agent.c (agent_run_command): Always use target_resume,
	target_stop, and target_wait.

gdb/gdbserver/
2014-07-09  Tom Tromey  <tromey@redhat.com>

	* target.c (target_wait, target_stop, target_resume): New
	functions.
---
 gdb/ChangeLog           |    9 +++++++++
 gdb/common/agent.c      |   27 +--------------------------
 gdb/gdbserver/ChangeLog |    5 +++++
 gdb/gdbserver/target.c  |   34 ++++++++++++++++++++++++++++++++++
 gdb/target.h            |   31 -------------------------------
 gdb/target/target.h     |   38 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 87 insertions(+), 57 deletions(-)

diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index 52de3d4..0fde2fd 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -230,18 +230,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)
@@ -277,25 +266,11 @@ 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;
     }
 
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index 44e1e11..f93163e 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -134,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 = GDB_SIGNAL_0;
+  (*the_target->resume) (&resume_info, 1);
+}
+
 int
 start_non_stop (int nonstop)
 {
diff --git a/gdb/target.h b/gdb/target.h
index 8a2faca..2a4783c 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1191,31 +1191,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);
@@ -1557,12 +1532,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
index b3bd719..cb96181 100644
--- a/gdb/target/target.h
+++ b/gdb/target/target.h
@@ -22,6 +22,37 @@
 
 /* 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);
+
 extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
 			       ssize_t len);
 
@@ -31,4 +62,11 @@ extern int target_read_uint32 (CORE_ADDR memaddr, unsigned int *result);
 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 11/15] More target unification

[Doug Evans]

Gary Benson writes:
 > This unifies a few more top-level target functions -- target_resume,
 > target_wait, and target_stop -- and the declaration of the variable
 > "non_stop".  This follows the usual pattern, where clients of "common"
 > are expected to define the objects appropriately, thus simplifying
 > common/agent.c a bit more.
 > 
 > gdb/
 > 2014-07-09  Tom Tromey  <tromey@redhat.com>
 > 
 > 	* target/target.h (target_resume, target_wait, target_stop)
 > 	(non_stop): Moved from target.h.
 > 	* target.h (target_resume, target_wait, target_stop, non_stop):
 > 	Move to target/target.h.
 > 	* common/agent.c (agent_run_command): Always use target_resume,
 > 	target_stop, and target_wait.
 > 
 > gdb/gdbserver/
 > 2014-07-09  Tom Tromey  <tromey@redhat.com>
 > 
 > 	* target.c (target_wait, target_stop, target_resume): New
 > 	functions.
 > [...]
 >
 > diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
 > index 44e1e11..f93163e 100644
 > --- a/gdb/gdbserver/target.c
 > +++ b/gdb/gdbserver/target.c
 > @@ -134,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 = GDB_SIGNAL_0;
 > +  (*the_target->resume) (&resume_info, 1);
 > +}

I'm guessing the ignoring of signal is an oversight, right?

Re: [PATCH 11/15] More target unification

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
> > +/* 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 = GDB_SIGNAL_0;
> > +  (*the_target->resume) (&resume_info, 1);
> > +}
> 
> I'm guessing the ignoring of signal is an oversight, right?

Looks that way, I'll fix it.
Good catch BTW!

Cheers,
Gary

-- 
http://gbenson.net/

[PATCH 15/15] Finally remove GDBSERVER (mostly) from linux-btrace.c

[Gary Benson]

This applies the usual GDBSERVER treatment to linux-btrace.c.
Now it is used only to find the gnulib header.

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

	* nat/linux-btrace.c: Don't include defs.h or server.h.
	* nat/linux-btrace.h (struct target_ops): Declare.
---
 gdb/ChangeLog          |    6 ++++++
 gdb/nat/linux-btrace.c |   13 ++++++++++---
 gdb/nat/linux-btrace.h |    2 ++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index dd7744b..3fd80bb 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -19,17 +19,24 @@
    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 <config.h>
+
 #ifdef GDBSERVER
-#include "server.h"
+#include "build-gnulib-gdbserver/config.h"
 #else
-#include "defs.h"
+#include "build-gnulib/config.h"
 #endif
 
+#include "common-types.h"
+#include "common-utils.h"
+#include "target/waitstatus.h"
+#include "errors.h"
+#include "gdb_locale.h"
+#include "gdb_signals.h"
 #include "linux-btrace.h"
 #include "common-utils.h"
 #include "gdb_assert.h"
 #include "regcache.h"
-#include "gdbthread.h"
 #include "gdb_wait.h"
 #include "i386-cpuid.h"
 
diff --git a/gdb/nat/linux-btrace.h b/gdb/nat/linux-btrace.h
index 85cd88b..708476f 100644
--- a/gdb/nat/linux-btrace.h
+++ b/gdb/nat/linux-btrace.h
@@ -22,6 +22,8 @@
 #ifndef LINUX_BTRACE_H
 #define LINUX_BTRACE_H
 
+struct target_ops;
+
 #include "btrace-common.h"
 #include "config.h"
 #include "vec.h"
-- 
1.7.1

[PATCH 09/15] Mostly remove GDBSERVER from linux-waitpid.c

[Gary Benson]

This commit mostly removes the use of GDBSERVER from
nat/linux-waitpid.c.  A use remains for some debugging
code that I will remove when the Linux thread_db code
is refactored.

gdb/
2014-07-09  Gary Benson  <gbenson@redhat.com>

	* configure.ac [AC_CHECK_HEADERS] <errno.h>: New check.
	* configure: Regenerate.
	* config.in: Likewise.
	* nat/linux-waitpid.c: Don't include server.h or defs.h.
	(linux_debug) [debug_threads]: New declaration.
---
 gdb/ChangeLog           |    8 ++++++++
 gdb/config.in           |    3 +++
 gdb/configure           |    2 +-
 gdb/configure.ac        |    2 +-
 gdb/nat/linux-waitpid.c |   13 ++++++++-----
 5 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/gdb/config.in b/gdb/config.in
index 8585b49..10b9887 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -129,6 +129,9 @@
 /* Define to 1 if you have the <elf_hp.h> header file. */
 #undef HAVE_ELF_HP_H
 
+/* Define to 1 if you have the <errno.h> header file. */
+#undef HAVE_ERRNO_H
+
 /* Define to 1 if your system has the etext variable. */
 #undef HAVE_ETEXT
 
diff --git a/gdb/configure b/gdb/configure
index a4c0a8c..45f3f5f 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -9307,7 +9307,7 @@ for ac_header in nlist.h machine/reg.h poll.h sys/poll.h proc_service.h \
 		  sys/reg.h sys/debugreg.h sys/select.h sys/syscall.h \
 		  termios.h termio.h \
 		  sgtty.h elf_hp.h \
-		  dlfcn.h
+		  dlfcn.h errno.h
 do :
   as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh`
 ac_fn_c_check_header_mongrel "$LINENO" "$ac_header" "$as_ac_Header" "$ac_includes_default"
diff --git a/gdb/configure.ac b/gdb/configure.ac
index a2ac15f..5f2ace4 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1254,7 +1254,7 @@ AC_CHECK_HEADERS([nlist.h machine/reg.h poll.h sys/poll.h proc_service.h \
 		  sys/reg.h sys/debugreg.h sys/select.h sys/syscall.h \
 		  termios.h termio.h \
 		  sgtty.h elf_hp.h \
-		  dlfcn.h])
+		  dlfcn.h errno.h])
 AC_CHECK_HEADERS(sys/proc.h, [], [],
 [#if HAVE_SYS_PARAM_H
 # include <sys/param.h>
diff --git a/gdb/nat/linux-waitpid.c b/gdb/nat/linux-waitpid.c
index 5159f03..b97ae58 100644
--- a/gdb/nat/linux-waitpid.c
+++ b/gdb/nat/linux-waitpid.c
@@ -17,11 +17,12 @@
    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"
+#include "config.h"
+
+#include <stdio.h>
+#include <stdarg.h>
+#ifdef HAVE_ERRNO_H
+#include <errno.h>
 #endif
 
 #include "linux-nat.h"
@@ -37,6 +38,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 09/15] Mostly remove GDBSERVER from linux-waitpid.c

[Doug Evans]

Gary Benson writes:
 > This commit mostly removes the use of GDBSERVER from
 > nat/linux-waitpid.c.  A use remains for some debugging
 > code that I will remove when the Linux thread_db code
 > is refactored.
 > 
 > gdb/
 > 2014-07-09  Gary Benson  <gbenson@redhat.com>
 > 
 > 	* configure.ac [AC_CHECK_HEADERS] <errno.h>: New check.
 > 	* configure: Regenerate.
 > 	* config.in: Likewise.
 > 	* nat/linux-waitpid.c: Don't include server.h or defs.h.
 > 	(linux_debug) [debug_threads]: New declaration.

gdb includes errno.h unconditionally,
so the test is for some gdbserver configs (right?).

It's going to be confusing to keep all the unconditional
inclusions of errno.h and yet seeing it tested for in configure.ac.
It would be good to document why things are the way they are.
E.g., add a comment to configure.ac or some such saying the test
is needed by common code used by gdb and gdbserver.

OTOH, I see there is gnulib/import/errno.in.h.
Can we solve this with gnulib and thus make errno.h
unconditionally includable everywhere?

Re: [PATCH 09/15] Mostly remove GDBSERVER from linux-waitpid.c

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
> > This commit mostly removes the use of GDBSERVER from
> > nat/linux-waitpid.c.  A use remains for some debugging
> > code that I will remove when the Linux thread_db code
> > is refactored.
> > 
> > gdb/
> > 2014-07-09  Gary Benson  <gbenson@redhat.com>
> > 
> > 	* configure.ac [AC_CHECK_HEADERS] <errno.h>: New check.
> > 	* configure: Regenerate.
> > 	* config.in: Likewise.
> > 	* nat/linux-waitpid.c: Don't include server.h or defs.h.
> > 	(linux_debug) [debug_threads]: New declaration.
> 
> gdb includes errno.h unconditionally, so the test is for some
> gdbserver configs (right?).

I think so.

> It's going to be confusing to keep all the unconditional inclusions
> of errno.h and yet seeing it tested for in configure.ac.  It would
> be good to document why things are the way they are.  E.g., add a
> comment to configure.ac or some such saying the test is needed by
> common code used by gdb and gdbserver.
> 
> OTOH, I see there is gnulib/import/errno.in.h.
> Can we solve this with gnulib and thus make errno.h
> unconditionally includable everywhere?

I see gdb/gdbserver/linux-low.c includes errno.h unconditionally.
I'm going to assume from that that linux-waitpid.c can do the same.
I'll remove the conditionals from there and revert the configury
changes.

Thanks,
Gary

--
http://gbenson.net/

[PATCH 08/15] Make btrace-common.h not use GDBSERVER

[Gary Benson]

This changes btrace-common.h not to include defs.h or server.h.
Instead it moves the defs.h include to btrace.c.

gdb/
2014-07-09  Tom Tromey  <tromey@redhat.com>

	* btrace.c: Include defs.h.
	* common/btrace-common.h: Don't include server.h or defs.h.
---
 gdb/ChangeLog              |    5 +++++
 gdb/btrace.c               |    1 +
 gdb/common/btrace-common.h |    6 ------
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 87a171e..b5c3c26 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -19,6 +19,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/>.  */
 
+#include "defs.h"
 #include "btrace.h"
 #include "gdbthread.h"
 #include "exceptions.h"
diff --git a/gdb/common/btrace-common.h b/gdb/common/btrace-common.h
index 25617bb..339e684 100644
--- a/gdb/common/btrace-common.h
+++ b/gdb/common/btrace-common.h
@@ -26,12 +26,6 @@
    inferior.  For presentation purposes, the branch trace is represented as a
    list of sequential control-flow blocks, one such list per thread.  */
 
-#ifdef GDBSERVER
-#  include "server.h"
-#else
-#  include "defs.h"
-#endif
-
 #include "vec.h"
 
 /* A branch trace block.
-- 
1.7.1

Re: [PATCH 08/15] Make btrace-common.h not use GDBSERVER

[Doug Evans]

Gary Benson writes:
 > This changes btrace-common.h not to include defs.h or server.h.
 > Instead it moves the defs.h include to btrace.c.
 > 
 > gdb/
 > 2014-07-09  Tom Tromey  <tromey@redhat.com>
 > 
 > 	* btrace.c: Include defs.h.
 > 	* common/btrace-common.h: Don't include server.h or defs.h.

LGTM

[PATCH 10/15] Add target/target.h

[Gary Benson]

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

gdb/
2014-07-09  Tom Tromey  <tromey@redhat.com>

	* target.h: Include target/target.h.
	(target_read_memory, target_write_memory): Don't declare.
	* target.c (target_read_uint32): New function.
	* common/agent.c: Don't include server.h or defs.h; update
	includes.
	(agent_get_helper_thread_id): Use target_read_uint32.
	(agent_run_command): Always use target_write_memory,
	target_read_memory.
	(agent_capability_check): Use target_read_uint32.
	* target/target.h: New file.

gdb/gdbserver/
2014-07-09  Tom Tromey  <tromey@redhat.com>

	* target.c (target_read_memory, target_read_uint32)
	(target_write_memory): New functions.
---
 gdb/ChangeLog           |   13 +++++++++++
 gdb/common/agent.c      |   56 ++++++++++++----------------------------------
 gdb/gdbserver/ChangeLog |    5 ++++
 gdb/gdbserver/target.c  |   24 ++++++++++++++++++++
 gdb/target.c            |   16 +++++++++++++
 gdb/target.h            |    7 +-----
 gdb/target/target.h     |   34 ++++++++++++++++++++++++++++
 7 files changed, 108 insertions(+), 47 deletions(-)
 create mode 100644 gdb/target/target.h

diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index 549b784..52de3d4 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -26,6 +26,16 @@
 #include "objfiles.h"
 #endif
 
+#include <stdarg.h>
+#include <errno.h>
+
+#include "common-types.h"
+#include "target/target.h"
+#include "errors.h"
+#include "ptid.h"
+#include "gdb_locale.h"
+#include "common-utils.h"
+
 #include <string.h>
 #include <unistd.h>
 #include "agent.h"
@@ -128,23 +138,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;
@@ -222,13 +218,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)
     {
@@ -310,13 +301,8 @@ agent_run_command (int pid, const char *cmd, int len)
 
   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;
@@ -336,20 +322,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..44e1e11 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)
diff --git a/gdb/target.c b/gdb/target.c
index c9c5e4b..0796059 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1306,6 +1306,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 8bf160c..8a2faca 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"
@@ -1278,9 +1279,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);
 
@@ -1288,9 +1286,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);
 
diff --git a/gdb/target/target.h b/gdb/target/target.h
new file mode 100644
index 0000000..b3bd719
--- /dev/null
+++ b/gdb/target/target.h
@@ -0,0 +1,34 @@
+/* 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
+
+/* This header is a stopgap until more code is shared.  */
+
+extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
+			       ssize_t len);
+
+
+extern int target_read_uint32 (CORE_ADDR memaddr, unsigned int *result);
+
+extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
+				ssize_t len);
+
+#endif /* TARGET_COMMON_H */
-- 
1.7.1

Re: [PATCH 10/15] Add target/target.h

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

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

Gary> +/* See target/target.h.  */
Gary> +
Gary> +int
Gary> +target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)

These comments refer to docs I never got around to writing... :

Gary> +extern int target_read_uint32 (CORE_ADDR memaddr, unsigned int *result);
Gary> +
Gary> +extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
Gary> +				ssize_t len);


Tom

Re: [PATCH 10/15] Add target/target.h

[Gary Benson]

Tom Tromey wrote:
> >>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
> 
> Gary> This adds target/target.h.  This file declares some functions
> Gary> that the "common" code can use and that the clients must
> Gary> implement.  It also changes code in common to use these
> Gary> functions.
> 
> Gary> +/* See target/target.h.  */
> Gary> +
> Gary> +int
> Gary> +target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
> 
> These comments refer to docs I never got around to writing... :
> 
> Gary> +extern int target_read_uint32 (CORE_ADDR memaddr, unsigned int *result);
> Gary> +
> Gary> +extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
> Gary> +				ssize_t len);

How does this sound?

  /* 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);
  
Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 10/15] Add target/target.h

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

Tom> These comments refer to docs I never got around to writing... :

Gary> How does this sound?
[...]

They look good to me, thank you.

Tom

[PATCH 06/15] Remove simple GDBSERVER uses from common, nat and target

[Gary Benson]

This removes various simple GDBSERVER uses from common, nat and
target.  The simple uses are just cases where the code includes defs.h
or server.h depending on GDBSERVER.  Instead, now the files include
the headers that they require.  Unfortunately we still need to check
GDBSERVER for some files to decide which gnulib config header to
import, but this is a step in the right direction.

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

	* common/buffer.c: Don't include server.h or defs.h; update
	includes.
	* common/common-utils.c: Don't include server.h or defs.h; update
	includes.
	* common/filestuff.c: Don't include server.h or defs.h; update
	includes.
	* common/filestuff.h: Include stdio.h.
	* common/format.c: Don't include server.h or defs.h; update
	includes.
	* common/gdb_vecs.c: Don't include server.h or defs.h; update
	includes.
	* common/print-utils.c: Don't include server.h or defs.h; update
	includes.
	* common/rsp-low.c: Don't include server.h or defs.h; update
	includes.
	* common/signals.c: Don't include server.h or defs.h; update
	includes.
	* common/vec.c: Don't include server.h or defs.h; update includes.
	* common/xml-utils.c: Don't include server.h or defs.h; update
	includes.
	* nat/linux-osdata.c: Don't include server.h or defs.h; update
	includes.
	* nat/linux-procfs.c: Don't include server.h or defs.h; update
	includes.
	* nat/linux-ptrace.c: Don't include server.h or defs.h; update
	includes.
	* nat/mips-linux-watch.h: Don't include server.h or defs.h; update
	includes.
	* target/waitstatus.c: Don't include server.h or defs.h; update
	includes.
---
 gdb/ChangeLog              |   34 ++++++++++++++++++++++++++++++++++
 gdb/common/buffer.c        |    9 ++++-----
 gdb/common/common-utils.c  |    9 ++++-----
 gdb/common/filestuff.c     |   12 +++++++++---
 gdb/common/filestuff.h     |    2 ++
 gdb/common/format.c        |   10 +++++-----
 gdb/common/gdb_vecs.c      |    9 ++++-----
 gdb/common/print-utils.c   |    8 +++-----
 gdb/common/rsp-low.c       |    8 +++-----
 gdb/common/signals.c       |   14 +++++++++++---
 gdb/common/vec.c           |    9 ++++-----
 gdb/common/xml-utils.c     |    7 ++-----
 gdb/nat/linux-osdata.c     |   11 +++++++++--
 gdb/nat/linux-procfs.c     |   14 +++++++++++---
 gdb/nat/linux-ptrace.c     |   16 +++++++++++++---
 gdb/nat/mips-linux-watch.c |    1 +
 gdb/nat/mips-linux-watch.h |    7 ++-----
 gdb/target/waitstatus.c    |    6 ------
 18 files changed, 121 insertions(+), 65 deletions(-)

diff --git a/gdb/common/buffer.c b/gdb/common/buffer.c
index 4d9edb8..80b4fa1 100644
--- a/gdb/common/buffer.c
+++ b/gdb/common/buffer.c
@@ -17,11 +17,10 @@
    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 <config.h>
+
+#include "libiberty.h"
+#include "common-utils.h"
 
 #include "xml-utils.h"
 #include "buffer.h"
diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index 29fe2c5..ff4856b 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -17,12 +17,11 @@
    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 <config.h>
 
+#include "libiberty.h"
+#include "common-utils.h"
+#include "gdb_locale.h"
 #include "gdb_assert.h"
 #include <string.h>
 
diff --git a/gdb/common/filestuff.c b/gdb/common/filestuff.c
index 4544926..1d93726 100644
--- a/gdb/common/filestuff.c
+++ b/gdb/common/filestuff.c
@@ -16,15 +16,21 @@
    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 <config.h>
+
 #ifdef GDBSERVER
-#include "server.h"
+#include "build-gnulib-gdbserver/config.h"
 #else
-#include "defs.h"
-#include <string.h>
+#include "build-gnulib/config.h"
 #endif
+
+#include "common-utils.h"
+#include "gdb_locale.h"
 #include "filestuff.h"
 #include "gdb_vecs.h"
 
+#include <errno.h>
+#include <stdlib.h>
 #include <string.h>
 #include <fcntl.h>
 #include <unistd.h>
diff --git a/gdb/common/filestuff.h b/gdb/common/filestuff.h
index 70e09aa..3bc0d40 100644
--- a/gdb/common/filestuff.h
+++ b/gdb/common/filestuff.h
@@ -19,6 +19,8 @@
 #ifndef FILESTUFF_H
 #define FILESTUFF_H
 
+#include <stdio.h>
+
 /* Note all the file descriptors which are open when this is called.
    These file descriptors will not be closed by close_most_fds.  */
 
diff --git a/gdb/common/format.c b/gdb/common/format.c
index bddfbc6..05156a8 100644
--- a/gdb/common/format.c
+++ b/gdb/common/format.c
@@ -17,14 +17,14 @@
    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 <config.h>
 
 #include <string.h>
 
+#include "libiberty.h"
+#include "common-utils.h"
+#include "errors.h"
+#include "gdb_locale.h"
 #include "format.h"
 
 struct format_piece *
diff --git a/gdb/common/gdb_vecs.c b/gdb/common/gdb_vecs.c
index 4a3330f..edfab81 100644
--- a/gdb/common/gdb_vecs.c
+++ b/gdb/common/gdb_vecs.c
@@ -17,12 +17,11 @@
    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 <config.h>
 
+#include "libiberty.h"
+#include "common-utils.h"
+#include "gdb_locale.h"
 #include "gdb_vecs.h"
 #include "host-defs.h"
 
diff --git a/gdb/common/print-utils.c b/gdb/common/print-utils.c
index 0e612a3..0026b1e 100644
--- a/gdb/common/print-utils.c
+++ b/gdb/common/print-utils.c
@@ -17,11 +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"
-#endif
+#include "common-utils.h"
+#include "common-types.h"
+#include "gdb_locale.h"
 
 #include "print-utils.h"
 
diff --git a/gdb/common/rsp-low.c b/gdb/common/rsp-low.c
index b777716..597113e 100644
--- a/gdb/common/rsp-low.c
+++ b/gdb/common/rsp-low.c
@@ -17,11 +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"
-#endif
+#include "common-utils.h"
+#include "common-types.h"
+#include "gdb_locale.h"
 
 #include <string.h>
 
diff --git a/gdb/common/signals.c b/gdb/common/signals.c
index 3c9cd41..8e903ea 100644
--- a/gdb/common/signals.c
+++ b/gdb/common/signals.c
@@ -17,17 +17,25 @@
    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 <config.h>
+
 #ifdef GDBSERVER
-#include "server.h"
+#include "build-gnulib-gdbserver/config.h"
 #else
-#include "defs.h"
-#include <string.h>
+#include "build-gnulib/config.h"
 #endif
 
+#include <string.h>
+
 #ifdef HAVE_SIGNAL_H
 #include <signal.h>
 #endif
 
+#include "libiberty.h"
+#include "common-utils.h"
+#include "gdb_assert.h"
+#include "errors.h"
+#include "gdb_locale.h"
 #include "gdb_signals.h"
 #include "gdb_assert.h"
 
diff --git a/gdb/common/vec.c b/gdb/common/vec.c
index 4611e5f..d6db2e5 100644
--- a/gdb/common/vec.c
+++ b/gdb/common/vec.c
@@ -17,12 +17,11 @@
    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 <config.h>
 
+#include "libiberty.h"
+#include "common-utils.h"
+#include "gdb_locale.h"
 #include "vec.h"
 
 struct vec_prefix
diff --git a/gdb/common/xml-utils.c b/gdb/common/xml-utils.c
index c6ceb69..d2b7d17 100644
--- a/gdb/common/xml-utils.c
+++ b/gdb/common/xml-utils.c
@@ -17,12 +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"
-#endif
+#include <config.h>
 
+#include "libiberty.h"
 #include "xml-utils.h"
 
 #include <string.h>
diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
index dae637b..8df6fc3 100644
--- a/gdb/nat/linux-osdata.c
+++ b/gdb/nat/linux-osdata.c
@@ -17,13 +17,20 @@
    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 <config.h>
+
 #ifdef GDBSERVER
-#include "server.h"
+#include "build-gnulib-gdbserver/config.h"
 #else
-#include "defs.h"
+#include "build-gnulib/config.h"
 #endif
 
+#include "libiberty.h"
+#include "common-types.h"
 #include "linux-osdata.h"
+#include "common-utils.h"
+#include "gdb_assert.h"
+#include "gdb_locale.h"
 
 #include <sys/types.h>
 #include <sys/sysinfo.h>
diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
index 1443a88..1577a62 100644
--- a/gdb/nat/linux-procfs.c
+++ b/gdb/nat/linux-procfs.c
@@ -16,13 +16,21 @@
    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 <config.h>
+
 #ifdef GDBSERVER
-#include "server.h"
+#include "build-gnulib-gdbserver/config.h"
 #else
-#include "defs.h"
-#include <string.h>
+#include "build-gnulib/config.h"
 #endif
 
+#include <string.h>
+#include <stdlib.h>
+
+#include "common-utils.h"
+#include "libiberty.h"
+#include "errors.h"
+#include "gdb_locale.h"
 #include "linux-procfs.h"
 #include "filestuff.h"
 
diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
index e3462ec..01bab14 100644
--- a/gdb/nat/linux-ptrace.c
+++ b/gdb/nat/linux-ptrace.c
@@ -16,13 +16,23 @@
    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 <config.h>
+
 #ifdef GDBSERVER
-#include "server.h"
+#include "build-gnulib-gdbserver/config.h"
 #else
-#include "defs.h"
-#include <string.h>
+#include "build-gnulib/config.h"
 #endif
 
+#include <string.h>
+#include <unistd.h>
+#include <errno.h>
+
+#include "libiberty.h"
+#include "common-utils.h"
+#include "gdb_locale.h"
+#include "errors.h"
+#include "gdb_assert.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 acfc7f4..f57e2fd 100644
--- a/gdb/nat/mips-linux-watch.c
+++ b/gdb/nat/mips-linux-watch.c
@@ -18,6 +18,7 @@
 #include <sys/ptrace.h>
 #include "mips-linux-watch.h"
 #include "gdb_assert.h"
+#include "gdb_locale.h"
 
 /* Assuming usable watch registers REGS, return the irw_mask of
    register N.  */
diff --git a/gdb/nat/mips-linux-watch.h b/gdb/nat/mips-linux-watch.h
index c9f6932..b5ddd4b 100644
--- a/gdb/nat/mips-linux-watch.h
+++ b/gdb/nat/mips-linux-watch.h
@@ -18,11 +18,8 @@
 #ifndef MIPS_LINUX_WATCH_H
 #define MIPS_LINUX_WATCH_H 1
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#endif
+#include "config.h"
+#include "common-types.h"
 
 #include <asm/ptrace.h>
 #include <stdint.h>
diff --git a/gdb/target/waitstatus.c b/gdb/target/waitstatus.c
index 4493555..9021477 100644
--- a/gdb/target/waitstatus.c
+++ b/gdb/target/waitstatus.c
@@ -17,12 +17,6 @@
    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 "waitstatus.h"
 
 /* Return a pretty printed form of target_waitstatus.
-- 
1.7.1

Re: [PATCH 06/15] Remove simple GDBSERVER uses from common, nat and target

[Doug Evans]

Gary Benson writes:
 > This removes various simple GDBSERVER uses from common, nat and
 > target.  The simple uses are just cases where the code includes defs.h
 > or server.h depending on GDBSERVER.  Instead, now the files include
 > the headers that they require.  Unfortunately we still need to check
 > GDBSERVER for some files to decide which gnulib config header to
 > import, but this is a step in the right direction.
 > 
 > gdb/
 > 2014-07-09  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* common/buffer.c: Don't include server.h or defs.h; update
 > 	includes.
 > 	* common/common-utils.c: Don't include server.h or defs.h; update
 > 	includes.
 > 	* common/filestuff.c: Don't include server.h or defs.h; update
 > 	includes.
 > 	* common/filestuff.h: Include stdio.h.
 > 	* common/format.c: Don't include server.h or defs.h; update
 > 	includes.
 > 	* common/gdb_vecs.c: Don't include server.h or defs.h; update
 > 	includes.
 > 	* common/print-utils.c: Don't include server.h or defs.h; update
 > 	includes.
 > 	* common/rsp-low.c: Don't include server.h or defs.h; update
 > 	includes.
 > 	* common/signals.c: Don't include server.h or defs.h; update
 > 	includes.
 > 	* common/vec.c: Don't include server.h or defs.h; update includes.
 > 	* common/xml-utils.c: Don't include server.h or defs.h; update
 > 	includes.
 > 	* nat/linux-osdata.c: Don't include server.h or defs.h; update
 > 	includes.
 > 	* nat/linux-procfs.c: Don't include server.h or defs.h; update
 > 	includes.
 > 	* nat/linux-ptrace.c: Don't include server.h or defs.h; update
 > 	includes.
 > 	* nat/mips-linux-watch.h: Don't include server.h or defs.h; update
 > 	includes.
 > 	* target/waitstatus.c: Don't include server.h or defs.h; update
 > 	includes.

LGTM, one nit:

How about

#include "config.h"

instead of

#include <config.h>
?

Re: [PATCH 06/15] Remove simple GDBSERVER uses from common, nat and target

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
> > This removes various simple GDBSERVER uses from common, nat and
> > target.  The simple uses are just cases where the code includes defs.h
> > or server.h depending on GDBSERVER.  Instead, now the files include
> > the headers that they require.  Unfortunately we still need to check
> > GDBSERVER for some files to decide which gnulib config header to
> > import, but this is a step in the right direction.
[snip]
> 
> LGTM, one nit:
> 
> How about
> 
> #include "config.h"
> 
> instead of
> 
> #include <config.h>
> ?

I've no objection with this, I'll make the change.

Thanks,
Gary

-- 
http://gbenson.net/

[PATCH 07/15] Remove GDBSERVER use from nat/i386-dregs.c

[Gary Benson]

This removes the use of GDBSERVER from nat/i386-dregs.c.  Neither
defs.h or server.h are included: the specific files required are
listed instead.  Also, a declaration previously made only outside
of gdbserver is made unconditional.

gdb/
2014-07-09  Gary Benson  <gbenson@redhat.com>

	* gdb/nat/i386-dregs.c: Don't include server.h or defs.h; update
	includes.
	(debug_hw_points): Declare regardless of GDBSERVER.
---
 gdb/ChangeLog        |    6 ++++++
 gdb/nat/i386-dregs.c |   16 ++++++++--------
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/gdb/nat/i386-dregs.c b/gdb/nat/i386-dregs.c
index e3272cd..63a05ea 100644
--- a/gdb/nat/i386-dregs.c
+++ b/gdb/nat/i386-dregs.c
@@ -17,12 +17,14 @@
    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-utils.h"
+#include "common-types.h"
+#include "break-common.h"
+#include "common-debug.h"
+#include "print-utils.h"
+#include "errors.h"
+#include "gdb_locale.h"
+#include "gdb_assert.h"
 #include "i386-dregs.h"
 
 /* Support for hardware watchpoints and breakpoints using the i386
@@ -175,10 +177,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 07/15] Remove GDBSERVER use from nat/i386-dregs.c

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

Gary> This removes the use of GDBSERVER from nat/i386-dregs.c.  Neither
Gary> defs.h or server.h are included: the specific files required are
Gary> listed instead.  Also, a declaration previously made only outside
Gary> of gdbserver is made unconditional.

Gary> -#ifdef GDBSERVER
Gary> -#include "server.h"
Gary> -#else
Gary> -#include "defs.h"
Gary> -#include "inferior.h"
Gary> -#endif
Gary> +#include "common-utils.h"

I think this should include config.h first.

Tom

Re: [PATCH 07/15] Remove GDBSERVER use from nat/i386-dregs.c

[Gary Benson]

Tom Tromey wrote:
> >>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
> 
> Gary> This removes the use of GDBSERVER from nat/i386-dregs.c.
> Gary> Neither defs.h or server.h are included: the specific files
> Gary> required are listed instead.  Also, a declaration previously
> Gary> made only outside of gdbserver is made unconditional.
> 
> Gary> -#ifdef GDBSERVER
> Gary> -#include "server.h"
> Gary> -#else
> Gary> -#include "defs.h"
> Gary> -#include "inferior.h"
> Gary> -#endif
> Gary> +#include "common-utils.h"
> 
> I think this should include config.h first.

common-utils.h includes it.  Or did you mean the gnulib one?

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 07/15] Remove GDBSERVER use from nat/i386-dregs.c

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

Gary> -#ifdef GDBSERVER
Gary> -#include "server.h"
Gary> -#else
Gary> -#include "defs.h"
Gary> -#include "inferior.h"
Gary> -#endif
Gary> +#include "common-utils.h"

>> I think this should include config.h first.

Gary> common-utils.h includes it.  Or did you mean the gnulib one?

Nope, not the gnulib one.  I didn't realize that common-utils.h includes
config.h.  That seems weird to me.

I think it's generally better for "random" headers not to include
config.h and to either have a "standard" base header (like defs.h or
server.h) that includes config.h, or to just have explicit includes of
config.h as the first thing in each .c.

I realize it's not a problem arising from your series.
But would you mind just having the .c include config.h explicitly?
Then at some point I'll go through and fix up common-utils.h and its
users.

Tom

Re: [PATCH 07/15] Remove GDBSERVER use from nat/i386-dregs.c

[Gary Benson]

Tom Tromey wrote:
> >>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
> 
> Gary> -#ifdef GDBSERVER
> Gary> -#include "server.h"
> Gary> -#else
> Gary> -#include "defs.h"
> Gary> -#include "inferior.h"
> Gary> -#endif
> Gary> +#include "common-utils.h"
> 
> >> I think this should include config.h first.
> 
> Gary> common-utils.h includes it.  Or did you mean the gnulib one?
> 
> Nope, not the gnulib one.  I didn't realize that common-utils.h
> includes config.h.  That seems weird to me.
> 
> I think it's generally better for "random" headers not to include
> config.h and to either have a "standard" base header (like defs.h or
> server.h) that includes config.h, or to just have explicit includes
> of config.h as the first thing in each .c.
> 
> I realize it's not a problem arising from your series.  But would
> you mind just having the .c include config.h explicitly?  Then at
> some point I'll go through and fix up common-utils.h and its users.

Ok, I'll do that.

Cheers,
Gary

-- 
http://gbenson.net/

Re: [PATCH 07/15] Remove GDBSERVER use from nat/i386-dregs.c

[Doug Evans]

Gary Benson writes:
 > This removes the use of GDBSERVER from nat/i386-dregs.c.  Neither
 > defs.h or server.h are included: the specific files required are
 > listed instead.  Also, a declaration previously made only outside
 > of gdbserver is made unconditional.
 > 
 > gdb/
 > 2014-07-09  Gary Benson  <gbenson@redhat.com>
 > 
 > 	* gdb/nat/i386-dregs.c: Don't include server.h or defs.h; update
 > 	includes.
 > 	(debug_hw_points): Declare regardless of GDBSERVER.

LGTM

[PATCH 05/15] Introduce and use debug_printf and debug_vprintf

[Gary Benson]

This introduces debug_printf and debug_vprintf, a function that
clients of "common" are expected to implement.  gdbserver's
existing debug_printf is repurposed as debug_vprintf, and a new
wrapper is written.

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-07-09  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* common/common-debug.h: New file.
	* utils.h: Include common-debug.h.
	* utils.c (debug_vprintf): New function.
	(debug_printf): Likewise.
	* common/agent.c (debug_agent_print): New function.
	(DEBUG_AGENT): Redefine.
	* nat/i386-dregs.c (debug_printf): Undefine.

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

	* utils.h: Include common-debug.h.
	* debug.h (debug_printf): Don't declare.
	* debug.c (debug_vprintf): New function.
	(debug_printf): Use the above.
---
 gdb/ChangeLog             |   11 +++++++++++
 gdb/common/agent.c        |   24 +++++++++++++++---------
 gdb/common/common-debug.h |   35 +++++++++++++++++++++++++++++++++++
 gdb/gdbserver/ChangeLog   |    8 ++++++++
 gdb/gdbserver/debug.c     |   23 ++++++++++++++++-------
 gdb/gdbserver/debug.h     |    1 -
 gdb/gdbserver/utils.h     |    1 +
 gdb/nat/i386-dregs.c      |    4 ----
 gdb/utils.c               |   20 ++++++++++++++++++++
 gdb/utils.h               |    1 +
 10 files changed, 107 insertions(+), 21 deletions(-)
 create mode 100644 gdb/common/common-debug.h

diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index 54f861a..549b784 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -33,15 +33,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.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/gdbserver/debug.c b/gdb/gdbserver/debug.c
index c50af76..c008b17 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,16 +52,26 @@ 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
 }
 
+/* Print a debugging message using debug_vprintf.  */
+
+void
+debug_printf (const char *format, ...)
+{
+  va_list ap;
+
+  va_start (ap, format);
+  debug_vprintf (format, ap);
+  va_end (ap);
+}
+
 /* Flush debugging output.
    This is called, for example, when starting an inferior to ensure all debug
    output thus far appears before any inferior output.  */
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/gdbserver/utils.h b/gdb/gdbserver/utils.h
index 819fa35..d48179d 100644
--- a/gdb/gdbserver/utils.h
+++ b/gdb/gdbserver/utils.h
@@ -21,6 +21,7 @@
 
 #include "print-utils.h"
 #include "errors.h"
+#include "common-debug.h"
 
 char *paddress (CORE_ADDR addr);
 char *pfildes (gdb_fildes_t fd);
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.  */
diff --git a/gdb/utils.c b/gdb/utils.c
index 6f47cb0..064e2ee 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -886,6 +886,26 @@ demangler_warning (const char *file, int line, const char *string, ...)
   va_end (ap);
 }
 
+/* See common/common-debug.h.  */
+
+void
+debug_vprintf (const char *fmt, va_list ap)
+{
+  vfprintf_unfiltered (gdb_stdlog, fmt, ap);
+}
+
+/* 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);
+}
+
 /* Dummy functions to keep add_prefix_cmd happy.  */
 
 static void
diff --git a/gdb/utils.h b/gdb/utils.h
index d9fe7e3..914d0b3 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -25,6 +25,7 @@
 #include "exceptions.h"
 #include "print-utils.h"
 #include "errors.h"
+#include "common-debug.h"
 
 extern void initialize_utils (void);
 
-- 
1.7.1

Re: [PATCH 05/15] Introduce and use debug_printf and debug_vprintf

[Doug Evans]

Gary Benson writes:
 > This introduces debug_printf and debug_vprintf, a function that
 > clients of "common" are expected to implement.  gdbserver's
 > existing debug_printf is repurposed as debug_vprintf, and a new
 > wrapper is written.
 > 
 > 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-07-09  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* common/common-debug.h: New file.
 > 	* utils.h: Include common-debug.h.
 > 	* utils.c (debug_vprintf): New function.
 > 	(debug_printf): Likewise.
 > 	* common/agent.c (debug_agent_print): New function.
 > 	(DEBUG_AGENT): Redefine.
 > 	* nat/i386-dregs.c (debug_printf): Undefine.
 > 
 > gdb/gdbserver/
 > 2014-07-09  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* utils.h: Include common-debug.h.
 > 	* debug.h (debug_printf): Don't declare.
 > 	* debug.c (debug_vprintf): New function.
 > 	(debug_printf): Use the above.

IWBN if there was more file naming consistency.
As a general rule, how objectionable is it to have
gdb/foo.c and gdbserver/foo.c for every shared foo.h header?
[Or common-foo.h header in the case of, e.g., common/common-debug.h.]
An alternative would be to move common-debug.h to a new directory,
e.g., shared, and call it shared/debug.h.  I'm only mentioning
this for discussion sake, it's not a requisite for this patch.

In this case, any objection to putting the gdb implementation
in gdb/debug.c instead of utils.c?

Re: [PATCH 05/15] Introduce and use debug_printf and debug_vprintf

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
> > 2014-07-09  Tom Tromey  <tromey@redhat.com>
> >             Gary Benson  <gbenson@redhat.com>
> > 
> >         * common/common-debug.h: New file.
> >         * utils.h: Include common-debug.h.
> >         * utils.c (debug_vprintf): New function.
> >         (debug_printf): Likewise.
> >         * common/agent.c (debug_agent_print): New function.
> >         (DEBUG_AGENT): Redefine.
> >         * nat/i386-dregs.c (debug_printf): Undefine.
> > 
> > gdb/gdbserver/
> > 2014-07-09  Tom Tromey  <tromey@redhat.com>
> >             Gary Benson  <gbenson@redhat.com>
> > 
> >         * utils.h: Include common-debug.h.
> >         * debug.h (debug_printf): Don't declare.
> >         * debug.c (debug_vprintf): New function.
> >         (debug_printf): Use the above.
> 
> IWBN if there was more file naming consistency.
> As a general rule, how objectionable is it to have gdb/foo.c
> and gdbserver/foo.c for every shared foo.h header?

I like it.

> [Or common-foo.h header in the case of, e.g., common/common-debug.h.]
> An alternative would be to move common-debug.h to a new directory,
> e.g., shared, and call it shared/debug.h.  I'm only mentioning this
> for discussion sake, it's not a requisite for this patch.

I have a script to automate the changes required so we can remove
"-I/path/to/gdb/common" from $CFLAGS.  This would allow GDB and/or
gdbserver to have their own "foo.h" that could include "common/foo.h".
The script also removes all the "common-" and "gdb_" prefixes from
header files in common.  I plan to submit a patch after this series
is in.  This means this series is creating files that will almost
immediately be moved, but the alternative is I end up having to
extensively fix this series (and I already did that once already!)

> In this case, any objection to putting the gdb implementation
> in gdb/debug.c instead of utils.c?

No objection from me, I'll do it.

Thanks,
Gary

-- 
http://gbenson.net/

[PATCH 12/15] Add target/symbol.h, update users

[Gary Benson]

This adds a new "target/symbol.h" file, that declares a symbol-lookup
function used by code in common.  It follows the usual pattern, where
clients of "common" have their own definitions of this function.  This
simplifies agent.c a bit.

gdb/
2014-07-09  Tom Tromey  <tromey@redhat.com>

	* target/symbol.h: New file.
	* target.h: Include target/symbol.h.
	* target.c (target_look_up_symbol): New function.
	* common/agent.c: Include target/symbol.h.
	(agent_look_up_symbols): Use target_look_up_symbol.

gdb/gdbserver/
2014-07-09  Tom Tromey  <tromey@redhat.com>

	* target.c: Include target/symbol.h.
	(target_look_up_symbol): New function.
---
 gdb/ChangeLog           |    8 ++++++++
 gdb/common/agent.c      |   13 ++-----------
 gdb/gdbserver/ChangeLog |    5 +++++
 gdb/gdbserver/target.c  |    9 +++++++++
 gdb/target.c            |   13 +++++++++++++
 gdb/target.h            |    1 +
 gdb/target/symbol.h     |   26 ++++++++++++++++++++++++++
 7 files changed, 64 insertions(+), 11 deletions(-)
 create mode 100644 gdb/target/symbol.h

diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index 0fde2fd..798dd1d 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -31,6 +31,7 @@
 
 #include "common-types.h"
 #include "target/target.h"
+#include "target/symbol.h"
 #include "errors.h"
 #include "ptid.h"
 #include "gdb_locale.h"
@@ -111,18 +112,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 f93163e..58b3d8c 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,14 @@ 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, void *data)
+{
+  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 0796059..047f6ec 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1322,6 +1322,19 @@ 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, void *data)
+{
+  struct bound_minimal_symbol sym
+    = lookup_minimal_symbol (name, NULL, (struct objfile *) data);
+
+  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 2a4783c..bb776e4 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..3e4591f
--- /dev/null
+++ b/gdb/target/symbol.h
@@ -0,0 +1,26 @@
+/* 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
+
+extern int target_look_up_symbol (const char *name, CORE_ADDR *addr,
+				  void *data);
+
+#endif /* TARGET_SYMBOL_H */
-- 
1.7.1

Re: [PATCH 12/15] Add target/symbol.h, update users

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

Gary> +/* See target/symbol.h.  */
Gary> +
Gary> +int
Gary> +target_look_up_symbol (const char *name, CORE_ADDR *addr, void *data)

I never wrote these docs either...

Tom

Re: [PATCH 12/15] Add target/symbol.h, update users

[Doug Evans]

On Thu, Jul 10, 2014 at 10:52 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
>
> Gary> +/* See target/symbol.h.  */
> Gary> +
> Gary> +int
> Gary> +target_look_up_symbol (const char *name, CORE_ADDR *addr, void *data)
>
> I never wrote these docs either...

Nit,
Anything with target_ as a prefix I think of as a target.h method.
[There are a few exceptions but as long as it's kept to a minimum it's
manageable.]
IWBN if the different, umm, subsystems of gdb were easily recognizable
in the code.
As target/* scales up, is there a risk of the code becoming harder to
read if target_ is used as a general prefix for things in target/*?
Dunno.  Just wondering.

Re: [PATCH 12/15] Add target/symbol.h, update users

[Tom Tromey]

Doug> As target/* scales up, is there a risk of the code becoming harder to
Doug> read if target_ is used as a general prefix for things in target/*?
Doug> Dunno.  Just wondering.

My long term goal is that gdb and gdbserver share the entire target
stack.  I think these patches further this goal.  I don't find the
result harder to read at all.

Tom

Re: [PATCH 12/15] Add target/symbol.h, update users

[Gary Benson]

Tom Tromey wrote:
> Doug> As target/* scales up, is there a risk of the code becoming
> Doug> harder to read if target_ is used as a general prefix for
> Doug> things in target/*?  Dunno.  Just wondering.
> 
> My long term goal is that gdb and gdbserver share the entire target
> stack.  I think these patches further this goal.  I don't find the
> result harder to read at all.

Doug, are you ok for me to leave it as it is, or, do you have an
alternative you would like me to implement instead?

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 12/15] Add target/symbol.h, update users

[Doug Evans]

On Fri, Jul 11, 2014 at 5:56 AM, Gary Benson <gbenson@redhat.com> wrote:
> Tom Tromey wrote:
>> Doug> As target/* scales up, is there a risk of the code becoming
>> Doug> harder to read if target_ is used as a general prefix for
>> Doug> things in target/*?  Dunno.  Just wondering.
>>
>> My long term goal is that gdb and gdbserver share the entire target
>> stack.  I think these patches further this goal.  I don't find the
>> result harder to read at all.
>
> Doug, are you ok for me to leave it as it is, or, do you have an
> alternative you would like me to implement instead?

Hi.  I'm not sure TBH.
I think it might be ok as is, but IWBN to see the full patch with comments.

Re: [PATCH 12/15] Add target/symbol.h, update users

[Gary Benson]

Doug Evans wrote:
> On Fri, Jul 11, 2014 at 5:56 AM, Gary Benson <gbenson@redhat.com> wrote:
> > Tom Tromey wrote:
> > > Doug> As target/* scales up, is there a risk of the code
> > > Doug> becoming harder to read if target_ is used as a general
> > > Doug> prefix for things in target/*?  Dunno.  Just wondering.
> > >
> > > My long term goal is that gdb and gdbserver share the entire
> > > target stack.  I think these patches further this goal.  I don't
> > > find the result harder to read at all.
> >
> > Doug, are you ok for me to leave it as it is, or, do you have an
> > alternative you would like me to implement instead?
> 
> Hi.  I'm not sure TBH.  I think it might be ok as is, but IWBN to
> see the full patch with comments.

In this case, the implementations are in gdb/{,gdbserver/}target.c
so maybe it would make sense to have the prototype in
gdb/target/target.h--though this comes with the caveat that I don't
have a good idea of what sharing the entire target stack will mean
right now.  It may be that Tom created the file anticipating other
functions that will go there in future.  Doug, Tom, I'd appreciate
both your opinions on this.

Wherever it goes (and whatever it's called) I think there'll always be
a bit of jiggling around with a refactoring of this size and nature.
I think we need to prioritize getting the right subset of code shared
over getting the interface perfect or we risk premature optimization.
Doug, I realize that this paragraph could be read as "I don't share
your concerns" but please don't read it this way!  I want a really
nice interface between the shared code (ie common/target/nat) and the
consumers (GDB and gdbserver).  I'm anticipating various functions
being moved and/or renamed as the exact subset of code to be shared
becomes clearer.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 12/15] Add target/symbol.h, update users

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

Gary> It may be that Tom created the file anticipating other functions
Gary> that will go there in future.

If I remember correctly I think I did it just to try to separate out
different conceptual bits.  I don't really care much about it per se, I
just think it would be nice if we could avoid the current problem of
header files full of unsorted goo.  Though admittedly a single
declaration in a single file hardly makes a dent.

Tom

Re: [PATCH 12/15] Add target/symbol.h, update users

[Gary Benson]

Tom Tromey wrote:
> >>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
> 
> Gary> +/* See target/symbol.h.  */
> Gary> +
> Gary> +int
> Gary> +target_look_up_symbol (const char *name, CORE_ADDR *addr, void *data)
> 
> I never wrote these docs either...

I've done this:

+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,
 -                                 void *data);
 +                                 struct objfile *objfile);

Note that I changed the final parameter to struct objfile *objfile and
added a forward declaration which is never filled out in gdbserver and
added an assertion that objfile == NULL in gdbserver.  I don't know
whether you will agree with this but I'll change it back if you prefer.

Thanks,
Gary

--
http://gbenson.net/

Re: [PATCH 12/15] Add target/symbol.h, update users

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

Gary> Note that I changed the final parameter to struct objfile *objfile and
Gary> added a forward declaration which is never filled out in gdbserver and
Gary> added an assertion that objfile == NULL in gdbserver.  I don't know
Gary> whether you will agree with this but I'll change it back if you prefer.

It seems fine to me.

Tom

[PATCH 14/15] Introduce common_get_thread_regcache

[Gary Benson]

This introduces common_get_thread_regcache 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-07-09  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* regcache.h (common_get_thread_regcache): Declare.
	* regcache.c (common_get_thread_regcache): New function.
	* nat/linux-btrace.h (common_get_thread_regcache): Declare.
	* nat/linux-btrace.c (perf_event_read_bts): Use
	common_get_thread_regcache.

gdb/gdbserver/
2014-07-09  Tom Tromey  <tromey@redhat.com>

	* regcache.c (common_get_thread_regcache): New function.
---
 gdb/ChangeLog            |    9 +++++++++
 gdb/gdbserver/ChangeLog  |    4 ++++
 gdb/gdbserver/regcache.c |    8 ++++++++
 gdb/nat/linux-btrace.c   |    6 +-----
 gdb/nat/linux-btrace.h   |    5 +++++
 gdb/regcache.c           |    7 +++++++
 gdb/regcache.h           |    1 +
 7 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index bed10b4..6f1cb39 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -65,6 +65,14 @@ get_thread_regcache (struct thread_info *thread, int fetch)
   return regcache;
 }
 
+/* See common/linux-btrace.h.  */
+
+struct regcache *
+common_get_thread_regcache (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 188220b..dd7744b 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -184,11 +184,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 = common_get_thread_regcache (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/nat/linux-btrace.h b/gdb/nat/linux-btrace.h
index 12e9b60..85cd88b 100644
--- a/gdb/nat/linux-btrace.h
+++ b/gdb/nat/linux-btrace.h
@@ -33,6 +33,11 @@
 #  include <linux/perf_event.h>
 #endif
 
+/* A hack until we have an independent regcache.  This must be
+   provided by the user.  */
+
+extern struct regcache *common_get_thread_regcache (ptid_t ptid);
+
 /* Branch trace target information per thread.  */
 struct btrace_target_info
 {
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 5ee90b0..047f8f4 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -537,6 +537,13 @@ get_current_regcache (void)
   return get_thread_regcache (inferior_ptid);
 }
 
+/* See common/linux-btrace.h.  */
+
+struct regcache *
+common_get_thread_regcache (ptid_t ptid)
+{
+  return get_thread_regcache (ptid);
+}
 
 /* Observer for the target_changed event.  */
 
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 8423f57..9679691 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -25,6 +25,7 @@ struct gdbarch;
 struct address_space;
 
 extern struct regcache *get_current_regcache (void);
+extern struct regcache *common_get_thread_regcache (ptid_t ptid);
 extern struct regcache *get_thread_regcache (ptid_t ptid);
 extern struct regcache *get_thread_arch_regcache (ptid_t, struct gdbarch *);
 extern struct regcache *get_thread_arch_aspace_regcache (ptid_t,
-- 
1.7.1

Re: [PATCH 14/15] Introduce common_get_thread_regcache

[Doug Evans]

Gary Benson writes:
 > This introduces common_get_thread_regcache 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-07-09  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* regcache.h (common_get_thread_regcache): Declare.
 > 	* regcache.c (common_get_thread_regcache): New function.
 > 	* nat/linux-btrace.h (common_get_thread_regcache): Declare.
 > 	* nat/linux-btrace.c (perf_event_read_bts): Use
 > 	common_get_thread_regcache.

When I think of "btrace" I don't think of "regcache".
Can common_get_thread_regcache go some place other than linux-btrace.h?

Also, why declare the function in two places?
I would expect it to be declared once in a common header.

Lastly, common_get_thread_regcache feels a bit weird as the name for
this function.  get_thread_regcache_for_ptid, or some such, feels better.

Cheers.

Re: [PATCH 14/15] Introduce common_get_thread_regcache

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
> > This introduces common_get_thread_regcache 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-07-09  Tom Tromey  <tromey@redhat.com>
> > 	    Gary Benson  <gbenson@redhat.com>
> > 
> > 	* regcache.h (common_get_thread_regcache): Declare.
> > 	* regcache.c (common_get_thread_regcache): New function.
> > 	* nat/linux-btrace.h (common_get_thread_regcache): Declare.
> > 	* nat/linux-btrace.c (perf_event_read_bts): Use
> > 	common_get_thread_regcache.
> 
> When I think of "btrace" I don't think of "regcache".  Can
> common_get_thread_regcache go some place other than linux-btrace.h?
> 
> Also, why declare the function in two places?  I would expect it to
> be declared once in a common header.
> 
> Lastly, common_get_thread_regcache feels a bit weird as the name for
> this function.  get_thread_regcache_for_ptid, or some such, feels
> better.

I renamed it get_thread_regcache_for_ptid and put it in its own
header, common/common-regcache.h.

Thanks,
Gary

-- 
http://gbenson.net/

[PATCH 13/15] Finally remove GDBSERVER (mostly) from agent.c

[Gary Benson]

This reduces the use of the GDBSERVER define in agent.c to deciding
which gnulib header to include.

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

	* common/agent.c: Don't include defs.h or server.h.
---
 gdb/ChangeLog      |    5 +++++
 gdb/common/agent.c |   19 ++++++++++++++-----
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index 798dd1d..0df4b19 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -17,15 +17,24 @@
    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 <config.h>
+
 #ifdef GDBSERVER
-#include "server.h"
+#include "build-gnulib-gdbserver/config.h"
 #else
-#include "defs.h"
-#include "target.h"
-#include "infrun.h"
-#include "objfiles.h"
+#include "build-gnulib/config.h"
 #endif
 
+#include <unistd.h>
+
+#include "ptid.h"
+#include "gdb_signals.h"
+#include "target/waitstatus.h"
+#include "common-types.h"
+#include "target/target.h"
+#include "target/symbol.h"
+#include "common-debug.h"
+
 #include <stdarg.h>
 #include <errno.h>
 
-- 
1.7.1

Re: [PATCH 13/15] Finally remove GDBSERVER (mostly) from agent.c

[Doug Evans]

Gary Benson writes:
 > This reduces the use of the GDBSERVER define in agent.c to deciding
 > which gnulib header to include.
 > 
 > gdb/
 > 2014-07-09  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* common/agent.c: Don't include defs.h or server.h.

LGTM.
Might want to expand on the changelog entry to be like 06/15(?).

E.g.,
	* common/buffer.c: Don't include server.h or defs.h; update
	includes.