[PATCH 0/9 v7] Common code cleanups

[PATCH 0/9 v7] Common code cleanups

[Gary Benson]

Hi all,

This series contains updated versions of the remaining common
cleanups patches I last posted on August 1 and 13.

I've documented the changes I've made for each patch in their
individual emails.  These paragraphs will not be included in
the commit messages.

Built and regtested on RHEL 6.5 x86_64.

Ok to commit?

Thanks,
Gary

--
http://gbenson.net/

[PATCH 2/9 v7] Introduce target/target.h

[Gary Benson]

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

This patch differs from the version I posted on August 1
(https://sourceware.org/ml/gdb-patches/2014-08/msg00014.html) in that
the target_stop, target_wait and target_resume parts have been split
into patch 3 of this series and that target_read_uint32's result is
now a uint32_t.

gdb/ChangeLog:

	* target/target.h: New file.
	* Makefile.in (HFILES_NO_SRCDIR): Add target/target.h.
	* 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: Include target/target.h.
	[!GDBSERVER]: Don't include target.h.
	(helper_thread_id): Type changed to uint32_t.
	(agent_get_helper_thread_id): Use target_read_uint32.
	(agent_run_command): Always use target_read_memory and
	target_write_memory.
	(agent_capability): Type changed to uint32_t.
	(agent_capability_check): Use target_read_uint32.

gdb/gdbserver/ChangeLog:

	* target.h: Include target/target.h.
	* target.c (target_read_memory, target_read_uint32)
	(target_write_memory): New functions.
---
 gdb/ChangeLog           |   17 +++++++++++++
 gdb/Makefile.in         |    2 +-
 gdb/common/agent.c      |   52 ++++++---------------------------------
 gdb/gdbserver/ChangeLog |    7 +++++
 gdb/gdbserver/target.c  |   24 ++++++++++++++++++
 gdb/gdbserver/target.h  |    1 +
 gdb/target.c            |   16 ++++++++++++
 gdb/target.h            |    7 ++---
 gdb/target/target.h     |   62 +++++++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 139 insertions(+), 49 deletions(-)
 create mode 100644 gdb/target/target.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index fbcdfb3..bb33d38 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -938,7 +938,7 @@ target/wait.h target/waitstatus.h nat/linux-nat.h nat/linux-waitpid.h \
 common/print-utils.h common/rsp-low.h nat/i386-dregs.h x86-linux-nat.h \
 i386-linux-nat.h common/common-defs.h common/errors.h common/common-types.h \
 common/common-debug.h common/cleanups.h common/gdb_setjmp.h \
-common/common-exceptions.h
+common/common-exceptions.h target/target.h
 
 # Header files that already have srcdir in them, or which are in objdir.
 
diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index 2963917..9f8ea9a 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -21,10 +21,10 @@
 #include "server.h"
 #else
 #include "defs.h"
-#include "target.h"
 #include "infrun.h"
 #include "objfiles.h"
 #endif
+#include "target/target.h"
 #include <unistd.h>
 #include "agent.h"
 #include "filestuff.h"
@@ -62,7 +62,7 @@ struct ipa_sym_addresses
 
 /* Cache of the helper thread id.  FIXME: this global should be made
    per-process.  */
-static unsigned int helper_thread_id = 0;
+static uint32_t helper_thread_id = 0;
 
 static struct
 {
@@ -126,23 +126,9 @@ agent_get_helper_thread_id (void)
 {
   if  (helper_thread_id == 0)
     {
-#ifdef GDBSERVER
-      if (read_inferior_memory (ipa_sym_addrs.addr_helper_thread_id,
-				(unsigned char *) &helper_thread_id,
-				sizeof helper_thread_id))
-#else
-      enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch ());
-      gdb_byte buf[4];
-
-      if (target_read_memory (ipa_sym_addrs.addr_helper_thread_id,
-			      buf, sizeof buf) == 0)
-	helper_thread_id = extract_unsigned_integer (buf, sizeof buf,
-						     byte_order);
-      else
-#endif
-	{
-	  warning (_("Error reading helper thread's id in lib"));
-	}
+      if (target_read_uint32 (ipa_sym_addrs.addr_helper_thread_id,
+			      &helper_thread_id))
+	warning (_("Error reading helper thread's id in lib"));
     }
 
   return helper_thread_id;
@@ -220,13 +206,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)
     {
@@ -308,13 +289,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;
@@ -325,7 +301,7 @@ agent_run_command (int pid, const char *cmd, int len)
 }
 
 /* Each bit of it stands for a capability of agent.  */
-static unsigned int agent_capability = 0;
+static uint32_t agent_capability = 0;
 
 /* Return true if agent has capability AGENT_CAP, otherwise return false.  */
 
@@ -334,20 +310,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..08506e5 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, ssize_t len)
+{
+  return read_inferior_memory (memaddr, myaddr, len);
+}
+
+/* See target/target.h.  */
+
+int
+target_read_uint32 (CORE_ADDR memaddr, uint32_t *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/gdbserver/target.h b/gdb/gdbserver/target.h
index f5eda8a..a08b753 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -21,6 +21,7 @@
 #ifndef TARGET_H
 #define TARGET_H
 
+#include "target/target.h"
 #include "target/resume.h"
 #include "target/wait.h"
 #include "target/waitstatus.h"
diff --git a/gdb/target.c b/gdb/target.c
index 8bf6031..711e7cb 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1269,6 +1269,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, uint32_t *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 85763ba..2ea7a2d 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"
@@ -1292,8 +1293,7 @@ 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);
+/* For target_read_memory see target/target.h.  */
 
 extern int target_read_raw_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
 				   ssize_t len);
@@ -1302,8 +1302,7 @@ 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);
+/* For target_write_memory see target/target.h.  */
 
 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..0a3c4b7
--- /dev/null
+++ b/gdb/target/target.h
@@ -0,0 +1,62 @@
+/* Declarations for common target functions.
+
+   Copyright (C) 1986-2014 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef TARGET_COMMON_H
+#define TARGET_COMMON_H
+
+#include "target/waitstatus.h"
+#include <stdint.h>
+
+/* This header is a stopgap until more code is shared.  */
+
+/* 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.  This function must be provided by
+   the client.  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.  This function must be provided by the client.
+   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, uint32_t *result);
+
+/* Write LEN bytes from MYADDR to target memory at address MEMADDR.
+   Return zero for success, nonzero if any error occurs.  This
+   function must be provided by the client.  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);
+
+#endif /* TARGET_COMMON_H */
-- 
1.7.1

Re: [PATCH 2/9 v7] Introduce target/target.h

[Pedro Alves]

On 08/29/2014 02:51 PM, Gary Benson wrote:
> This introduces target/target.h.  This file declares some functions
> that the shared code can use and that clients must implement.  It also
> changes some shared code to use these functions.
> 
> This patch differs from the version I posted on August 1
> (https://sourceware.org/ml/gdb-patches/2014-08/msg00014.html) in that
> the target_stop, target_wait and target_resume parts have been split
> into patch 3 of this series and that target_read_uint32's result is
> now a uint32_t.

OK.

Thanks,
Pedro Alves

> 
> gdb/ChangeLog:
> 
> 	* target/target.h: New file.
> 	* Makefile.in (HFILES_NO_SRCDIR): Add target/target.h.
> 	* 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: Include target/target.h.
> 	[!GDBSERVER]: Don't include target.h.
> 	(helper_thread_id): Type changed to uint32_t.
> 	(agent_get_helper_thread_id): Use target_read_uint32.
> 	(agent_run_command): Always use target_read_memory and
> 	target_write_memory.
> 	(agent_capability): Type changed to uint32_t.
> 	(agent_capability_check): Use target_read_uint32.
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* target.h: Include target/target.h.
> 	* target.c (target_read_memory, target_read_uint32)
> 	(target_write_memory): New functions.

[PATCH 1/9 v7] Introduce show_debug_regs

[Gary Benson]

This commit adds a new global flag show_debug_regs to common-debug.h
to replace the flag debug_hw_points used by gdbserver and by the
Linux x86 and AArch64 ports, and to replace the flag maint_show_dr
used by the Linux MIPS port.

This patch is unchanged from the version I posted August 13:
https://sourceware.org/ml/gdb-patches/2014-08/msg00219.html

gdb/ChangeLog:

	* common/common-debug.h (show_debug_regs): Declare.
	* common/common-debug.c (show_debug_regs): Define.
	* aarch64-linux-nat.c (debug_hw_points): Don't define.  Replace
	all uses with show_debug_regs.  Replace all uses that considered
	debug_hw_points as a multi-value integer with straight boolean
	uses.
	* i386-nat.c (debug_hw_points): Don't define.  Replace all uses
	with show_debug_regs.
	* nat/i386-dregs.c (debug_hw_points): Don't declare.  Replace
	all uses with show_debug_regs.
	* mips-linux-nat.c (maint_show_dr): Don't define.  Replace all
	uses with show_debug_regs.

gdb/gdbserver/ChangeLog:

	* server.h (debug_hw_points): Don't declare.
	* server.c (debug_hw_points): Don't define.  Replace all uses
	with show_debug_regs.
	* linux-aarch64-low.c (debug_hw_points): Don't define.  Replace
	all uses with show_debug_regs.
---
 gdb/ChangeLog                     |   15 +++++++++++++++
 gdb/aarch64-linux-nat.c           |   30 +++++++++++++-----------------
 gdb/common/common-debug.c         |    4 ++++
 gdb/common/common-debug.h         |    5 +++++
 gdb/gdbserver/ChangeLog           |    8 ++++++++
 gdb/gdbserver/linux-aarch64-low.c |   19 ++++++++-----------
 gdb/gdbserver/server.c            |    7 ++-----
 gdb/gdbserver/server.h            |    1 -
 gdb/i386-nat.c                    |    5 +----
 gdb/mips-linux-nat.c              |   10 +++-------
 gdb/nat/i386-dregs.c              |   13 ++++---------
 11 files changed, 63 insertions(+), 54 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 1184197..1c1832f 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -119,10 +119,6 @@ get_thread_id (ptid_t ptid)
 static int aarch64_num_bp_regs;
 static int aarch64_num_wp_regs;
 
-/* Debugging of hardware breakpoint/watchpoint support.  */
-
-static int debug_hw_points;
-
 /* Each bit of a variable of this type is used to indicate whether a
    hardware breakpoint or watchpoint setting has been changed since
    the last update.
@@ -363,7 +359,7 @@ debug_reg_change_callback (struct lwp_info *lwp, void *ptr)
   if (info == NULL)
     info = lwp->arch_private = XCNEW (struct arch_lwp_info);
 
-  if (debug_hw_points)
+  if (show_debug_regs)
     {
       fprintf_unfiltered (gdb_stdlog,
 			  "debug_reg_change_callback: \n\tOn entry:\n");
@@ -392,7 +388,7 @@ debug_reg_change_callback (struct lwp_info *lwp, void *ptr)
   if (!lwp->stopped)
     linux_stop_lwp (lwp);
 
-  if (debug_hw_points)
+  if (show_debug_regs)
     {
       fprintf_unfiltered (gdb_stdlog,
 			  "\tOn exit:\n\tpid%d, dr_changed_bp=0x%s, "
@@ -677,7 +673,7 @@ aarch64_linux_prepare_to_resume (struct lwp_info *lwp)
       struct aarch64_debug_reg_state *state
 	= aarch64_get_debug_reg_state (ptid_get_pid (lwp->ptid));
 
-      if (debug_hw_points)
+      if (show_debug_regs)
 	fprintf_unfiltered (gdb_stdlog, "prepare_to_resume thread %d\n", tid);
 
       /* Watchpoints.  */
@@ -1200,7 +1196,7 @@ aarch64_linux_insert_hw_breakpoint (struct target_ops *self,
   const int len = 4;
   const int type = hw_execute;
 
-  if (debug_hw_points)
+  if (show_debug_regs)
     fprintf_unfiltered
       (gdb_stdlog,
        "insert_hw_breakpoint on entry (addr=0x%08lx, len=%d))\n",
@@ -1208,7 +1204,7 @@ aarch64_linux_insert_hw_breakpoint (struct target_ops *self,
 
   ret = aarch64_handle_breakpoint (type, addr, len, 1 /* is_insert */);
 
-  if (debug_hw_points > 1)
+  if (show_debug_regs)
     {
       struct aarch64_debug_reg_state *state
 	= aarch64_get_debug_reg_state (ptid_get_pid (inferior_ptid));
@@ -1233,14 +1229,14 @@ aarch64_linux_remove_hw_breakpoint (struct target_ops *self,
   const int len = 4;
   const int type = hw_execute;
 
-  if (debug_hw_points)
+  if (show_debug_regs)
     fprintf_unfiltered
       (gdb_stdlog, "remove_hw_breakpoint on entry (addr=0x%08lx, len=%d))\n",
        (unsigned long) addr, len);
 
   ret = aarch64_handle_breakpoint (type, addr, len, 0 /* is_insert */);
 
-  if (debug_hw_points > 1)
+  if (show_debug_regs)
     {
       struct aarch64_debug_reg_state *state
 	= aarch64_get_debug_reg_state (ptid_get_pid (inferior_ptid));
@@ -1297,7 +1293,7 @@ aarch64_handle_unaligned_watchpoint (int type, CORE_ADDR addr, int len,
 	ret = aarch64_dr_state_remove_one_point (state, type, aligned_addr,
 						 aligned_len);
 
-      if (debug_hw_points)
+      if (show_debug_regs)
 	fprintf_unfiltered (gdb_stdlog,
 "handle_unaligned_watchpoint: is_insert: %d\n"
 "                             aligned_addr: 0x%08lx, aligned_len: %d\n"
@@ -1335,7 +1331,7 @@ aarch64_linux_insert_watchpoint (struct target_ops *self,
 {
   int ret;
 
-  if (debug_hw_points)
+  if (show_debug_regs)
     fprintf_unfiltered (gdb_stdlog,
 			"insert_watchpoint on entry (addr=0x%08lx, len=%d)\n",
 			(unsigned long) addr, len);
@@ -1344,7 +1340,7 @@ aarch64_linux_insert_watchpoint (struct target_ops *self,
 
   ret = aarch64_handle_watchpoint (type, addr, len, 1 /* is_insert */);
 
-  if (debug_hw_points > 1)
+  if (show_debug_regs)
     {
       struct aarch64_debug_reg_state *state
 	= aarch64_get_debug_reg_state (ptid_get_pid (inferior_ptid));
@@ -1368,7 +1364,7 @@ aarch64_linux_remove_watchpoint (struct target_ops *self,
 {
   int ret;
 
-  if (debug_hw_points)
+  if (show_debug_regs)
     fprintf_unfiltered (gdb_stdlog,
 			"remove_watchpoint on entry (addr=0x%08lx, len=%d)\n",
 			(unsigned long) addr, len);
@@ -1377,7 +1373,7 @@ aarch64_linux_remove_watchpoint (struct target_ops *self,
 
   ret = aarch64_handle_watchpoint (type, addr, len, 0 /* is_insert */);
 
-  if (debug_hw_points > 1)
+  if (show_debug_regs)
     {
       struct aarch64_debug_reg_state *state
 	= aarch64_get_debug_reg_state (ptid_get_pid (inferior_ptid));
@@ -1496,7 +1492,7 @@ add_show_debug_regs_command (void)
   /* A maintenance command to enable printing the internal DRi mirror
      variables.  */
   add_setshow_boolean_cmd ("show-debug-regs", class_maintenance,
-			   &debug_hw_points, _("\
+			   &show_debug_regs, _("\
 Set whether to show variables that mirror the AArch64 debug registers."), _("\
 Show whether to show variables that mirror the AArch64 debug registers."), _("\
 Use \"on\" to enable, \"off\" to disable.\n\
diff --git a/gdb/common/common-debug.c b/gdb/common/common-debug.c
index 660fc70..5cac3bf 100644
--- a/gdb/common/common-debug.c
+++ b/gdb/common/common-debug.c
@@ -26,6 +26,10 @@
 
 /* See common/common-debug.h.  */
 
+int show_debug_regs;
+
+/* See common/common-debug.h.  */
+
 void
 debug_printf (const char *fmt, ...)
 {
diff --git a/gdb/common/common-debug.h b/gdb/common/common-debug.h
index c2bb192..348b0e3 100644
--- a/gdb/common/common-debug.h
+++ b/gdb/common/common-debug.h
@@ -20,6 +20,11 @@
 #ifndef COMMON_DEBUG_H
 #define COMMON_DEBUG_H
 
+/* Set to nonzero to enable debugging of hardware breakpoint/
+   watchpoint support code.  */
+
+extern int show_debug_regs;
+
 /* Print a formatted message to the appropriate channel for
    debugging output for the client.  */
 
diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index 6066e15..ca096b0 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -267,9 +267,6 @@ aarch64_store_fpregset (struct regcache *regcache, const void *buf)
     supply_register (regcache, AARCH64_V0_REGNO + i, &regset->vregs[i]);
 }
 
-/* Debugging of hardware breakpoint/watchpoint support.  */
-extern int debug_hw_points;
-
 /* Enable miscellaneous debugging output.  The name is historical - it
    was originally used to debug LinuxThreads support.  */
 extern int debug_threads;
@@ -626,7 +623,7 @@ debug_reg_change_callback (struct inferior_list_entry *entry, void *ptr)
   dr_changed_t *dr_changed_ptr;
   dr_changed_t dr_changed;
 
-  if (debug_hw_points)
+  if (show_debug_regs)
     {
       fprintf (stderr, "debug_reg_change_callback: \n\tOn entry:\n");
       fprintf (stderr, "\tpid%d, tid: %ld, dr_changed_bp=0x%llx, "
@@ -677,7 +674,7 @@ debug_reg_change_callback (struct inferior_list_entry *entry, void *ptr)
 	linux_stop_lwp (lwp);
     }
 
-  if (debug_hw_points)
+  if (show_debug_regs)
     {
       fprintf (stderr, "\tOn exit:\n\tpid%d, tid: %ld, dr_changed_bp=0x%llx, "
 	       "dr_changed_wp=0x%llx\n",
@@ -917,7 +914,7 @@ aarch64_handle_unaligned_watchpoint (enum target_hw_bp_type type,
 	ret = aarch64_dr_state_remove_one_point (state, type, aligned_addr,
 						 aligned_len);
 
-      if (debug_hw_points)
+      if (show_debug_regs)
 	fprintf (stderr,
  "handle_unaligned_watchpoint: is_insert: %d\n"
  "                             aligned_addr: 0x%s, aligned_len: %d\n"
@@ -973,7 +970,7 @@ aarch64_insert_point (enum raw_bkpt_type type, CORE_ADDR addr,
   int ret;
   enum target_hw_bp_type targ_type;
 
-  if (debug_hw_points)
+  if (show_debug_regs)
     fprintf (stderr, "insert_point on entry (addr=0x%08lx, len=%d)\n",
 	     (unsigned long) addr, len);
 
@@ -987,7 +984,7 @@ aarch64_insert_point (enum raw_bkpt_type type, CORE_ADDR addr,
     ret =
       aarch64_handle_breakpoint (targ_type, addr, len, 1 /* is_insert */);
 
-  if (debug_hw_points > 1)
+  if (show_debug_regs > 1)
     aarch64_show_debug_reg_state (aarch64_get_debug_reg_state (),
 				  "insert_point", addr, len, targ_type);
 
@@ -1009,7 +1006,7 @@ aarch64_remove_point (enum raw_bkpt_type type, CORE_ADDR addr,
   int ret;
   enum target_hw_bp_type targ_type;
 
-  if (debug_hw_points)
+  if (show_debug_regs)
     fprintf (stderr, "remove_point on entry (addr=0x%08lx, len=%d)\n",
 	     (unsigned long) addr, len);
 
@@ -1024,7 +1021,7 @@ aarch64_remove_point (enum raw_bkpt_type type, CORE_ADDR addr,
     ret =
       aarch64_handle_breakpoint (targ_type, addr, len, 0 /* is_insert */);
 
-  if (debug_hw_points > 1)
+  if (show_debug_regs > 1)
     aarch64_show_debug_reg_state (aarch64_get_debug_reg_state (),
 				  "remove_point", addr, len, targ_type);
 
@@ -1150,7 +1147,7 @@ aarch64_linux_prepare_to_resume (struct lwp_info *lwp)
       struct aarch64_debug_reg_state *state
 	= &proc->private->arch_private->debug_reg_state;
 
-      if (debug_hw_points)
+      if (show_debug_regs)
 	fprintf (stderr, "prepare_to_resume thread %ld\n", lwpid_of (thread));
 
       /* Watchpoints.  */
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 81cb2e1..8d95761 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -69,9 +69,6 @@ int disable_randomization = 1;
 
 static char **program_argv, **wrapper_argv;
 
-/* Enable debugging of h/w breakpoint/watchpoint support.  */
-int debug_hw_points;
-
 int pass_signals[GDB_SIGNAL_LAST];
 int program_signals[GDB_SIGNAL_LAST];
 int program_signals_p;
@@ -1012,12 +1009,12 @@ handle_monitor_command (char *mon, char *own_buf)
     }
   else if (strcmp (mon, "set debug-hw-points 1") == 0)
     {
-      debug_hw_points = 1;
+      show_debug_regs = 1;
       monitor_output ("H/W point debugging output enabled.\n");
     }
   else if (strcmp (mon, "set debug-hw-points 0") == 0)
     {
-      debug_hw_points = 0;
+      show_debug_regs = 0;
       monitor_output ("H/W point debugging output disabled.\n");
     }
   else if (strcmp (mon, "set remote-debug 1") == 0)
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 812f533..ed54f77 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -82,7 +82,6 @@ extern ptid_t cont_thread;
 extern ptid_t general_thread;
 
 extern int server_waiting;
-extern int debug_hw_points;
 extern int pass_signals[];
 extern int program_signals[];
 extern int program_signals_p;
diff --git a/gdb/i386-nat.c b/gdb/i386-nat.c
index 499fffb..4a64759 100644
--- a/gdb/i386-nat.c
+++ b/gdb/i386-nat.c
@@ -33,9 +33,6 @@
    The functions below implement debug registers sharing by reference
    counts, and allow to watch regions up to 16 bytes long.  */
 
-/* Whether or not to print the mirrored debug registers.  */
-int debug_hw_points;
-
 /* Low-level function vector.  */
 struct i386_dr_low_type i386_dr_low;
 
@@ -272,7 +269,7 @@ add_show_debug_regs_command (void)
   /* A maintenance command to enable printing the internal DRi mirror
      variables.  */
   add_setshow_boolean_cmd ("show-debug-regs", class_maintenance,
-			   &debug_hw_points, _("\
+			   &show_debug_regs, _("\
 Set whether to show variables that mirror the x86 debug registers."), _("\
 Show whether to show variables that mirror the x86 debug registers."), _("\
 Use \"on\" to enable, \"off\" to disable.\n\
diff --git a/gdb/mips-linux-nat.c b/gdb/mips-linux-nat.c
index 88faa1e..daf2307 100644
--- a/gdb/mips-linux-nat.c
+++ b/gdb/mips-linux-nat.c
@@ -50,10 +50,6 @@
    we'll clear this and use PTRACE_PEEKUSER instead.  */
 static int have_ptrace_regsets = 1;
 
-/* Whether or not to print the mirrored debug registers.  */
-
-static int maint_show_dr;
-
 /* Saved function pointers to fetch and store a single register using
    PTRACE_PEEKUSER and PTRACE_POKEUSER.  */
 
@@ -689,7 +685,7 @@ mips_linux_insert_watchpoint (struct target_ops *self,
   watch_mirror = regs;
   retval = write_watchpoint_regs ();
 
-  if (maint_show_dr)
+  if (show_debug_regs)
     mips_show_dr ("insert_watchpoint", addr, len, type);
 
   return retval;
@@ -737,7 +733,7 @@ mips_linux_remove_watchpoint (struct target_ops *self,
 
   retval = write_watchpoint_regs ();
 
-  if (maint_show_dr)
+  if (show_debug_regs)
     mips_show_dr ("remove_watchpoint", addr, len, type);
 
   return retval;
@@ -774,7 +770,7 @@ _initialize_mips_linux_nat (void)
   struct target_ops *t;
 
   add_setshow_boolean_cmd ("show-debug-regs", class_maintenance,
-			   &maint_show_dr, _("\
+			   &show_debug_regs, _("\
 Set whether to show variables that mirror the mips debug registers."), _("\
 Show whether to show variables that mirror the mips debug registers."), _("\
 Use \"on\" to enable, \"off\" to disable.\n\
diff --git a/gdb/nat/i386-dregs.c b/gdb/nat/i386-dregs.c
index e3272cd..7afe09b 100644
--- a/gdb/nat/i386-dregs.c
+++ b/gdb/nat/i386-dregs.c
@@ -175,11 +175,6 @@
 /* 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.  */
 
 static void
@@ -511,7 +506,7 @@ i386_dr_insert_watchpoint (struct i386_debug_reg_state *state,
   if (retval == 0)
     i386_update_inferior_debug_regs (state, &local_state);
 
-  if (debug_hw_points)
+  if (show_debug_regs)
     i386_show_dr (state, "insert_watchpoint", addr, len, type);
 
   return retval;
@@ -550,7 +545,7 @@ i386_dr_remove_watchpoint (struct i386_debug_reg_state *state,
   if (retval == 0)
     i386_update_inferior_debug_regs (state, &local_state);
 
-  if (debug_hw_points)
+  if (show_debug_regs)
     i386_show_dr (state, "remove_watchpoint", addr, len, type);
 
   return retval;
@@ -640,12 +635,12 @@ i386_dr_stopped_data_address (struct i386_debug_reg_state *state,
 	{
 	  addr = i386_dr_low_get_addr (i);
 	  rc = 1;
-	  if (debug_hw_points)
+	  if (show_debug_regs)
 	    i386_show_dr (state, "watchpoint_hit", addr, -1, hw_write);
 	}
     }
 
-  if (debug_hw_points && addr == 0)
+  if (show_debug_regs && addr == 0)
     i386_show_dr (state, "stopped_data_addr", 0, 0, hw_write);
 
   if (rc)
-- 
1.7.1

Re: [PATCH 1/9 v7] Introduce show_debug_regs

[Pedro Alves]

On 08/29/2014 02:51 PM, Gary Benson wrote:
> -  if (debug_hw_points)
> +  if (show_debug_regs)
>      fprintf_unfiltered
>        (gdb_stdlog,
>         "insert_hw_breakpoint on entry (addr=0x%08lx, len=%d))\n",
> @@ -1208,7 +1204,7 @@ aarch64_linux_insert_hw_breakpoint (struct target_ops *self,
>  
>    ret = aarch64_handle_breakpoint (type, addr, len, 1 /* is_insert */);
>  
> -  if (debug_hw_points > 1)
> +  if (show_debug_regs)
>      {

Please mention this change (and others like it, afaics, only in the
Aarch64 port) in the commit log.  This is changing behavior, but we're
left with no clue on whether it was investigated and decided the change
is desirable.  The previous intention seems to
be to only show the debug output if a higher verbosity level was shown.
Like e.g., "maint set show-debug-regs 2".  But, Aarch64's
"maint set show-debug-regs" command is registered as a boolean command,
so there is actually no way currently AFAICS for debug_hw_points to
end up '> 1'.  So that was actually dead code, and this change makes it
undead.  That is fine with me, and this is just debug code, and we can
always remove it if it ends up too verbose.  Doug was also OK with the
patch, so go ahead and push with the commit log adjusted.

Thanks,
Pedro Alves

[PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Gary Benson]

This commit introduces two new functions to stop and restart target
processes that shared code can use and that clients must implement.
It also changes some shared code to use these functions.

The changes in this patch replace the target_stop, target_wait
and target_resume parts of this patch I posted on August 1:
https://sourceware.org/ml/gdb-patches/2014-08/msg00014.html
The remainder of that patch is in patch 2 of this series.

gdb/ChangeLog:

	* target/target.h (target_stop_ptid, target_continue_ptid):
	Declare.
	* target.c (target_stop_ptid, target_continue_ptid): New
	functions.
	* common/agent.c [!GDBSERVER]: Don't include infrun.h.
	(agent_run_command): Always use target_stop_ptid and
	target_continue_ptid.

gdb/gdbserver/ChangeLog:

	* target.c (target_stop_ptid, target_continue_ptid): New
	functions.
---
 gdb/ChangeLog           |   10 ++++++++++
 gdb/common/agent.c      |   37 ++-----------------------------------
 gdb/gdbserver/ChangeLog |    5 +++++
 gdb/gdbserver/target.c  |   32 ++++++++++++++++++++++++++++++++
 gdb/target.c            |   25 +++++++++++++++++++++++++
 gdb/target/target.h     |   12 ++++++++++++
 6 files changed, 86 insertions(+), 35 deletions(-)

diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index 9f8ea9a..0ac73a9 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -21,7 +21,6 @@
 #include "server.h"
 #else
 #include "defs.h"
-#include "infrun.h"
 #include "objfiles.h"
 #endif
 #include "target/target.h"
@@ -218,18 +217,7 @@ agent_run_command (int pid, const char *cmd, int len)
   DEBUG_AGENT ("agent: resumed helper thread\n");
 
   /* Resume helper thread.  */
-#ifdef GDBSERVER
-{
-  struct thread_resume resume_info;
-
-  resume_info.thread = ptid;
-  resume_info.kind = resume_continue;
-  resume_info.sig = GDB_SIGNAL_0;
-  (*the_target->resume) (&resume_info, 1);
-}
-#else
- target_resume (ptid, 0, GDB_SIGNAL_0);
-#endif
+  target_continue_ptid (ptid);
 
   fd = gdb_connect_sync_socket (pid);
   if (fd >= 0)
@@ -261,30 +249,9 @@ agent_run_command (int pid, const char *cmd, int len)
   /* Need to read response with the inferior stopped.  */
   if (!ptid_equal (ptid, null_ptid))
     {
-      struct target_waitstatus status;
-      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;
+      target_stop_ptid (ptid);
     }
 
   if (fd >= 0)
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index 08506e5..e51b7db 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -134,6 +134,38 @@ mywait (ptid_t ptid, struct target_waitstatus *ourstatus, int options,
   return ret;
 }
 
+/* See target/target.h.  */
+
+void
+target_stop_ptid (ptid_t ptid)
+{
+  struct target_waitstatus status;
+  int was_non_stop = non_stop;
+  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);
+  non_stop = was_non_stop;
+}
+
+/* See target/target.h.  */
+
+void
+target_continue_ptid (ptid_t ptid)
+{
+  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);
+}
+
 int
 start_non_stop (int nonstop)
 {
diff --git a/gdb/target.c b/gdb/target.c
index 711e7cb..339b1d1 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3027,6 +3027,31 @@ target_stop (ptid_t ptid)
   (*current_target.to_stop) (&current_target, ptid);
 }
 
+/* See target/target.h.  */
+
+void
+target_stop_ptid (ptid_t ptid)
+{
+  struct target_waitstatus status;
+  int was_non_stop = non_stop;
+
+  non_stop = 1;
+  target_stop (ptid);
+
+  memset (&status, 0, sizeof (status));
+  target_wait (ptid, &status, 0);
+
+  non_stop = was_non_stop;
+}
+
+/* See target/target.h.  */
+
+void
+target_continue_ptid (ptid_t ptid)
+{
+  target_resume (ptid, 0, GDB_SIGNAL_0);
+}
+
 /* Concatenate ELEM to LIST, a comma separate list, and return the
    result.  The LIST incoming argument is released.  */
 
diff --git a/gdb/target/target.h b/gdb/target/target.h
index 0a3c4b7..902de64 100644
--- a/gdb/target/target.h
+++ b/gdb/target/target.h
@@ -59,4 +59,16 @@ extern int target_read_uint32 (CORE_ADDR memaddr, uint32_t *result);
 extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
 				ssize_t len);
 
+/* Make target stop in a continuable fashion.  (For instance, under
+   Unix, this should act like SIGSTOP).  This function must be
+   provided by the client.  This function is normally used by GUIs
+   to implement a stop button.  */
+
+extern void target_stop_ptid (ptid_t ptid);
+
+/* Restart a target that was previously stopped by target_stop_ptid.
+   This function must be provided by the client.  */
+
+extern void target_continue_ptid (ptid_t ptid);
+
 #endif /* TARGET_COMMON_H */
-- 
1.7.1

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Pedro Alves]

On 08/29/2014 02:51 PM, Gary Benson wrote:
> This commit introduces two new functions to stop and restart target
> processes that shared code can use and that clients must implement.
> It also changes some shared code to use these functions.
> 
> The changes in this patch replace the target_stop, target_wait
> and target_resume parts of this patch I posted on August 1:
> https://sourceware.org/ml/gdb-patches/2014-08/msg00014.html
> The remainder of that patch is in patch 2 of this series.
> 
> gdb/ChangeLog:
> 
> 	* target/target.h (target_stop_ptid, target_continue_ptid):
> 	Declare.
> 	* target.c (target_stop_ptid, target_continue_ptid): New
> 	functions.
> 	* common/agent.c [!GDBSERVER]: Don't include infrun.h.
> 	(agent_run_command): Always use target_stop_ptid and
> 	target_continue_ptid.
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* target.c (target_stop_ptid, target_continue_ptid): New
> 	functions.

OK, with ...

> +/* Make target stop in a continuable fashion.  (For instance, under
> +   Unix, this should act like SIGSTOP).  This function must be
> +   provided by the client.  This function is normally used by GUIs
> +   to implement a stop button.  */

... Please drop the last sentence referring to GUIs.

> +
> +extern void target_stop_ptid (ptid_t ptid);

Thanks,
Pedro Alves

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Doug Evans]

Gary Benson writes:
 > This commit introduces two new functions to stop and restart target
 > processes that shared code can use and that clients must implement.
 > It also changes some shared code to use these functions.
 > 
 > The changes in this patch replace the target_stop, target_wait
 > and target_resume parts of this patch I posted on August 1:
 > https://sourceware.org/ml/gdb-patches/2014-08/msg00014.html
 > The remainder of that patch is in patch 2 of this series.
 > 
 > gdb/ChangeLog:
 > 
 > 	* target/target.h (target_stop_ptid, target_continue_ptid):
 > 	Declare.
 > 	* target.c (target_stop_ptid, target_continue_ptid): New
 > 	functions.
 > 	* common/agent.c [!GDBSERVER]: Don't include infrun.h.
 > 	(agent_run_command): Always use target_stop_ptid and
 > 	target_continue_ptid.
 > 
 > gdb/gdbserver/ChangeLog:
 > 
 > 	* target.c (target_stop_ptid, target_continue_ptid): New
 > 	functions.
 > [...]
 > diff --git a/gdb/target.c b/gdb/target.c
 > index 711e7cb..339b1d1 100644
 > --- a/gdb/target.c
 > +++ b/gdb/target.c
 > @@ -3027,6 +3027,31 @@ target_stop (ptid_t ptid)
 >    (*current_target.to_stop) (&current_target, ptid);
 >  }
 >  
 > [...]
 > +/* See target/target.h.  */
 > +
 > +void
 > +target_continue_ptid (ptid_t ptid)
 > +{
 > +  target_resume (ptid, 0, GDB_SIGNAL_0);
 > +}
 > +

Hi.
How come GDB_SIGNAL_0 is used here?
Maybe it's correct, but it's not immediately clear.

The reason I ask is because there are two ways to "continue"
the inferior:
1) resume it where it left off, and if it stopped because
   of a signal then forward on that signal (assuming the
   signal is not "nopass") (GDB_SIGNAL_DEFAULT).
2) Either inject a new signal (GDB_SIGNAL_FOO) or cancel out a previously
   queued signal (GDB_SIGNAL_0).

GDB_SIGNAL_0 is used to resume the target and discarding
any signal that it may have stopped for.
GDB_SIGNAL_DEFAULT is used for (1).

I realize the comments for target_resume say to not pass
GDB_SIGNAL_DEFAULT to it.  But the name "target_continue_ptid"
with no option to choose between (1) and (2)
says to me "do what GDB_SIGNAL_DEFAULT" does.

 > +/* Restart a target that was previously stopped by target_stop_ptid.
 > +   This function must be provided by the client.  */
 > +
 > +extern void target_continue_ptid (ptid_t ptid);
 > +

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
> > This commit introduces two new functions to stop and restart
> > target processes that shared code can use and that clients must
> > implement.  It also changes some shared code to use these
> > functions.
> [...]
> > +/* See target/target.h.  */
> > +
> > +void
> > +target_continue_ptid (ptid_t ptid)
> > +{
> > +  target_resume (ptid, 0, GDB_SIGNAL_0);
> > +}
> 
> How come GDB_SIGNAL_0 is used here?
> Maybe it's correct, but it's not immediately clear.
> 
> The reason I ask is because there are two ways to "continue"
> the inferior:
> 1) resume it where it left off, and if it stopped because
>    of a signal then forward on that signal (assuming the
>    signal is not "nopass") (GDB_SIGNAL_DEFAULT).
> 2) Either inject a new signal (GDB_SIGNAL_FOO) or cancel out
>    a previously queued signal (GDB_SIGNAL_0).
> 
> GDB_SIGNAL_0 is used to resume the target and discarding
> any signal that it may have stopped for.
> GDB_SIGNAL_DEFAULT is used for (1).
> 
> I realize the comments for target_resume say to not pass
> GDB_SIGNAL_DEFAULT to it.  But the name "target_continue_ptid"
> with no option to choose between (1) and (2)
> says to me "do what GDB_SIGNAL_DEFAULT" does.

I don't know the answer to this, I just moved the code from one
place to the next :)  Possibly it's because (I think) under the
hood target_stop_ptid sends a SIGSTOP to the inferior, so you
don't want to restart it with that signal queued.

The comment for target_continue_ptid says:

> > +/* Restart a target that was previously stopped by target_stop_ptid.
> > +   This function must be provided by the client.  */

That implies to me "don't use this for targets not previously
stopped by target_stop_ptid".

Maybe someone more familiar with this code could elaborate?

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Pedro Alves]

On 09/11/2014 11:26 AM, Gary Benson wrote:
> Doug Evans wrote:
>> Gary Benson writes:
>>> This commit introduces two new functions to stop and restart
>>> target processes that shared code can use and that clients must
>>> implement.  It also changes some shared code to use these
>>> functions.
>> [...]
>>> +/* See target/target.h.  */
>>> +
>>> +void
>>> +target_continue_ptid (ptid_t ptid)
>>> +{
>>> +  target_resume (ptid, 0, GDB_SIGNAL_0);
>>> +}
>>
>> How come GDB_SIGNAL_0 is used here?
>> Maybe it's correct, but it's not immediately clear.
>>
>> The reason I ask is because there are two ways to "continue"
>> the inferior:
>> 1) resume it where it left off, and if it stopped because
>>    of a signal then forward on that signal (assuming the
>>    signal is not "nopass") (GDB_SIGNAL_DEFAULT).
>> 2) Either inject a new signal (GDB_SIGNAL_FOO) or cancel out
>>    a previously queued signal (GDB_SIGNAL_0).
>>
>> GDB_SIGNAL_0 is used to resume the target and discarding
>> any signal that it may have stopped for.
>> GDB_SIGNAL_DEFAULT is used for (1).
>>
>> I realize the comments for target_resume say to not pass
>> GDB_SIGNAL_DEFAULT to it.  But the name "target_continue_ptid"
>> with no option to choose between (1) and (2)
>> says to me "do what GDB_SIGNAL_DEFAULT" does.
> 
> I don't know the answer to this, I just moved the code from one
> place to the next :)  Possibly it's because (I think) under the
> hood target_stop_ptid sends a SIGSTOP to the inferior, so you
> don't want to restart it with that signal queued.

No, that's not it.  The SIGSTOP is an internal detail of the
target implementation.  It's reported to the target_wait caller
as GDB_SIGNAL_0 (meaning, the thread stopped for no reason
other than that GDB wanted it to stop).

> The comment for target_continue_ptid says:
> 
>>> +/* Restart a target that was previously stopped by target_stop_ptid.
>>> +   This function must be provided by the client.  */
> 
> That implies to me "don't use this for targets not previously
> stopped by target_stop_ptid".
> 
> Maybe someone more familiar with this code could elaborate?

I guess that'd be me...

In the case that target_stop_ptid returns anything else other
than GDB_SIGNAL_0, resuming with GDB_SIGNAL_0, GDB_SIGNAL_DEFAULT or
the signal that the thread had stopped for would all be wrong.
If we got a synchronous SIGSEGV, for instance, we'd want to abort
the agent call and return error.  We'd likely disable access to the
agent from there on, as it's messed up.  If instead we got an
asynchronous signal, we'd want to hold on to it, and requeue it
later, once the agent command is done with.

[As an example showing this same complication being handled
similarly, see See enqueue_one_deferred_signal and
collecting_fast_tracepoint in gdbserver/linux-low.c.  This
handles the case of getting signals when they arrive while
a thread is in a fast tracepoint jump pad.    That's a lower
level though.]

But, we have to look at where/how this is presently being used
to understand why the current code got away from handling
that complication.

The thread that we're interacting with blocks all signals.
See gdbserver/tracepoint.c:

static void
gdb_agent_init (void)
{
  int res;
  pthread_t thread;
  sigset_t new_mask;
  sigset_t orig_mask;

  /* We want the helper thread to be as transparent as possible, so
     have it inherit an all-signals-blocked mask.  */

  sigfillset (&new_mask);
  res = pthread_sigmask (SIG_SETMASK, &new_mask, &orig_mask);
  if (res)

and, we run commands with all breakpoints lifted:

/* Ask the in-process agent to run a command.  Since we don't want to
   have to handle the IPA hitting breakpoints while running the
   command, we pause all threads, remove all breakpoints, and then set
   the helper thread re-running.  We communicate with the helper
   thread by means of direct memory xfering, and a socket for
   synchronization.  */

static int
run_inferior_command (char *cmd, int len)
{
  int err = -1;
  int pid = ptid_get_pid (current_ptid);

  trace_debug ("run_inferior_command: running: %s", cmd);

  pause_all (0);
  uninsert_all_breakpoints ();

  err = agent_run_command (pid, (const char *) cmd, len);



If we want to reuse target_stop_ptid for other cases in the
future, we'll likely make it return the stop waitstatus then.

For now, I think just documenting target_continue_ptid as
resuming with no signal is good enough.

Thanks,
Pedro Alves

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Doug Evans]

Pedro Alves writes:
 > On 09/11/2014 11:26 AM, Gary Benson wrote:
 > > Doug Evans wrote:
 > >> Gary Benson writes:
 > >>> This commit introduces two new functions to stop and restart
 > >>> target processes that shared code can use and that clients must
 > >>> implement.  It also changes some shared code to use these
 > >>> functions.
 > >> [...]
 > >>> +/* See target/target.h.  */
 > >>> +
 > >>> +void
 > >>> +target_continue_ptid (ptid_t ptid)
 > >>> +{
 > >>> +  target_resume (ptid, 0, GDB_SIGNAL_0);
 > >>> +}
 > >>
 > >> How come GDB_SIGNAL_0 is used here?
 > >> Maybe it's correct, but it's not immediately clear.
 > >>
 > >> The reason I ask is because there are two ways to "continue"
 > >> the inferior:
 > >> 1) resume it where it left off, and if it stopped because
 > >>    of a signal then forward on that signal (assuming the
 > >>    signal is not "nopass") (GDB_SIGNAL_DEFAULT).
 > >> 2) Either inject a new signal (GDB_SIGNAL_FOO) or cancel out
 > >>    a previously queued signal (GDB_SIGNAL_0).
 > >>
 > >> GDB_SIGNAL_0 is used to resume the target and discarding
 > >> any signal that it may have stopped for.
 > >> GDB_SIGNAL_DEFAULT is used for (1).
 > >>
 > >> I realize the comments for target_resume say to not pass
 > >> GDB_SIGNAL_DEFAULT to it.  But the name "target_continue_ptid"
 > >> with no option to choose between (1) and (2)
 > >> says to me "do what GDB_SIGNAL_DEFAULT" does.
 >
 > [...]
 > 
 > For now, I think just documenting target_continue_ptid as
 > resuming with no signal is good enough.

That may be sufficient for me to make this patch checkin-able,
but before then I'd like to understand how and where
this function will be used from gdb.

btw, how about target_continue_with_no_signal (ptid_t ptid) ?

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Pedro Alves]

On 09/12/2014 05:53 PM, Doug Evans wrote:

> That may be sufficient for me to make this patch checkin-able,
> but before then I'd like to understand how and where
> this function will be used from gdb.

Can you clarify?  I don't have any plan to use this
elsewhere myself.  All we're doing is factoring out
the current use behind a common function so that both gdb
and gdbserver can implement it their own way.

> btw, how about target_continue_with_no_signal (ptid_t ptid) ?

Fine with me.

Thanks,
Pedro Alves

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Doug Evans]

Pedro Alves writes:
 > On 09/12/2014 05:53 PM, Doug Evans wrote:
 > 
 > > That may be sufficient for me to make this patch checkin-able,
 > > but before then I'd like to understand how and where
 > > this function will be used from gdb.
 > 
 > Can you clarify?  I don't have any plan to use this
 > elsewhere myself.  All we're doing is factoring out
 > the current use behind a common function so that both gdb
 > and gdbserver can implement it their own way.

I don't see the function being used in gdb by this patch set.
So, absent further info, the change to gdb/target.c is just adding
dead code.  I'm assuming that's not the case (*1), but until I
understand how it's going to be used in gdb it's not clear to
me whether code that uses it will be confusing to read.

(*1): I could have missed its use.
But even if it's dead code today, I don't mind it
in this particular case.

 > > btw, how about target_continue_with_no_signal (ptid_t ptid) ?
 > 
 > Fine with me.

In which case I'm happy.
Code that uses that name can be read unambiguously
(to me anyway).

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Pedro Alves]

On 09/12/2014 06:38 PM, Doug Evans wrote:
> Pedro Alves writes:
>  > On 09/12/2014 05:53 PM, Doug Evans wrote:
>  > 
>  > > That may be sufficient for me to make this patch checkin-able,
>  > > but before then I'd like to understand how and where
>  > > this function will be used from gdb.
>  > 
>  > Can you clarify?  I don't have any plan to use this
>  > elsewhere myself.  All we're doing is factoring out
>  > the current use behind a common function so that both gdb
>  > and gdbserver can implement it their own way.
> 
> I don't see the function being used in gdb by this patch set.
> So, absent further info, the change to gdb/target.c is just adding
> dead code.  I'm assuming that's not the case (*1), but until I
> understand how it's going to be used in gdb it's not clear to
> me whether code that uses it will be confusing to read.

It's used in common/agent.c, which is used by gdb as well.

 linux-nat.c:  agent_run_command (pid, s, strlen (s) + 1);
 linux-nat.c:      agent_run_command (pid, s, strlen (s) + 1);

It's because it's used in gdb that the current code, before
Gary's patch has the #ifdef GDBSERVER #else /* gdb code */ bits.
He's just moving that code around.

Thanks,
Pedro Alves

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Doug Evans]

Pedro Alves writes:
 > On 09/12/2014 06:38 PM, Doug Evans wrote:
 > > Pedro Alves writes:
 > >  > On 09/12/2014 05:53 PM, Doug Evans wrote:
 > >  > 
 > >  > > That may be sufficient for me to make this patch checkin-able,
 > >  > > but before then I'd like to understand how and where
 > >  > > this function will be used from gdb.
 > >  > 
 > >  > Can you clarify?  I don't have any plan to use this
 > >  > elsewhere myself.  All we're doing is factoring out
 > >  > the current use behind a common function so that both gdb
 > >  > and gdbserver can implement it their own way.
 > > 
 > > I don't see the function being used in gdb by this patch set.
 > > So, absent further info, the change to gdb/target.c is just adding
 > > dead code.  I'm assuming that's not the case (*1), but until I
 > > understand how it's going to be used in gdb it's not clear to
 > > me whether code that uses it will be confusing to read.
 > 
 > It's used in common/agent.c, which is used by gdb as well.
 > 
 >  linux-nat.c:  agent_run_command (pid, s, strlen (s) + 1);
 >  linux-nat.c:      agent_run_command (pid, s, strlen (s) + 1);
 > 
 > It's because it's used in gdb that the current code, before
 > Gary's patch has the #ifdef GDBSERVER #else /* gdb code */ bits.
 > He's just moving that code around.

Ah.
I'm liking target_continue_with_no_signal even more.
[assuming one wants to leave the signature as is]

btw, I noticed this in linux-nat.c:

  /* Pause all */
  target_stop (ptid);

  memcpy (s, "qTfSTM", sizeof ("qTfSTM"));
  s[sizeof ("qTfSTM")] = 0;

  agent_run_command (pid, s, strlen (s) + 1);

And I remembered you saying:

 > I just now noticed the elephant in the room -- target_stop is
 > asynchronous, doesn't wait for a stop, while and target_stop_ptid
 > is synchronous.  [...]

If the above code is right, I think a clarifying comment
is required somewhere.  It's odd that one can call agent_run_command
when the inferior may or may not be stopped yet.
[Or is there a bug here? - if I'm reading the gdbserver version
correctly it first waits for the inferior to stop]

The function comment for target_stop says nothing one way or the other
about whether it's asynchronous which doesn't help.

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Pedro Alves]

On 09/12/2014 07:08 PM, Doug Evans wrote:

> And I remembered you saying:
> 
>  > I just now noticed the elephant in the room -- target_stop is
>  > asynchronous, doesn't wait for a stop, while and target_stop_ptid
>  > is synchronous.  [...]
> 
> If the above code is right, I think a clarifying comment
> is required somewhere.  It's odd that one can call agent_run_command
> when the inferior may or may not be stopped yet.
> [Or is there a bug here? - if I'm reading the gdbserver version
> correctly it first waits for the inferior to stop]

It's a bug.

(Note that the GDB side interfaces with an out-of-tree
agent, not GDBserver's agent.  I don't know the status of
that agent.)

Thanks,
Pedro Alves

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Doug Evans]

On Fri, Sep 12, 2014 at 11:19 AM, Pedro Alves <palves@redhat.com> wrote:
> On 09/12/2014 07:08 PM, Doug Evans wrote:
>
>> And I remembered you saying:
>>
>>  > I just now noticed the elephant in the room -- target_stop is
>>  > asynchronous, doesn't wait for a stop, while and target_stop_ptid
>>  > is synchronous.  [...]
>>
>> If the above code is right, I think a clarifying comment
>> is required somewhere.  It's odd that one can call agent_run_command
>> when the inferior may or may not be stopped yet.
>> [Or is there a bug here? - if I'm reading the gdbserver version
>> correctly it first waits for the inferior to stop]
>
> It's a bug.
>
> (Note that the GDB side interfaces with an out-of-tree
> agent, not GDBserver's agent.  I don't know the status of
> that agent.)

Heh.
Data point that target_stop should be named target_stop_async?
1/2 :-)

[I'd like to point out that all I had to do was read the code (glance
is more apt) to see the bug (given that your comment about target_stop
was still fresh in my mind).  Unambiguous/clear function names are
worth at least some effort.]

btw, out-of-tree agent, and not gdbserver's agent?
That's odd.  Where would I find this agent?

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Gary Benson]

Doug Evans wrote:
> On Fri, Sep 12, 2014 at 11:19 AM, Pedro Alves <palves@redhat.com> wrote:
> > On 09/12/2014 07:08 PM, Doug Evans wrote:
> > > Pedro Alves wrote:
> > > > I just now noticed the elephant in the room -- target_stop
> > > > is asynchronous, doesn't wait for a stop, while and
> > > > target_stop_ptid is synchronous.  [...]
> > >
> > > If the above code is right, I think a clarifying comment is
> > > required somewhere.  It's odd that one can call
> > > agent_run_command when the inferior may or may not be stopped
> > > yet.  [Or is there a bug here? - if I'm reading the gdbserver
> > > version correctly it first waits for the inferior to stop]
> >
> > It's a bug.
> >
> > (Note that the GDB side interfaces with an out-of-tree
> > agent, not GDBserver's agent.  I don't know the status of
> > that agent.)
> 
> Data point that target_stop should be named target_stop_async?

Ok, can I get a summary of this thread, I'm struggling to follow it.

 a) What should the functions be called:
     - target_stop_async / target_stop_wait
     - target_continue_async / target_continue_no_signal
     - something else?

 b) Is there a bug here I need to address?

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Doug Evans]

On Mon, Sep 15, 2014 at 3:07 AM, Gary Benson <gbenson@redhat.com> wrote:
> Doug Evans wrote:
>> On Fri, Sep 12, 2014 at 11:19 AM, Pedro Alves <palves@redhat.com> wrote:
>> > On 09/12/2014 07:08 PM, Doug Evans wrote:
>> > > Pedro Alves wrote:
>> > > > I just now noticed the elephant in the room -- target_stop
>> > > > is asynchronous, doesn't wait for a stop, while and
>> > > > target_stop_ptid is synchronous.  [...]
>> > >
>> > > If the above code is right, I think a clarifying comment is
>> > > required somewhere.  It's odd that one can call
>> > > agent_run_command when the inferior may or may not be stopped
>> > > yet.  [Or is there a bug here? - if I'm reading the gdbserver
>> > > version correctly it first waits for the inferior to stop]
>> >
>> > It's a bug.
>> >
>> > (Note that the GDB side interfaces with an out-of-tree
>> > agent, not GDBserver's agent.  I don't know the status of
>> > that agent.)
>>
>> Data point that target_stop should be named target_stop_async?
>
> Ok, can I get a summary of this thread, I'm struggling to follow it.
>
>  a) What should the functions be called:
>      - target_stop_async / target_stop_wait
>      - target_continue_async / target_continue_no_signal
>      - something else?
>
>  b) Is there a bug here I need to address?

At this point I think we're still in discussion mode, there are no
conclusions/agreements yet, except for the agreement to use
target_continue_with_no_signal instead of target_continue_ptid.

To advance the discussion,
the async case is the subtle case IMO.  Evidently (and I'm just going
by what I see, there may be more data here) someone (*1) looked at the
name "target_stop" and thought it was async (which is probably what
I'd do).  The function comment doesn't specify.  One could argue it's
sufficient to just fix the function comment, but if we're going to
have a mix of similar functions, some of which are async and some
sync, then IMO we should also make the difference stand out in the
code where it's read.  I'd be happy with a convention where all async
functions ended with _async or _no_wait (the latter reads better to
me), but I'm guessing I'd be happy with a different convention as
well.

FAOD,
there is a bug, but it's not one you specifically need to address.
I pointed it out because it's a data point that contributes to the discussion.

(*1): I've looked at git log and blame. I'm speaking generically here
because I think this is unlikely to be a one-off kind of issue. Plus I
can well imagine me making a similar mistake too.  Plus the bug got
through code review.

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Doug Evans]

On Mon, Sep 15, 2014 at 9:00 AM, Doug Evans <dje@google.com> wrote:
> [...]
> To advance the discussion,
> the async case is the subtle case IMO.  Evidently (and I'm just going
> by what I see, there may be more data here) someone (*1) looked at the
> name "target_stop" and thought it was async (which is probably what

Bleah.  s/async/synchronous/

> I'd do).  The function comment doesn't specify.  One could argue it's
> sufficient to just fix the function comment, but if we're going to
> have a mix of similar functions, some of which are async and some
> sync, then IMO we should also make the difference stand out in the
> code where it's read.  I'd be happy with a convention where all async
> functions ended with _async or _no_wait (the latter reads better to
> me), but I'm guessing I'd be happy with a different convention as
> well.
>
> FAOD,
> there is a bug, but it's not one you specifically need to address.
> I pointed it out because it's a data point that contributes to the discussion.
>
> (*1): I've looked at git log and blame. I'm speaking generically here
> because I think this is unlikely to be a one-off kind of issue. Plus I
> can well imagine me making a similar mistake too.  Plus the bug got
> through code review.

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Gary Benson]

Doug Evans wrote:
> On Mon, Sep 15, 2014 at 3:07 AM, Gary Benson <gbenson@redhat.com> wrote:
> > Doug Evans wrote:
> > > On Fri, Sep 12, 2014 at 11:19 AM, Pedro Alves <palves@redhat.com> wrote:
> > > > On 09/12/2014 07:08 PM, Doug Evans wrote:
> > > > > Pedro Alves wrote:
> > > > > > I just now noticed the elephant in the room -- target_stop
> > > > > > is asynchronous, doesn't wait for a stop, while and
> > > > > > target_stop_ptid is synchronous.  [...]
> > > > >
> > > > > If the above code is right, I think a clarifying comment is
> > > > > required somewhere.  It's odd that one can call
> > > > > agent_run_command when the inferior may or may not be stopped
> > > > > yet.  [Or is there a bug here? - if I'm reading the gdbserver
> > > > > version correctly it first waits for the inferior to stop]
> > > >
> > > > It's a bug.
> > > >
> > > > (Note that the GDB side interfaces with an out-of-tree
> > > > agent, not GDBserver's agent.  I don't know the status of
> > > > that agent.)
> > >
> > > Data point that target_stop should be named target_stop_async?
> >
> > Ok, can I get a summary of this thread, I'm struggling to follow it.
> >
> >  a) What should the functions be called:
> >      - target_stop_async / target_stop_wait
> >      - target_continue_async / target_continue_no_signal
> >      - something else?
> >
> >  b) Is there a bug here I need to address?
> 
> At this point I think we're still in discussion mode, there are no
> conclusions/agreements yet, except for the agreement to use
> target_continue_with_no_signal instead of target_continue_ptid.
> 
> To advance the discussion,
> the async case is the subtle case IMO.  Evidently (and I'm just going
> by what I see, there may be more data here) someone (*1) looked at the
> name "target_stop" and thought it was async (which is probably what
> I'd do).  The function comment doesn't specify.  One could argue it's
> sufficient to just fix the function comment, but if we're going to
> have a mix of similar functions, some of which are async and some
> sync, then IMO we should also make the difference stand out in the
> code where it's read.  I'd be happy with a convention where all async
> functions ended with _async or _no_wait (the latter reads better to
> me), but I'm guessing I'd be happy with a different convention as
> well.
> 
> FAOD,
> there is a bug, but it's not one you specifically need to address.
> I pointed it out because it's a data point that contributes to the discussion.
> 
> (*1): I've looked at git log and blame. I'm speaking generically here
> because I think this is unlikely to be a one-off kind of issue. Plus I
> can well imagine me making a similar mistake too.  Plus the bug got
> through code review.

Ok, so what I think you're saying is:

 a) The target_stop method in GDB should be renamed target_stop_async

 b) The common code target_stop_ptid should be renamed... target_stop ?

 c) The bug is that in this code in linux-nat.c:

      /* Pause all */
      target_stop (ptid);

      memcpy (s, "qTfSTM", sizeof ("qTfSTM"));
      s[sizeof ("qTfSTM")] = 0;

      agent_run_command (pid, s, strlen (s) + 1);

    the "target_stop" should actually be a call to whatever
    target_stop_ptid is renamed to.

One final thing--and I may now be going over something that has been
discussed before--could target_continue_ptid be better renamed as
target_resume_no_signal?  It seems to sit better alongside GDB's
target_resume.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Pedro Alves]

On 09/16/2014 10:49 AM, Gary Benson wrote:

> 
>  c) The bug is that in this code in linux-nat.c:
> 
>       /* Pause all */
>       target_stop (ptid);
> 
>       memcpy (s, "qTfSTM", sizeof ("qTfSTM"));
>       s[sizeof ("qTfSTM")] = 0;
> 
>       agent_run_command (pid, s, strlen (s) + 1);
> 
>     the "target_stop" should actually be a call to whatever
>     target_stop_ptid is renamed to.
> 
> One final thing--and I may now be going over something that has been
> discussed before--could target_continue_ptid be better renamed as
> target_resume_no_signal?  It seems to sit better alongside GDB's
> target_resume.

No, the "resume" interface can "continue" and "step" (and on gdbserver,
stop/interrupt too), but target_continue_ptid _only_ continues.

Thanks,
Pedro Alves

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Pedro Alves]

On 09/15/2014 05:00 PM, Doug Evans wrote:
> On Mon, Sep 15, 2014 at 3:07 AM, Gary Benson <gbenson@redhat.com> wrote:
>> Doug Evans wrote:
>>> On Fri, Sep 12, 2014 at 11:19 AM, Pedro Alves <palves@redhat.com> wrote:
>>>> On 09/12/2014 07:08 PM, Doug Evans wrote:
>>>>> Pedro Alves wrote:
>>>>>> I just now noticed the elephant in the room -- target_stop
>>>>>> is asynchronous, doesn't wait for a stop, while and
>>>>>> target_stop_ptid is synchronous.  [...]
>>>>>
>>>>> If the above code is right, I think a clarifying comment is
>>>>> required somewhere.  It's odd that one can call
>>>>> agent_run_command when the inferior may or may not be stopped
>>>>> yet.  [Or is there a bug here? - if I'm reading the gdbserver
>>>>> version correctly it first waits for the inferior to stop]
>>>>
>>>> It's a bug.
>>>>
>>>> (Note that the GDB side interfaces with an out-of-tree
>>>> agent, not GDBserver's agent.  I don't know the status of
>>>> that agent.)
>>>
>>> Data point that target_stop should be named target_stop_async?
>>
>> Ok, can I get a summary of this thread, I'm struggling to follow it.
>>
>>  a) What should the functions be called:
>>      - target_stop_async / target_stop_wait
>>      - target_continue_async / target_continue_no_signal
>>      - something else?
>>
>>  b) Is there a bug here I need to address?
> 
> At this point I think we're still in discussion mode, there are no
> conclusions/agreements yet, except for the agreement to use
> target_continue_with_no_signal instead of target_continue_ptid.
> 
> To advance the discussion,
> the async case is the subtle case IMO.  Evidently (and I'm just going
> by what I see, there may be more data here) someone (*1) looked at the
> name "target_stop" and thought it was async (which is probably what
> I'd do).  The function comment doesn't specify.  One could argue it's
> sufficient to just fix the function comment, but if we're going to
> have a mix of similar functions, some of which are async and some
> sync, then IMO we should also make the difference stand out in the
> code where it's read. I'd be happy with a convention where all async
> functions ended with _async or _no_wait (the latter reads better to
> me), but I'm guessing I'd be happy with a different convention as
> well.

At first blush, this looks to me like looking for a generalization / hard
rule where one might not be necessary, or otherwise, we'd need more a
better/clearer rule/constrain on which methods this would apply to.  E.g.,
would you rename target_xfer_partial?  Certainly memory accesses can
be expected to work in an asynchronous way, with read results being sent
over with asynchronous notifications (even though we don't do it like that).
What about target_resume?  I'm not sure renaming that would be useful,
for example.  So I'd like to see a comprehensive list of functions
that'd you're proposing would be affected in order to be able to
comment further or even be able to see the same patterns you're seeing.

target_stop_and_wait (the existing target_stop_ptid) isn't really a
target method, unlike target_stop, target_resume, etc. -- it's a
helper method that sits on top of target methods.  So I'd go with
clarifying target_stop's "asyncness" in its comments, name the
helper target_stop_and_wait.  I think that's already better
than the status quo.  And then we can discuss conventions and
(potentially wholesale) renaming separately.  It's not a big deal
if we have to rename the new function again.

Thanks,
Pedro Alves

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Doug Evans]

On Tue, Sep 16, 2014 at 3:36 AM, Pedro Alves <palves@redhat.com> wrote:
> On 09/15/2014 05:00 PM, Doug Evans wrote:
>> On Mon, Sep 15, 2014 at 3:07 AM, Gary Benson <gbenson@redhat.com> wrote:
>>> Doug Evans wrote:
>>>> On Fri, Sep 12, 2014 at 11:19 AM, Pedro Alves <palves@redhat.com> wrote:
>>>>> On 09/12/2014 07:08 PM, Doug Evans wrote:
>>>>>> Pedro Alves wrote:
>>>>>>> I just now noticed the elephant in the room -- target_stop
>>>>>>> is asynchronous, doesn't wait for a stop, while and
>>>>>>> target_stop_ptid is synchronous.  [...]
>>>>>>
>>>>>> If the above code is right, I think a clarifying comment is
>>>>>> required somewhere.  It's odd that one can call
>>>>>> agent_run_command when the inferior may or may not be stopped
>>>>>> yet.  [Or is there a bug here? - if I'm reading the gdbserver
>>>>>> version correctly it first waits for the inferior to stop]
>>>>>
>>>>> It's a bug.
>>>>>
>>>>> (Note that the GDB side interfaces with an out-of-tree
>>>>> agent, not GDBserver's agent.  I don't know the status of
>>>>> that agent.)
>>>>
>>>> Data point that target_stop should be named target_stop_async?
>>>
>>> Ok, can I get a summary of this thread, I'm struggling to follow it.
>>>
>>>  a) What should the functions be called:
>>>      - target_stop_async / target_stop_wait
>>>      - target_continue_async / target_continue_no_signal
>>>      - something else?
>>>
>>>  b) Is there a bug here I need to address?
>>
>> At this point I think we're still in discussion mode, there are no
>> conclusions/agreements yet, except for the agreement to use
>> target_continue_with_no_signal instead of target_continue_ptid.
>>
>> To advance the discussion,
>> the async case is the subtle case IMO.  Evidently (and I'm just going
>> by what I see, there may be more data here) someone (*1) looked at the
>> name "target_stop" and thought it was async (which is probably what
>> I'd do).  The function comment doesn't specify.  One could argue it's
>> sufficient to just fix the function comment, but if we're going to
>> have a mix of similar functions, some of which are async and some
>> sync, then IMO we should also make the difference stand out in the
>> code where it's read. I'd be happy with a convention where all async
>> functions ended with _async or _no_wait (the latter reads better to
>> me), but I'm guessing I'd be happy with a different convention as
>> well.
>
> At first blush, this looks to me like looking for a generalization / hard
> rule where one might not be necessary, or otherwise, we'd need more a
> better/clearer rule/constrain on which methods this would apply to.  E.g.,
> would you rename target_xfer_partial?  Certainly memory accesses can
> be expected to work in an asynchronous way, with read results being sent
> over with asynchronous notifications (even though we don't do it like that).

Are you asking about target_xfer_partial as it exists today,
or a hypothetical version where it is used to request the transfer
but the transfer happens asynchronously? [Or both? 1/2 :-)]
I think it's more the latter, but I'll answer for both.

When I think of a "read memory" operation, absent any info specifying
sync/async, I'd expect that the request completes before control is
returned to the caller.  If it didn't wait for the request to complete that
would be unexpected.  Therefore I'd leave the name of
target_xfer_partial as it exists today alone.
That same reasoning to me says a hypothetical "memory read"
request that was asynchronous should have something in its
name that says to the reader "Hey, this is async!  Heads up!".

> What about target_resume?  I'm not sure renaming that would be useful,
> for example.

I thought about target_resume.  It was an semi-interesting case
that immediately popped into my head at the time.
And then I tried to think how the typical reader would interpret it.
I'm not a typical reader, but I think(!) people would expect it to be
asynchronous in the sense that the inferior is resumed and
control returns to gdb.  IOW target_resume doesn't also wait
for the inferior to stop after it has been resumed.
Therefore I see no need to rename it (say to target_resume_no_wait).

> So I'd like to see a comprehensive list of functions
> that'd you're proposing would be affected in order to be able to
> comment further or even be able to see the same patterns you're seeing.

I think these things can be approached on a by-demand basis.
All I'm suggesting(!) [and it is just a suggestion] is having a convention
for when the topic comes up.

> target_stop_and_wait (the existing target_stop_ptid) isn't really a
> target method, unlike target_stop, target_resume, etc. -- it's a
> helper method that sits on top of target methods.

Whether or not a function is a target method is orthogonal to
what its name suggests to the reader about what it does
and how it behaves.

> So I'd go with
> clarifying target_stop's "asyncness" in its comments, name the
> helper target_stop_and_wait.  I think that's already better
> than the status quo.  And then we can discuss conventions and
> (potentially wholesale) renaming separately.  It's not a big deal
> if we have to rename the new function again.

This is a step in the right direction, and if that is all people are
comfortable with right now that's fine by me.
[I've gotten target_stop's asyncness burned into memory,
but I can't help but worry about the next reader.]

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Pedro Alves]

On 09/16/2014 10:18 PM, Doug Evans wrote:
> On Tue, Sep 16, 2014 at 3:36 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 09/15/2014 05:00 PM, Doug Evans wrote:
>>> On Mon, Sep 15, 2014 at 3:07 AM, Gary Benson <gbenson@redhat.com> wrote:
>>>> Doug Evans wrote:
>>>>> On Fri, Sep 12, 2014 at 11:19 AM, Pedro Alves <palves@redhat.com> wrote:
>>>>>> On 09/12/2014 07:08 PM, Doug Evans wrote:
>>>>>>> Pedro Alves wrote:
>>>>>>>> I just now noticed the elephant in the room -- target_stop
>>>>>>>> is asynchronous, doesn't wait for a stop, while and
>>>>>>>> target_stop_ptid is synchronous.  [...]
>>>>>>>
>>>>>>> If the above code is right, I think a clarifying comment is
>>>>>>> required somewhere.  It's odd that one can call
>>>>>>> agent_run_command when the inferior may or may not be stopped
>>>>>>> yet.  [Or is there a bug here? - if I'm reading the gdbserver
>>>>>>> version correctly it first waits for the inferior to stop]
>>>>>>
>>>>>> It's a bug.
>>>>>>
>>>>>> (Note that the GDB side interfaces with an out-of-tree
>>>>>> agent, not GDBserver's agent.  I don't know the status of
>>>>>> that agent.)
>>>>>
>>>>> Data point that target_stop should be named target_stop_async?
>>>>
>>>> Ok, can I get a summary of this thread, I'm struggling to follow it.
>>>>
>>>>  a) What should the functions be called:
>>>>      - target_stop_async / target_stop_wait
>>>>      - target_continue_async / target_continue_no_signal
>>>>      - something else?
>>>>
>>>>  b) Is there a bug here I need to address?
>>>
>>> At this point I think we're still in discussion mode, there are no
>>> conclusions/agreements yet, except for the agreement to use
>>> target_continue_with_no_signal instead of target_continue_ptid.
>>>
>>> To advance the discussion,
>>> the async case is the subtle case IMO.  Evidently (and I'm just going
>>> by what I see, there may be more data here) someone (*1) looked at the
>>> name "target_stop" and thought it was async (which is probably what
>>> I'd do).  The function comment doesn't specify.  One could argue it's
>>> sufficient to just fix the function comment, but if we're going to
>>> have a mix of similar functions, some of which are async and some
>>> sync, then IMO we should also make the difference stand out in the
>>> code where it's read. I'd be happy with a convention where all async
>>> functions ended with _async or _no_wait (the latter reads better to
>>> me), but I'm guessing I'd be happy with a different convention as
>>> well.
>>
>> At first blush, this looks to me like looking for a generalization / hard
>> rule where one might not be necessary, or otherwise, we'd need more a
>> better/clearer rule/constrain on which methods this would apply to.  E.g.,
>> would you rename target_xfer_partial?  Certainly memory accesses can
>> be expected to work in an asynchronous way, with read results being sent
>> over with asynchronous notifications (even though we don't do it like that).
> 
> Are you asking about target_xfer_partial as it exists today,
> or a hypothetical version where it is used to request the transfer
> but the transfer happens asynchronously? [Or both? 1/2 :-)]
> I think it's more the latter, but I'll answer for both.
> 
> When I think of a "read memory" operation, absent any info specifying
> sync/async, I'd expect that the request completes before control is
> returned to the caller.  If it didn't wait for the request to complete that
> would be unexpected.  Therefore I'd leave the name of
> target_xfer_partial as it exists today alone.
> That same reasoning to me says a hypothetical "memory read"
> request that was asynchronous should have something in its
> name that says to the reader "Hey, this is async!  Heads up!".
> 
>> What about target_resume?  I'm not sure renaming that would be useful,
>> for example.
> 
> I thought about target_resume.  It was an semi-interesting case
> that immediately popped into my head at the time.
> And then I tried to think how the typical reader would interpret it.
> I'm not a typical reader, but I think(!) people would expect it to be
> asynchronous in the sense that the inferior is resumed and
> control returns to gdb.  IOW target_resume doesn't also wait
> for the inferior to stop after it has been resumed.
> Therefore I see no need to rename it (say to target_resume_no_wait).
> 
>> So I'd like to see a comprehensive list of functions
>> that'd you're proposing would be affected in order to be able to
>> comment further or even be able to see the same patterns you're seeing.
> 
> I think these things can be approached on a by-demand basis.
> All I'm suggesting(!) [and it is just a suggestion] is having a convention
> for when the topic comes up.

That's the thing.  You've now shown that you've given some thought over
a couple cases, but when you first raised this, you didn't, which puts us
in a position of having to stop and go through the same exercises and
looking for patterns, checking what might or not make sense, etc.
There's no way to tell whether the suggestion makes sense without that
info; IOW, the suggestion was too vague and ends up being
counterproductive (in the sense that we spend a bunch of time on it
instead of moving forward with other more important things).  Please
don't take me wrong.  I'm in "how to communicate better mode".  I would
have liked to have been able to reply quicker to these emails, but I
just don't know how -- fyi, today's and yesterday's emails on this
subject took me a few hours.

I guess my thing is that I'm not particularly fond of appending
_async/_sync/_no_wait to target methods names throughout.  Maybe
we'll find even better names as we move along.

>> target_stop_and_wait (the existing target_stop_ptid) isn't really a
>> target method, unlike target_stop, target_resume, etc. -- it's a
>> helper method that sits on top of target methods.
> 
> Whether or not a function is a target method is orthogonal to
> what its name suggests to the reader about what it does
> and how it behaves.

I was just trying to say that we could call target_stop_ptid
pause_thread or something else without "target_" in the name even,
though I'm not suggesting that.

>> So I'd go with
>> clarifying target_stop's "asyncness" in its comments, name the
>> helper target_stop_and_wait.  I think that's already better
>> than the status quo.  And then we can discuss conventions and
>> (potentially wholesale) renaming separately.  It's not a big deal
>> if we have to rename the new function again.
> 
> This is a step in the right direction, and if that is all people are
> comfortable with right now that's fine by me.

Thanks.  Let's go forward with that, so the common/ work can
continue, and continue discussing the naming in parallel.

> [I've gotten target_stop's asyncness burned into memory,
> but I can't help but worry about the next reader.]

We could even pick IMO more natural terms than _async or _no_wait,
e.g., target_request_stop.

Thanks,
Pedro Alves

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Doug Evans]

On Wed, Sep 17, 2014 at 4:30 AM, Pedro Alves <palves@redhat.com> wrote:
> On 09/16/2014 10:18 PM, Doug Evans wrote:
>> On Tue, Sep 16, 2014 at 3:36 AM, Pedro Alves <palves@redhat.com> wrote:
>>> On 09/15/2014 05:00 PM, Doug Evans wrote:
>>>> On Mon, Sep 15, 2014 at 3:07 AM, Gary Benson <gbenson@redhat.com> wrote:
>>>>> Doug Evans wrote:
>>>>>> On Fri, Sep 12, 2014 at 11:19 AM, Pedro Alves <palves@redhat.com> wrote:
>>>>>>> On 09/12/2014 07:08 PM, Doug Evans wrote:
>>>>>>>> Pedro Alves wrote:
>>>>>>>>> I just now noticed the elephant in the room -- target_stop
>>>>>>>>> is asynchronous, doesn't wait for a stop, while and
>>>>>>>>> target_stop_ptid is synchronous.  [...]
>>>>>>>>
>>>>>>>> If the above code is right, I think a clarifying comment is
>>>>>>>> required somewhere.  It's odd that one can call
>>>>>>>> agent_run_command when the inferior may or may not be stopped
>>>>>>>> yet.  [Or is there a bug here? - if I'm reading the gdbserver
>>>>>>>> version correctly it first waits for the inferior to stop]
>>>>>>>
>>>>>>> It's a bug.
>>>>>>>
>>>>>>> (Note that the GDB side interfaces with an out-of-tree
>>>>>>> agent, not GDBserver's agent.  I don't know the status of
>>>>>>> that agent.)
>>>>>>
>>>>>> Data point that target_stop should be named target_stop_async?
>>>>>
>>>>> Ok, can I get a summary of this thread, I'm struggling to follow it.
>>>>>
>>>>>  a) What should the functions be called:
>>>>>      - target_stop_async / target_stop_wait
>>>>>      - target_continue_async / target_continue_no_signal
>>>>>      - something else?
>>>>>
>>>>>  b) Is there a bug here I need to address?
>>>>
>>>> At this point I think we're still in discussion mode, there are no
>>>> conclusions/agreements yet, except for the agreement to use
>>>> target_continue_with_no_signal instead of target_continue_ptid.
>>>>
>>>> To advance the discussion,
>>>> the async case is the subtle case IMO.  Evidently (and I'm just going
>>>> by what I see, there may be more data here) someone (*1) looked at the
>>>> name "target_stop" and thought it was async (which is probably what
>>>> I'd do).  The function comment doesn't specify.  One could argue it's
>>>> sufficient to just fix the function comment, but if we're going to
>>>> have a mix of similar functions, some of which are async and some
>>>> sync, then IMO we should also make the difference stand out in the
>>>> code where it's read. I'd be happy with a convention where all async
>>>> functions ended with _async or _no_wait (the latter reads better to
>>>> me), but I'm guessing I'd be happy with a different convention as
>>>> well.
>>>
>>> At first blush, this looks to me like looking for a generalization / hard
>>> rule where one might not be necessary, or otherwise, we'd need more a
>>> better/clearer rule/constrain on which methods this would apply to.  E.g.,
>>> would you rename target_xfer_partial?  Certainly memory accesses can
>>> be expected to work in an asynchronous way, with read results being sent
>>> over with asynchronous notifications (even though we don't do it like that).
>>
>> Are you asking about target_xfer_partial as it exists today,
>> or a hypothetical version where it is used to request the transfer
>> but the transfer happens asynchronously? [Or both? 1/2 :-)]
>> I think it's more the latter, but I'll answer for both.
>>
>> When I think of a "read memory" operation, absent any info specifying
>> sync/async, I'd expect that the request completes before control is
>> returned to the caller.  If it didn't wait for the request to complete that
>> would be unexpected.  Therefore I'd leave the name of
>> target_xfer_partial as it exists today alone.
>> That same reasoning to me says a hypothetical "memory read"
>> request that was asynchronous should have something in its
>> name that says to the reader "Hey, this is async!  Heads up!".
>>
>>> What about target_resume?  I'm not sure renaming that would be useful,
>>> for example.
>>
>> I thought about target_resume.  It was an semi-interesting case
>> that immediately popped into my head at the time.
>> And then I tried to think how the typical reader would interpret it.
>> I'm not a typical reader, but I think(!) people would expect it to be
>> asynchronous in the sense that the inferior is resumed and
>> control returns to gdb.  IOW target_resume doesn't also wait
>> for the inferior to stop after it has been resumed.
>> Therefore I see no need to rename it (say to target_resume_no_wait).
>>
>>> So I'd like to see a comprehensive list of functions
>>> that'd you're proposing would be affected in order to be able to
>>> comment further or even be able to see the same patterns you're seeing.
>>
>> I think these things can be approached on a by-demand basis.
>> All I'm suggesting(!) [and it is just a suggestion] is having a convention
>> for when the topic comes up.
>
> That's the thing.  You've now shown that you've given some thought over
> a couple cases,

It's not clear to me what your point is here.
I thought of target_resume earlier, and I hardly spent any time on it.
I wouldn't have expected anyone else to either.

I thought of target_xfer_partial when you mentioned it.

> but when you first raised this, you didn't, which puts us
> in a position of having to stop and go through the same exercises and
> looking for patterns, checking what might or not make sense, etc.

Let's assume for the sake of discussion I *had* also thought of
target_xfer_partial earlier.  Mentioning those two cases would
have made any difference to the time being put into this discussion?
Wow, noted for next time.

> There's no way to tell whether the suggestion makes sense without that
> info; IOW, the suggestion was too vague and ends up being
> counterproductive (in the sense that we spend a bunch of time on it
> instead of moving forward with other more important things).

re: "more important things":
Eh?
The topic at hand is readable code and avoiding bugs,
and yet if I go through gdb-patches@ for even the last week
what will I find?  The standard bits of hacking, bug fixing,
and administrivia (e.g., vxworks cleanup).

Characterizing this as spending time on less important things bothers me.
gdb has several problems I wish people had spent a bit more time on.
[OTOH, I'm not suggesting spending *too* much time on this.
That should go without saying though.]

> Please
> don't take me wrong.  I'm in "how to communicate better mode".  I would
> have liked to have been able to reply quicker to these emails, but I
> just don't know how -- fyi, today's and yesterday's emails on this
> subject took me a few hours.

If the situation were reversed and I didn't have time at that moment,
I'd have just said it's an interesting idea but let's table this discussion
until we can collect more info.

> I guess my thing is that I'm not particularly fond of appending
> _async/_sync/_no_wait to target methods names throughout.  Maybe
> we'll find even better names as we move along.
>
>>> target_stop_and_wait (the existing target_stop_ptid) isn't really a
>>> target method, unlike target_stop, target_resume, etc. -- it's a
>>> helper method that sits on top of target methods.
>>
>> Whether or not a function is a target method is orthogonal to
>> what its name suggests to the reader about what it does
>> and how it behaves.
>
> I was just trying to say that we could call target_stop_ptid
> pause_thread or something else without "target_" in the name even,
> though I'm not suggesting that.

Ah, thanks for clearing that up ...
Still, it's a bit tangential to the discussion at hand.
No worries though.

>>> So I'd go with
>>> clarifying target_stop's "asyncness" in its comments, name the
>>> helper target_stop_and_wait.  I think that's already better
>>> than the status quo.  And then we can discuss conventions and
>>> (potentially wholesale) renaming separately.  It's not a big deal
>>> if we have to rename the new function again.
>>
>> This is a step in the right direction, and if that is all people are
>> comfortable with right now that's fine by me.
>
> Thanks.  Let's go forward with that, so the common/ work can
> continue, and continue discussing the naming in parallel.

FAOD, I was not suggesting blocking the common/ work for this.
I'd expect some parallelization.

>> [I've gotten target_stop's asyncness burned into memory,
>> but I can't help but worry about the next reader.]
>
> We could even pick IMO more natural terms than _async or _no_wait,
> e.g., target_request_stop.

As long as it solves the problem in a reasonable way
and we're consistent I think that'd be ok.

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Pedro Alves]

On 09/17/2014 07:20 PM, Doug Evans wrote:

>>> I thought about target_resume.  It was an semi-interesting case
>>> that immediately popped into my head at the time.
>>> And then I tried to think how the typical reader would interpret it.
>>> I'm not a typical reader, but I think(!) people would expect it to be
>>> asynchronous in the sense that the inferior is resumed and
>>> control returns to gdb.  IOW target_resume doesn't also wait
>>> for the inferior to stop after it has been resumed.
>>> Therefore I see no need to rename it (say to target_resume_no_wait).

OK.  I was reading it like "a convention where all async functions
ended with _async or _no_wait" would be applied throughout.  I could
see instead that restricted to cases where we have two variants -- I
guess that's where my understanding was.

> re: "more important things":
> Eh?

I'm sorry about that.  I clearly overreacted...

> Characterizing this as spending time on less important things bothers me.
> gdb has several problems I wish people had spent a bit more time on.
> [OTOH, I'm not suggesting spending *too* much time on this.
> That should go without saying though.]

> 
>> Please
>> don't take me wrong.  I'm in "how to communicate better mode".  I would
>> have liked to have been able to reply quicker to these emails, but I
>> just don't know how -- fyi, today's and yesterday's emails on this
>> subject took me a few hours.
> 
> If the situation were reversed and I didn't have time at that moment,
> I'd have just said it's an interesting idea but let's table this discussion
> until we can collect more info.

OK.  I'll be sure to ask for clarification earlier too, as I seem to
have misunderstood what you were actually suggesting.

Thanks,
Pedro Alves

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Doug Evans]

On Fri, Sep 19, 2014 at 9:50 AM, Pedro Alves <palves@redhat.com> wrote:
> On 09/17/2014 07:20 PM, Doug Evans wrote:
>
>>>> I thought about target_resume.  It was an semi-interesting case
>>>> that immediately popped into my head at the time.
>>>> And then I tried to think how the typical reader would interpret it.
>>>> I'm not a typical reader, but I think(!) people would expect it to be
>>>> asynchronous in the sense that the inferior is resumed and
>>>> control returns to gdb.  IOW target_resume doesn't also wait
>>>> for the inferior to stop after it has been resumed.
>>>> Therefore I see no need to rename it (say to target_resume_no_wait).
>
> OK.  I was reading it like "a convention where all async functions
> ended with _async or _no_wait" would be applied throughout.  I could
> see instead that restricted to cases where we have two variants -- I
> guess that's where my understanding was.

I did write "... that is otherwise ambiguous".
ref: https://sourceware.org/ml/gdb-patches/2014-09/msg00440.html

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Pedro Alves]

On 09/12/2014 07:29 PM, Doug Evans wrote:

> btw, out-of-tree agent, and not gdbserver's agent?
> That's odd.  Where would I find this agent?

I don't have a link handy; I'd have to do a web search.  Yao was
working on it.  Try looking for the history around the patch
that made common/agent.c work in gdb.

Thanks,
Pedro Alves

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Pedro Alves]

On 08/29/2014 02:51 PM, Gary Benson wrote:


> +/* See target/target.h.  */
> +
> +void
> +target_stop_ptid (ptid_t ptid)
> +{
> +  struct target_waitstatus status;
> +  int was_non_stop = non_stop;
> +
> +  non_stop = 1;
> +  target_stop (ptid);
> +
> +  memset (&status, 0, sizeof (status));
> +  target_wait (ptid, &status, 0);
> +
> +  non_stop = was_non_stop;
> +}

One thing that was bugging me was that given that the names
of target_stop and target_stop_ptid are so similar and that
they have the same signature is ripe for confusion.

I just now noticed the elephant in the room -- target_stop is
asynchronous, doesn't wait for a stop, while and target_stop_ptid
is synchronous.  Would you mind renaming this to target_stop_wait
or some such?  And then add an explicit "and wait for it to stop"
or some such to the function's description.

Thanks,
Pedro Alves

Re: [PATCH 3/9 v7] Introduce target_{stop,continue}_ptid

[Doug Evans]

Pedro Alves writes:
 > On 08/29/2014 02:51 PM, Gary Benson wrote:
 > 
 > > +/* See target/target.h.  */
 > > +
 > > +void
 > > +target_stop_ptid (ptid_t ptid)
 > > +{
 > > +  struct target_waitstatus status;
 > > +  int was_non_stop = non_stop;
 > > +
 > > +  non_stop = 1;
 > > +  target_stop (ptid);
 > > +
 > > +  memset (&status, 0, sizeof (status));
 > > +  target_wait (ptid, &status, 0);
 > > +
 > > +  non_stop = was_non_stop;
 > > +}
 > 
 > One thing that was bugging me was that given that the names
 > of target_stop and target_stop_ptid are so similar and that
 > they have the same signature is ripe for confusion.
 > 
 > I just now noticed the elephant in the room -- target_stop is
 > asynchronous, doesn't wait for a stop, while and target_stop_ptid
 > is synchronous.  Would you mind renaming this to target_stop_wait
 > or some such?  And then add an explicit "and wait for it to stop"
 > or some such to the function's description.

target_stop_and_wait would be a significantly more readable to me.
["stop_wait" makes me pause too long to figure out what was meant]

Or if a plain reading of "target_stop" implies waiting,
then rename target_stop to target_stop_no_wait or target_stop_async.
and use "target_stop" for "target stop and wait".

Do we want a convention for marking async vs sync routines?
e.g. append every async routine that is otherwise ambiguous with _async?
Or if gdb is becoming generally async, turn it around and
append _sync to every sync routine that is otherwise ambiguous.
[Or even allow both in certain instances.]
That way we'd no longer have to have individual discussions
whenever the topic comes up.

[PATCH 4/9 v7] Introduce target/symbol.h

[Gary Benson]

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

This patch differs from the version I posted on August 1
(https://sourceware.org/ml/gdb-patches/2014-08/msg00013.html) in that
the new function has been renamed as find_minimal_symbol_address and
that it lives in minsyms.c for GDB and a new file symbol.c for
gdbserver rather than in target.c for both.

gdb/ChangeLog:

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

gdb/gdbserver/ChangeLog:

	* symbol.c: New file.
	* Makefile.in (SFILES): Add symbol.c.
	(OBS): Add symbol.o.
---
 gdb/ChangeLog             |   11 +++++++++++
 gdb/Makefile.in           |    2 +-
 gdb/common/agent.c        |   15 +++------------
 gdb/gdbserver/ChangeLog   |    7 +++++++
 gdb/gdbserver/Makefile.in |    4 ++--
 gdb/gdbserver/symbol.c    |   32 ++++++++++++++++++++++++++++++++
 gdb/minsyms.c             |   15 +++++++++++++++
 gdb/target.h              |    1 +
 gdb/target/symbol.h       |   37 +++++++++++++++++++++++++++++++++++++
 9 files changed, 109 insertions(+), 15 deletions(-)
 create mode 100644 gdb/gdbserver/symbol.c
 create mode 100644 gdb/target/symbol.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index bb33d38..49b7c74 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -938,7 +938,7 @@ target/wait.h target/waitstatus.h nat/linux-nat.h nat/linux-waitpid.h \
 common/print-utils.h common/rsp-low.h nat/i386-dregs.h x86-linux-nat.h \
 i386-linux-nat.h common/common-defs.h common/errors.h common/common-types.h \
 common/common-debug.h common/cleanups.h common/gdb_setjmp.h \
-common/common-exceptions.h target/target.h
+common/common-exceptions.h target/target.h target/symbol.h
 
 # Header files that already have srcdir in them, or which are in objdir.
 
diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index 0ac73a9..239497f 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -21,9 +21,9 @@
 #include "server.h"
 #else
 #include "defs.h"
-#include "objfiles.h"
 #endif
 #include "target/target.h"
+#include "target/symbol.h"
 #include <unistd.h>
 #include "agent.h"
 #include "filestuff.h"
@@ -98,18 +98,9 @@ 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 (find_minimal_symbol_address (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/Makefile.in b/gdb/gdbserver/Makefile.in
index 21ac6f2..8a313f9 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -171,7 +171,7 @@ SFILES=	$(srcdir)/gdbreplay.c $(srcdir)/inferiors.c $(srcdir)/dll.c \
 	$(srcdir)/nat/mips-linux-watch.c $(srcdir)/common/print-utils.c \
 	$(srcdir)/common/rsp-low.c $(srcdir)/common/errors.c \
 	$(srcdir)/common/common-debug.c $(srcdir)/common/cleanups.c \
-	$(srcdir)/common/common-exceptions.c
+	$(srcdir)/common/common-exceptions.c $(srcdir)/symbol.c
 
 DEPFILES = @GDBSERVER_DEPFILES@
 
@@ -185,7 +185,7 @@ OBS = agent.o ax.o inferiors.o regcache.o remote-utils.o server.o signals.o \
       mem-break.o hostio.o event-loop.o tracepoint.o xml-utils.o \
       common-utils.o ptid.o buffer.o format.o filestuff.o dll.o notif.o \
       tdesc.o print-utils.o rsp-low.o errors.o common-debug.o cleanups.o \
-      common-exceptions.o \
+      common-exceptions.o symbol.o \
       $(XML_BUILTIN) $(DEPFILES) $(LIBOBJS)
 GDBREPLAY_OBS = gdbreplay.o version.o
 GDBSERVER_LIBS = @GDBSERVER_LIBS@
diff --git a/gdb/gdbserver/symbol.c b/gdb/gdbserver/symbol.c
new file mode 100644
index 0000000..6b967d1
--- /dev/null
+++ b/gdb/gdbserver/symbol.c
@@ -0,0 +1,32 @@
+/* Symbol manipulating routines for the remote server for GDB.
+
+   Copyright (C) 2014 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "server.h"
+#include "target/symbol.h"
+
+/* See target/symbol.h.  */
+
+int
+find_minimal_symbol_address (const char *name, CORE_ADDR *addr,
+			     struct objfile *objfile)
+{
+  gdb_assert (objfile == NULL);
+
+  return look_up_one_symbol (name, addr, 1) != 1;
+}
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index fd7fcd9..f4a20f8 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -299,6 +299,21 @@ lookup_bound_minimal_symbol (const char *name)
   return lookup_minimal_symbol (name, NULL, NULL);
 }
 
+/* See target/symbol.h.  */
+
+int
+find_minimal_symbol_address (const char *name, CORE_ADDR *addr,
+			     struct objfile *objfile)
+{
+  struct bound_minimal_symbol sym
+    = lookup_minimal_symbol (name, NULL, objfile);
+
+  if (sym.minsym != NULL)
+    *addr = BMSYMBOL_VALUE_ADDRESS (sym);
+
+  return sym.minsym == NULL;
+}
+
 /* See minsyms.h.  */
 
 void
diff --git a/gdb/target.h b/gdb/target.h
index 2ea7a2d..b07ab3e 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..8191b89
--- /dev/null
+++ b/gdb/target/symbol.h
@@ -0,0 +1,37 @@
+/* Declarations of target symbol functions.
+
+   Copyright (C) 1986-2014 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef TARGET_SYMBOL_H
+#define TARGET_SYMBOL_H
+
+struct objfile;
+
+/* Find a symbol that matches NAME.  Limit the search to OBJFILE if
+   OBJFILE is non-NULL and the implementation supports limiting the
+   search to specific object files.  NAME may be mangled or demangled.
+   If a match is found, store the matching symbol's address in ADDR
+   and return zero.  Returns nonzero 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.  This function must be provided by the client.  */
+
+extern int find_minimal_symbol_address (const char *name, CORE_ADDR *addr,
+				  struct objfile *objfile);
+
+#endif /* TARGET_SYMBOL_H */
-- 
1.7.1

Re: [PATCH 4/9 v7] Introduce target/symbol.h

[Pedro Alves]

On 08/29/2014 02:51 PM, Gary Benson wrote:
> This introduces target/symbol.h.  This file declares a function that
> the shared code can use and that the clients must implement.  It also
> changes some shared code to use these functions.
> 
> This patch differs from the version I posted on August 1
> (https://sourceware.org/ml/gdb-patches/2014-08/msg00013.html) in that
> the new function has been renamed as find_minimal_symbol_address and
> that it lives in minsyms.c for GDB and a new file symbol.c for
> gdbserver rather than in target.c for both.

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

Thanks.  Though for the same reason in the old version calling
this new method target_foo looked wrong, because this is not interacting
with the target vector abstraction (it looks like something
for a client vector instead), I don't think this belongs in
target/ at all.  I think it should go in common/ instead
for now.

Related, this:

> 	* target.h: Include target/symbol.h.

looks wrong to me too, and it isn't clear to me why you needed it.

Thanks,
Pedro Alves

Re: [PATCH 4/9 v7] Introduce target/symbol.h

[Gary Benson]

Pedro Alves wrote:
> On 08/29/2014 02:51 PM, Gary Benson wrote:
> > This introduces target/symbol.h.  This file declares a function
> > that the shared code can use and that the clients must implement.
> > It also changes some shared code to use these functions.
> > 
> > This patch differs from the version I posted on August 1
> > (https://sourceware.org/ml/gdb-patches/2014-08/msg00013.html)
> > in that the new function has been renamed as
> > find_minimal_symbol_address and that it lives in minsyms.c for GDB
> > and a new file symbol.c for gdbserver rather than in target.c for
> > both.
> > 
> > gdb/ChangeLog:
> > 
> > 	* target/symbol.h: New file.
> > 	* Makefile.in (HFILES_NO_SRCDIR): Add target/symbol.h.
> > 	* target.h: Include target/symbol.h.
> > 	* minsyms.c (find_minimal_symbol_address): New function.
> > 	* common/agent.c: Include target/symbol.h.
> > 	[!GDBSERVER]: Don't include objfiles.h.
> > 	(agent_look_up_symbols): Use find_minimal_symbol_address.
> 
> Thanks.  Though for the same reason in the old version calling this
> new method target_foo looked wrong, because this is not interacting
> with the target vector abstraction (it looks like something for a
> client vector instead), I don't think this belongs in target/ at
> all.  I think it should go in common/ instead for now.

Ok, I'll put it in common.

> Related, this:
> 
> > 	* target.h: Include target/symbol.h.
> 
> looks wrong to me too, and it isn't clear to me why you needed it.

Probably a hangover from some old version or another.  Removed.

Going to push the updated commit inlined below.

Cheers,
Gary

-- 
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3dadc78..bcc6642 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2014-09-11  Tom Tromey  <tromey@redhat.com>
+	    Gary Benson  <gbenson@redhat.com>
+
+	* common/symbol.h: New file.
+	* Makefile.in (HFILES_NO_SRCDIR): Add common/symbol.h.
+	* minsyms.c (find_minimal_symbol_address): New function.
+	* common/agent.c: Include common/symbol.h.
+	[!GDBSERVER]: Don't include objfiles.h.
+	(agent_look_up_symbols): Use find_minimal_symbol_address.
+
 2014-09-11  Gary Benson  <gbenson@redhat.com>
 
 	* target/target.h (target_stop_ptid, target_continue_ptid):
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 55b9829..f6b9176 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -938,7 +938,7 @@ target/wait.h target/waitstatus.h nat/linux-nat.h nat/linux-waitpid.h \
 common/print-utils.h common/rsp-low.h nat/x86-dregs.h x86-linux-nat.h \
 i386-linux-nat.h common/common-defs.h common/errors.h common/common-types.h \
 common/common-debug.h common/cleanups.h common/gdb_setjmp.h \
-common/common-exceptions.h target/target.h
+common/common-exceptions.h target/target.h common/symbol.h
 
 # Header files that already have srcdir in them, or which are in objdir.
 
diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index 0ac73a9..433ae2a 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -21,9 +21,9 @@
 #include "server.h"
 #else
 #include "defs.h"
-#include "objfiles.h"
 #endif
 #include "target/target.h"
+#include "common/symbol.h"
 #include <unistd.h>
 #include "agent.h"
 #include "filestuff.h"
@@ -98,18 +98,9 @@ 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 (find_minimal_symbol_address (symbol_list[i].name, addrp,
+				       arg) != 0)
 	{
 	  DEBUG_AGENT ("symbol `%s' not found\n", symbol_list[i].name);
 	  return -1;
diff --git a/gdb/common/symbol.h b/gdb/common/symbol.h
new file mode 100644
index 0000000..723d7fa
--- /dev/null
+++ b/gdb/common/symbol.h
@@ -0,0 +1,37 @@
+/* Declarations of common symbol 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_SYMBOL_H
+#define COMMON_SYMBOL_H
+
+struct objfile;
+
+/* Find a symbol that matches NAME.  Limit the search to OBJFILE if
+   OBJFILE is non-NULL and the implementation supports limiting the
+   search to specific object files.  NAME may be mangled or demangled.
+   If a match is found, store the matching symbol's address in ADDR
+   and return zero.  Returns nonzero 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.  This function must be provided by the client.  */
+
+extern int find_minimal_symbol_address (const char *name, CORE_ADDR *addr,
+					struct objfile *objfile);
+
+#endif /* COMMON_SYMBOL_H */
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 8a5ee1f..0ea5a42 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,10 @@
+2014-09-11  Tom Tromey  <tromey@redhat.com>
+	    Gary Benson  <gbenson@redhat.com>
+
+	* symbol.c: New file.
+	* Makefile.in (SFILES): Add symbol.c.
+	(OBS): Add symbol.o.
+
 2014-09-11  Gary Benson  <gbenson@redhat.com>
 
 	* target.c (target_stop_ptid, target_continue_ptid): New
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 1447e61..074d93d 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -171,7 +171,7 @@ SFILES=	$(srcdir)/gdbreplay.c $(srcdir)/inferiors.c $(srcdir)/dll.c \
 	$(srcdir)/nat/mips-linux-watch.c $(srcdir)/common/print-utils.c \
 	$(srcdir)/common/rsp-low.c $(srcdir)/common/errors.c \
 	$(srcdir)/common/common-debug.c $(srcdir)/common/cleanups.c \
-	$(srcdir)/common/common-exceptions.c
+	$(srcdir)/common/common-exceptions.c $(srcdir)/symbol.c
 
 DEPFILES = @GDBSERVER_DEPFILES@
 
@@ -185,7 +185,7 @@ OBS = agent.o ax.o inferiors.o regcache.o remote-utils.o server.o signals.o \
       mem-break.o hostio.o event-loop.o tracepoint.o xml-utils.o \
       common-utils.o ptid.o buffer.o format.o filestuff.o dll.o notif.o \
       tdesc.o print-utils.o rsp-low.o errors.o common-debug.o cleanups.o \
-      common-exceptions.o \
+      common-exceptions.o symbol.o \
       $(XML_BUILTIN) $(DEPFILES) $(LIBOBJS)
 GDBREPLAY_OBS = gdbreplay.o version.o
 GDBSERVER_LIBS = @GDBSERVER_LIBS@
diff --git a/gdb/gdbserver/symbol.c b/gdb/gdbserver/symbol.c
new file mode 100644
index 0000000..d1e6eef
--- /dev/null
+++ b/gdb/gdbserver/symbol.c
@@ -0,0 +1,32 @@
+/* Symbol manipulating routines for the remote server for GDB.
+
+   Copyright (C) 2014 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "server.h"
+#include "symbol.h"
+
+/* See common/symbol.h.  */
+
+int
+find_minimal_symbol_address (const char *name, CORE_ADDR *addr,
+			     struct objfile *objfile)
+{
+  gdb_assert (objfile == NULL);
+
+  return look_up_one_symbol (name, addr, 1) != 1;
+}
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index fd7fcd9..8eb7c85 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -50,6 +50,7 @@
 #include "cp-support.h"
 #include "language.h"
 #include "cli/cli-utils.h"
+#include "symbol.h"
 
 /* Accumulate the minimal symbols for each objfile in bunches of BUNCH_SIZE.
    At the end, copy them all into one newly allocated location on an objfile's
@@ -299,6 +300,21 @@ lookup_bound_minimal_symbol (const char *name)
   return lookup_minimal_symbol (name, NULL, NULL);
 }
 
+/* See common/symbol.h.  */
+
+int
+find_minimal_symbol_address (const char *name, CORE_ADDR *addr,
+			     struct objfile *objfile)
+{
+  struct bound_minimal_symbol sym
+    = lookup_minimal_symbol (name, NULL, objfile);
+
+  if (sym.minsym != NULL)
+    *addr = BMSYMBOL_VALUE_ADDRESS (sym);
+
+  return sym.minsym == NULL;
+}
+
 /* See minsyms.h.  */
 
 void

[PATCH 8/9 v7] Remove GDBSERVER uses from i386-dregs.c

[Gary Benson]

This commit makes nat/i386-dregs.c include common-defs.h rather than
defs.h or server.h.  An extra header required including in order to
support this change.

This patch differs from the version I posted on August 1
(https://sourceware.org/ml/gdb-patches/2014-08/msg00011.html)
in that a hack made unnecessary by patch 1 of this series has
been removed.

gdb/ChangeLog:

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

diff --git a/gdb/nat/i386-dregs.c b/gdb/nat/i386-dregs.c
index 7afe09b..2f22299 100644
--- a/gdb/nat/i386-dregs.c
+++ b/gdb/nat/i386-dregs.c
@@ -17,13 +17,9 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#include "inferior.h"
-#endif
+#include "common-defs.h"
 #include "i386-dregs.h"
+#include "break-common.h"
 
 /* Support for hardware watchpoints and breakpoints using the i386
    debug registers.
-- 
1.7.1

Re: [PATCH 8/9 v7] Remove GDBSERVER uses from i386-dregs.c

[Pedro Alves]

On 08/29/2014 02:51 PM, Gary Benson wrote:
> This commit makes nat/i386-dregs.c include common-defs.h rather than
> defs.h or server.h.  An extra header required including in order to
> support this change.
> 
> This patch differs from the version I posted on August 1
> (https://sourceware.org/ml/gdb-patches/2014-08/msg00011.html)
> in that a hack made unnecessary by patch 1 of this series has
> been removed.
> 
> gdb/ChangeLog:
> 
> 	* nat/i386-dregs.c: Include common-defs.h and break-common.h.
> 	Don't include defs.h or server.h.

OK.

Thanks,
Pedro Alves

[PATCH 7/9 v7] Remove GDBSERVER uses from linux-btrace.c

[Gary Benson]

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

This patch is unchanged from the version I posted August 1:
https://sourceware.org/ml/gdb-patches/2014-08/msg00009.html

gdb/ChangeLog:

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

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

Re: [PATCH 7/9 v7] Remove GDBSERVER uses from linux-btrace.c

[Pedro Alves]

On 08/29/2014 02:51 PM, Gary Benson wrote:
> This commit makes nat/linux-btrace.c include common-defs.h rather
> than defs.h or server.h.  A couple of minor changes were required
> to support this change.
> 
> This patch is unchanged from the version I posted August 1:
> https://sourceware.org/ml/gdb-patches/2014-08/msg00009.html
> 
> gdb/ChangeLog:
> 
> 	* nat/linux-btrace.c: Include common-defs.h.
> 	Don't include defs.h, server.h or gdbthread.h.
> 	* nat/linux-btrace.h (struct target_ops): New forward declaration.

Looks good to me too.

Thanks,
Pedro Alves

[PATCH 9/9 v7] Remove one GDBSERVER use from linux-waitpid.c

[Gary Benson]

This commit makes nat/linux-waitpid.c include common-defs.h rather
than defs.h or server.h.  Some debug code previously only built for
gdbserver is now also built for GDB, and a use of GDBSERVER has had
to remain in order to select the variable used to determine whether
debug printing should be enabled.

This patch differs from the version I posted on August 1
(https://sourceware.org/ml/gdb-patches/2014-08/msg00016.html)
in that previously debug printing was only built for gdbserver
(enabled when debug_threads is set) but now it is also built
for GDB (enabled when debug_linux_nat is set).  I had to make
debug_linux_nat nonstatic to do this.

gdb/ChangeLog:

	* linux-nat.c (debug_linux_nat): Make nonstatic.
	* nat/linux-waitpid.c: Include common-defs.h.
	Don't include defs.h, server.h or signal.h.
	(debug_linux_waitpid): New macro and extern.
	(linux_debug): Use debug_linux_waitpid and debug_vprintf.
---
 gdb/ChangeLog           |    8 ++++++++
 gdb/linux-nat.c         |    3 ++-
 gdb/nat/linux-waitpid.c |   25 +++++++++++++------------
 3 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 0898442..2e0aedc 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -204,7 +204,8 @@ static target_xfer_partial_ftype *super_xfer_partial;
    Called by our to_close.  */
 static void (*super_close) (struct target_ops *);
 
-static unsigned int debug_linux_nat;
+unsigned int debug_linux_nat;
+
 static void
 show_debug_linux_nat (struct ui_file *file, int from_tty,
 		      struct cmd_list_element *c, const char *value)
diff --git a/gdb/nat/linux-waitpid.c b/gdb/nat/linux-waitpid.c
index 53847ac..7fea870 100644
--- a/gdb/nat/linux-waitpid.c
+++ b/gdb/nat/linux-waitpid.c
@@ -17,16 +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 "common-defs.h"
+#include "linux-nat.h"
+#include "linux-waitpid.h"
+#include "gdb_wait.h"
+
+/* FIXME: Need a common variable to select Linux debug output.  */
+
 #ifdef GDBSERVER
-#include "server.h"
+#define debug_linux_waitpid debug_threads
 #else
-#include "defs.h"
-#include "signal.h"
+#define debug_linux_waitpid debug_linux_nat
 #endif
 
-#include "linux-nat.h"
-#include "linux-waitpid.h"
-#include "gdb_wait.h"
+extern int debug_linux_waitpid;
 
 /* Print debugging output based on the format string FORMAT and
    its parameters.  */
@@ -34,17 +38,14 @@
 static inline void
 linux_debug (const char *format, ...)
 {
-#ifdef GDBSERVER
-  if (debug_threads)
+  if (debug_linux_waitpid)
     {
       va_list args;
+
       va_start (args, format);
-      vfprintf (stderr, format, args);
+      debug_vprintf (format, args);
       va_end (args);
     }
-#else
-  /* GDB-specific debugging output.  */
-#endif
 }
 
 /* Convert wait status STATUS to a string.  Used for printing debug
-- 
1.7.1

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

[Pedro Alves]

On 08/29/2014 02:51 PM, Gary Benson wrote:

>  
> -static unsigned int debug_linux_nat;
> +unsigned int debug_linux_nat;
> +

> +#define debug_linux_waitpid debug_linux_nat
>  #endif

> +extern int debug_linux_waitpid;
>  

The dangers of extern...  This ends up undefined code, as
the variable is defined as signed, and then declared here
as signed.

Sorry, but now that I see it, IMO the end up resulting code
doesn't really look better than what we already have.  :-/  We
end up with #ifdef GDBSERVER anyway, so might be best to
either leave this be until we come up with a complete solution.

Thanks,
Pedro Alves

[PATCH v8] Clarify GDBSERVER use in linux-waitpid.c

[Gary Benson]

Pedro Alves wrote:
> Sorry, but now that I see it, IMO the end up resulting code
> doesn't really look better than what we already have.  :-/
> We end up with #ifdef GDBSERVER anyway, so might be best to
> either leave this be until we come up with a complete solution.

Ok, how about this:

This commit makes linux-waitpid.c include common-defs.h.  GDB's
inclusion of defs.h is removed, but gdbserver's inclusion of
server.h remains to support some gdbserver-specific debug code
that cannot presently be merged.  A new FIXME documents this.

gdb/ChangeLog:

	* nat/linux-waitpid.c: Include common-defs.h.
	[GDBSERVER]: Add FIXME comment.
	[!GDBSERVER]: Don't include defs.h or signal.h.
	(linux_debug) [!GDBSERVER]: Remove empty block.
---
 gdb/ChangeLog           |    7 +++++++
 gdb/nat/linux-waitpid.c |   11 ++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/gdb/nat/linux-waitpid.c b/gdb/nat/linux-waitpid.c
index 53847ac..04cdc3d 100644
--- a/gdb/nat/linux-waitpid.c
+++ b/gdb/nat/linux-waitpid.c
@@ -17,11 +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/>.  */
 
+#include "common-defs.h"
+
 #ifdef GDBSERVER
+/* FIXME: server.h is required for the definition of debug_threads
+   which is used in the gdbserver-specific debug printing in
+   linux_debug.  This code should be made available to GDB also,
+   but the lack of a suitable flag to enable it prevents this.  */
 #include "server.h"
-#else
-#include "defs.h"
-#include "signal.h"
 #endif
 
 #include "linux-nat.h"
@@ -42,8 +45,6 @@ linux_debug (const char *format, ...)
       vfprintf (stderr, format, args);
       va_end (args);
     }
-#else
-  /* GDB-specific debugging output.  */
 #endif
 }
 
-- 
1.7.1

Re: [PATCH v8] Clarify GDBSERVER use in linux-waitpid.c

[Pedro Alves]

On 09/12/2014 11:03 AM, Gary Benson wrote:
> Pedro Alves wrote:
>> Sorry, but now that I see it, IMO the end up resulting code
>> doesn't really look better than what we already have.  :-/
>> We end up with #ifdef GDBSERVER anyway, so might be best to
>> either leave this be until we come up with a complete solution.
> 
> Ok, how about this:
> 
> This commit makes linux-waitpid.c include common-defs.h.  GDB's
> inclusion of defs.h is removed, but gdbserver's inclusion of
> server.h remains to support some gdbserver-specific debug code
> that cannot presently be merged.  A new FIXME documents this.
> 
> gdb/ChangeLog:
> 
> 	* nat/linux-waitpid.c: Include common-defs.h.
> 	[GDBSERVER]: Add FIXME comment.
> 	[!GDBSERVER]: Don't include defs.h or signal.h.
> 	(linux_debug) [!GDBSERVER]: Remove empty block.

OK.

Thanks,
Pedro Alves

Re: [PATCH v8] Clarify GDBSERVER use in linux-waitpid.c

[Gary Benson]

Pedro Alves wrote:
> On 09/12/2014 11:03 AM, Gary Benson wrote:
> > Pedro Alves wrote:
> > > Sorry, but now that I see it, IMO the end up resulting code
> > > doesn't really look better than what we already have.  :-/
> > > We end up with #ifdef GDBSERVER anyway, so might be best to
> > > either leave this be until we come up with a complete solution.
> > 
> > Ok, how about this:
> > 
> > This commit makes linux-waitpid.c include common-defs.h.  GDB's
> > inclusion of defs.h is removed, but gdbserver's inclusion of
> > server.h remains to support some gdbserver-specific debug code
> > that cannot presently be merged.  A new FIXME documents this.
> > 
> > gdb/ChangeLog:
> > 
> > 	* nat/linux-waitpid.c: Include common-defs.h.
> > 	[GDBSERVER]: Add FIXME comment.
> > 	[!GDBSERVER]: Don't include defs.h or signal.h.
> > 	(linux_debug) [!GDBSERVER]: Remove empty block.
> 
> OK.

Ok, this whole series is pushed now and (barring the comment Doug
wanted changed) complete.  Thanks for the help guys!

Cheers,
Gary

-- 
http://gbenson.net/

[PATCH 5/9 v7] Introduce common-regcache.h

[Gary Benson]

This introduces common-regcache.h.  This contains two functions that
allow nat/linux-btrace.c to be simplified.  A better long term
solution would be unify the regcache code, but this is sufficient for
now.

This patch differs from the version I posted on August 1
(https://sourceware.org/ml/gdb-patches/2014-08/msg00010.html) in that
some suggested documentation changes have been made and that various
updates were required to reflect regcache changes recently committed
by Andreas Arnez.

gdb/ChangeLog:

	* common/common-regcache.h: New file.
	* Makefile.in (HFILES_NO_SRCDIR): Add common/common-regcache.h.
	* regcache.h: Include common-regcache.h.
	(regcache_read_pc): Don't declare.
	* regcache.c (get_thread_regcache_for_ptid): New function.
	* nat/linux-btrace.c: Don't include regcache.h.
	Include common-regcache.h.
	(perf_event_read_bts): Use get_thread_regcache_for_ptid.

gdb/gdbserver/ChangeLog:

	* regcache.h: Include common-regcache.h.
	(regcache_read_pc): Don't declare.
	* regcache.c (get_thread_regcache_for_ptid): New function.
---
 gdb/ChangeLog                |   12 ++++++++++++
 gdb/Makefile.in              |    3 ++-
 gdb/common/common-regcache.h |   35 +++++++++++++++++++++++++++++++++++
 gdb/gdbserver/ChangeLog      |    7 +++++++
 gdb/gdbserver/regcache.c     |    8 ++++++++
 gdb/gdbserver/regcache.h     |    4 ++--
 gdb/nat/linux-btrace.c       |    8 ++------
 gdb/regcache.c               |    7 +++++++
 gdb/regcache.h               |    3 ++-
 9 files changed, 77 insertions(+), 10 deletions(-)
 create mode 100644 gdb/common/common-regcache.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 49b7c74..b9e766f 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -938,7 +938,8 @@ target/wait.h target/waitstatus.h nat/linux-nat.h nat/linux-waitpid.h \
 common/print-utils.h common/rsp-low.h nat/i386-dregs.h x86-linux-nat.h \
 i386-linux-nat.h common/common-defs.h common/errors.h common/common-types.h \
 common/common-debug.h common/cleanups.h common/gdb_setjmp.h \
-common/common-exceptions.h target/target.h target/symbol.h
+common/common-exceptions.h target/target.h target/symbol.h \
+common/common-regcache.h
 
 # Header files that already have srcdir in them, or which are in objdir.
 
diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
new file mode 100644
index 0000000..aef6bfd
--- /dev/null
+++ b/gdb/common/common-regcache.h
@@ -0,0 +1,35 @@
+/* Cache and manage the values of registers
+
+   Copyright (C) 1986-2014 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef COMMON_REGCACHE_H
+#define COMMON_REGCACHE_H
+
+/* This header is a stopgap until we have an independent regcache.  */
+
+/* Return the register cache associated with the thread specified
+   by PTID.  This function must be provided by the client.  */
+
+extern struct regcache *get_thread_regcache_for_ptid (ptid_t ptid);
+
+/* Read the PC register.  This function must be provided by the
+   client.  */
+
+extern CORE_ADDR regcache_read_pc (struct regcache *regcache);
+
+#endif /* COMMON_REGCACHE_H */
diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index fda2069..ad66ff7 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -60,6 +60,14 @@ get_thread_regcache (struct thread_info *thread, int fetch)
   return regcache;
 }
 
+/* See common/common-regcache.h.  */
+
+struct regcache *
+get_thread_regcache_for_ptid (ptid_t ptid)
+{
+  return get_thread_regcache (find_thread_ptid (ptid), 1);
+}
+
 void
 regcache_invalidate_thread (struct thread_info *thread)
 {
diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h
index 891fead..0c933f3 100644
--- a/gdb/gdbserver/regcache.h
+++ b/gdb/gdbserver/regcache.h
@@ -19,6 +19,8 @@
 #ifndef REGCACHE_H
 #define REGCACHE_H
 
+#include "common-regcache.h"
+
 struct thread_info;
 struct target_desc;
 
@@ -91,8 +93,6 @@ void registers_to_string (struct regcache *regcache, char *buf);
 
 void registers_from_string (struct regcache *regcache, char *buf);
 
-CORE_ADDR regcache_read_pc (struct regcache *regcache);
-
 void regcache_write_pc (struct regcache *regcache, CORE_ADDR pc);
 
 /* Return a pointer to the description of register ``n''.  */
diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index f6fdbda..cf98582 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -26,7 +26,7 @@
 #endif
 
 #include "linux-btrace.h"
-#include "regcache.h"
+#include "common-regcache.h"
 #include "gdbthread.h"
 #include "gdb_wait.h"
 #include "i386-cpuid.h"
@@ -180,11 +180,7 @@ perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
   gdb_assert (start <= end);
 
   /* The first block ends at the current pc.  */
-#ifdef GDBSERVER
-  regcache = get_thread_regcache (find_thread_ptid (tinfo->ptid), 1);
-#else
-  regcache = get_thread_regcache (tinfo->ptid);
-#endif
+  regcache = get_thread_regcache_for_ptid (tinfo->ptid);
   block.end = regcache_read_pc (regcache);
 
   /* The buffer may contain a partial record as its last entry (i.e. when the
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 05b8fb9..9b6c794 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -536,6 +536,13 @@ get_current_regcache (void)
   return get_thread_regcache (inferior_ptid);
 }
 
+/* See common/common-regcache.h.  */
+
+struct regcache *
+get_thread_regcache_for_ptid (ptid_t ptid)
+{
+  return get_thread_regcache (ptid);
+}
 
 /* Observer for the target_changed event.  */
 
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 0361f22..37cffe3 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -20,6 +20,8 @@
 #ifndef REGCACHE_H
 #define REGCACHE_H
 
+#include "common-regcache.h"
+
 struct regcache;
 struct gdbarch;
 struct address_space;
@@ -135,7 +137,6 @@ void regcache_cooked_write_part (struct regcache *regcache, int regnum,
 
 /* Special routines to read/write the PC.  */
 
-extern CORE_ADDR regcache_read_pc (struct regcache *regcache);
 extern void regcache_write_pc (struct regcache *regcache, CORE_ADDR pc);
 
 /* Transfer a raw register [0..NUM_REGS) between the regcache and the
-- 
1.7.1

Re: [PATCH 5/9 v7] Introduce common-regcache.h

[Pedro Alves]

On 08/29/2014 02:51 PM, Gary Benson wrote:
> This introduces common-regcache.h.  This contains two functions that
> allow nat/linux-btrace.c to be simplified.  A better long term
> solution would be unify the regcache code, but this is sufficient for
> now.
> 
> This patch differs from the version I posted on August 1
> (https://sourceware.org/ml/gdb-patches/2014-08/msg00010.html) in that
> some suggested documentation changes have been made and that various
> updates were required to reflect regcache changes recently committed
> by Andreas Arnez.
> 
> gdb/ChangeLog:
> 
> 	* common/common-regcache.h: New file.
> 	* Makefile.in (HFILES_NO_SRCDIR): Add common/common-regcache.h.
> 	* regcache.h: Include common-regcache.h.
> 	(regcache_read_pc): Don't declare.
> 	* regcache.c (get_thread_regcache_for_ptid): New function.
> 	* nat/linux-btrace.c: Don't include regcache.h.
> 	Include common-regcache.h.
> 	(perf_event_read_bts): Use get_thread_regcache_for_ptid.
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* regcache.h: Include common-regcache.h.
> 	(regcache_read_pc): Don't declare.
> 	* regcache.c (get_thread_regcache_for_ptid): New function.
> diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h
> index 891fead..0c933f3 100644
> --- a/gdb/gdbserver/regcache.h
> +++ b/gdb/gdbserver/regcache.h
> @@ -19,6 +19,8 @@
>  #ifndef REGCACHE_H
>  #define REGCACHE_H
>  
> +#include "common-regcache.h"
> +
>  struct thread_info;
>  struct target_desc;
>  
> @@ -91,8 +93,6 @@ void registers_to_string (struct regcache *regcache, char *buf);
>  
>  void registers_from_string (struct regcache *regcache, char *buf);
>  
> -CORE_ADDR regcache_read_pc (struct regcache *regcache);
> -

Like in the target patches, please leave breadcrumbs pointing
to the shared header (here and elsewhere).

Otherwise looks good to me too.

Thanks,
Pedro Alves

Re: [PATCH 5/9 v7] Introduce common-regcache.h

[Doug Evans]

Gary Benson writes:
 > This introduces common-regcache.h.  This contains two functions that
 > allow nat/linux-btrace.c to be simplified.  A better long term
 > solution would be unify the regcache code, but this is sufficient for
 > now.
 > 
 > This patch differs from the version I posted on August 1
 > (https://sourceware.org/ml/gdb-patches/2014-08/msg00010.html) in that
 > some suggested documentation changes have been made and that various
 > updates were required to reflect regcache changes recently committed
 > by Andreas Arnez.
 > 
 > gdb/ChangeLog:
 > 
 > 	* common/common-regcache.h: New file.
 > 	* Makefile.in (HFILES_NO_SRCDIR): Add common/common-regcache.h.
 > 	* regcache.h: Include common-regcache.h.
 > 	(regcache_read_pc): Don't declare.
 > 	* regcache.c (get_thread_regcache_for_ptid): New function.
 > 	* nat/linux-btrace.c: Don't include regcache.h.
 > 	Include common-regcache.h.
 > 	(perf_event_read_bts): Use get_thread_regcache_for_ptid.
 > 
 > gdb/gdbserver/ChangeLog:
 > 
 > 	* regcache.h: Include common-regcache.h.
 > 	(regcache_read_pc): Don't declare.
 > 	* regcache.c (get_thread_regcache_for_ptid): New function.
 >[...]
 > diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
 > new file mode 100644
 > index 0000000..aef6bfd
 > --- /dev/null
 > +++ b/gdb/common/common-regcache.h
 > @@ -0,0 +1,35 @@
 > +/* Cache and manage the values of registers
 > +
 > +   Copyright (C) 1986-2014 Free Software Foundation, Inc.
 > +
 > +   This file is part of GDB.
 > +
 > +   This program is free software; you can redistribute it and/or modify
 > +   it under the terms of the GNU General Public License as published by
 > +   the Free Software Foundation; either version 3 of the License, or
 > +   (at your option) any later version.
 > +
 > +   This program is distributed in the hope that it will be useful,
 > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
 > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 > +   GNU General Public License for more details.
 > +
 > +   You should have received a copy of the GNU General Public License
 > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 > +
 > +#ifndef COMMON_REGCACHE_H
 > +#define COMMON_REGCACHE_H
 > +
 > +/* This header is a stopgap until we have an independent regcache.  */
 > +
 > +/* Return the register cache associated with the thread specified
 > +   by PTID.  This function must be provided by the client.  */

Hi.
Can you add a comment here explaining the ownership of the memory
object returned?
E.g., is it cached "internally" to the function so that the caller
doesn't have to free it?

Thanks!

 > +
 > +extern struct regcache *get_thread_regcache_for_ptid (ptid_t ptid);
 > +
 > +/* Read the PC register.  This function must be provided by the
 > +   client.  */
 > +
 > +extern CORE_ADDR regcache_read_pc (struct regcache *regcache);
 > +
 > +#endif /* COMMON_REGCACHE_H */

Re: [PATCH 5/9 v7] Introduce common-regcache.h

[Gary Benson]

Doug Evans wrote:
> Gary Benson writes:
> > +/* Return the register cache associated with the thread specified
> > +   by PTID.  This function must be provided by the client.  */
> 
> Can you add a comment here explaining the ownership of the memory
> object returned?  E.g., is it cached "internally" to the function
> so that the caller doesn't have to free it?

That seems odd.  We don't document other similar functions this way:
I'm thinking GDB's get_current_arch, current_inferior, target_gdbarch,
or gdbserver's current_process, current_target_desc.  It seems the
pattern is to note if the caller must free the object and to remain
quiet otherwise.

How about I change the comment to "Return _a_pointer_to_ the register
cache..."?  (underlines for emphasis here).

Cheers,
Gary

-- 
http://gbenson.net/

Re: [PATCH 5/9 v7] Introduce common-regcache.h

[Doug Evans]

On Thu, Sep 11, 2014 at 4:02 AM, Gary Benson <gbenson@redhat.com> wrote:
> Doug Evans wrote:
>> Gary Benson writes:
>> > +/* Return the register cache associated with the thread specified
>> > +   by PTID.  This function must be provided by the client.  */
>>
>> Can you add a comment here explaining the ownership of the memory
>> object returned?  E.g., is it cached "internally" to the function
>> so that the caller doesn't have to free it?
>
> That seems odd.  We don't document other similar functions this way:
> I'm thinking GDB's get_current_arch, current_inferior, target_gdbarch,
> or gdbserver's current_process, current_target_desc.  It seems the
> pattern is to note if the caller must free the object and to remain
> quiet otherwise.

Yeah, but I never liked such things being implicit.
I can't trust a missing "caller must free" as being intentional.
[One can equally argue the presence of "caller must free" (or "not
free") isn't necessarily trustable, but such things don't change that
often.]

With a name like "get_current_foo", the "current" makes things less
implicit (at least to me).

If we were using c++ then object ownership can (often, though not
always) be more clearly expressed and then such things can be more
reasonably left as being implicit in comments.
But we don't have that so if we're going to be cleaning things up, and
maybe even paying a little attention to API design, I figure IWBN to
have things be clearer than they are today.

[Plus I get bitten time and again by taking gdb's existing practice as
something we actually want to keep. :-)]

> How about I change the comment to "Return _a_pointer_to_ the register
> cache..."?  (underlines for emphasis here).

If one was going to add emphasis, I'd emphasize _the_. :-)

Re: [PATCH 5/9 v7] Introduce common-regcache.h

[Gary Benson]

Doug Evans wrote:
> On Thu, Sep 11, 2014 at 4:02 AM, Gary Benson <gbenson@redhat.com> wrote:
> > Doug Evans wrote:
> > > Gary Benson writes:
> > > > +/* Return the register cache associated with the thread specified
> > > > +   by PTID.  This function must be provided by the client.  */
> > >
> > > Can you add a comment here explaining the ownership of the
> > > memory object returned?  E.g., is it cached "internally" to
> > > the function so that the caller doesn't have to free it?
> >
> > That seems odd.  We don't document other similar functions this
> > way: I'm thinking GDB's get_current_arch, current_inferior,
> > target_gdbarch, or gdbserver's current_process,
> > current_target_desc.  It seems the pattern is to note if the
> > caller must free the object and to remain quiet otherwise.
> 
> Yeah, but I never liked such things being implicit.  I can't trust a
> missing "caller must free" as being intentional.  [One can equally
> argue the presence of "caller must free" (or "not free") isn't
> necessarily trustable, but such things don't change that often.]
> 
> With a name like "get_current_foo", the "current" makes things less
> implicit (at least to me).
> 
> If we were using c++ then object ownership can (often, though not
> always) be more clearly expressed and then such things can be more
> reasonably left as being implicit in comments.  But we don't have
> that so if we're going to be cleaning things up, and maybe even
> paying a little attention to API design, I figure IWBN to have
> things be clearer than they are today.
> 
> [Plus I get bitten time and again by taking gdb's existing practice
> as something we actually want to keep. :-)]
> 
> > How about I change the comment to "Return _a_pointer_to_ the
> > register cache..."?  (underlines for emphasis here).
> 
> If one was going to add emphasis, I'd emphasize _the_. :-)

I tried to figure out how to succinctly write what you're asking me
to here but I'm stuck.  Is there a function documented in this way
in GDB I can use as an example?

In the interests of keeping things moving I've pushed the patch with
this comment:

   Return a pointer to the register cache associated with the
   thread specified by PTID.  This function must be provided by
   the client.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH 5/9 v7] Introduce common-regcache.h

[Doug Evans]

On Fri, Sep 12, 2014 at 2:45 AM, Gary Benson <gbenson@redhat.com> wrote:
> I tried to figure out how to succinctly write what you're asking me
> to here but I'm stuck.  Is there a function documented in this way
> in GDB I can use as an example?
>
> In the interests of keeping things moving I've pushed the patch with
> this comment:
>
>    Return a pointer to the register cache associated with the
>    thread specified by PTID.  This function must be provided by
>    the client.

Fine by me.

[PATCH 6/9 v7] Include common-defs.h instead of defs.h/server.h in shared code

[Gary Benson]

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

This patch is unchanged from the version I posted August 1:
https://sourceware.org/ml/gdb-patches/2014-08/msg00012.html

gdb/ChangeLog:

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

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

Re: [PATCH 6/9 v7] Include common-defs.h instead of defs.h/server.h in shared code

[Pedro Alves]

On 08/29/2014 02:51 PM, Gary Benson wrote:
> This commit makes 19 of the 22 shared .c files in common, nat and
> target include common-defs.h instead of defs.h/server.h.  The
> remaining three files need slight extra work and are dealt with
> in separate commits.
> 
> This patch is unchanged from the version I posted August 1:
> https://sourceware.org/ml/gdb-patches/2014-08/msg00012.html
> 
> gdb/ChangeLog:
> 
> 	* common/agent.c: Include common-defs.h.
> 	Don't include defs.h or server.h.
> 	* common/buffer.c: Likewise.
> 	* common/common-debug.c: Likewise.
> 	* common/common-utils.c: Likewise.
> 	* common/errors.c: Likewise.
> 	* common/filestuff.c: Likewise.
> 	* common/format.c: Likewise.
> 	* common/gdb_vecs.c: Likewise.
> 	* common/print-utils.c: Likewise.
> 	* common/ptid.c: Likewise.
> 	* common/rsp-low.c: Likewise.
> 	* common/signals.c: Likewise.
> 	* common/vec.c: Likewise.
> 	* common/xml-utils.c: Likewise.
> 	* nat/linux-osdata.c: Likewise.
> 	* nat/linux-procfs.c: Likewise.
> 	* nat/linux-ptrace.c: Likewise.
> 	* nat/mips-linux-watch.c: Likewise.
> 	* target/waitstatus.c: Likewise.

Looks good to me.

Thanks,
Pedro Alves

Re: [PATCH 0/9 v7] Common code cleanups

[Doug Evans]

Gary Benson writes:
 > Hi all,
 > 
 > This series contains updated versions of the remaining common
 > cleanups patches I last posted on August 1 and 13.
 > 
 > I've documented the changes I've made for each patch in their
 > individual emails.  These paragraphs will not be included in
 > the commit messages.
 > 
 > Built and regtested on RHEL 6.5 x86_64.
 > 
 > Ok to commit?
 > 
 > Thanks,
 > Gary

Hi.

FAOD, I have no further comments beyond what Pedro has mentioned.