Re: [patch 1/8] Generalize interaction with agent in gdb/gdbserver

[Yao Qi <yao@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 310 bytes --]

On 01/23/2012 09:38 PM, Yao Qi wrote:
> +void
> +agent_run_command (int pid, const char *cmd, int len)
> +{

When reading this patch today, I find this function should return a
value so that caller of this function can easily check the success or
failure.

Here is the updated version.

-- 
Yao (齐尧)

[-- Attachment #2: 0001-move-gdbserver-tracepoint-to-common-agent.patch --]
[-- Type: text/x-patch, Size: 16212 bytes --]

gdb:

2012-01-23  Yao Qi  <yao@codesourcery.com>

	* common/agent.c: New.
	* common/agent.h: New.

gdb/gdbserver:

2012-01-23  Yao Qi  <yao@codesourcery.com>

        * Makefile.in (OBS): Add agent.o.
        Add new rule for agent.o.
        * tracepoint.c (run_inferior_command_1):
        (run_inferior_command): Call agent_run_command.
        (gdb_ust_connect_sync_socket): Deleted.  Move it to
        common/agent.c.
        (resume_thread, stop_thread): Likewise.
---
 gdb/common/agent.c         |  268 ++++++++++++++++++++++++++++++++++++++++++++
 gdb/common/agent.h         |   38 ++++++
 gdb/gdbserver/Makefile.in  |    6 +-
 gdb/gdbserver/tracepoint.c |  171 +---------------------------
 4 files changed, 317 insertions(+), 166 deletions(-)
 create mode 100644 gdb/common/agent.c
 create mode 100644 gdb/common/agent.h

diff --git a/gdb/common/agent.c b/gdb/common/agent.c
new file mode 100644
index 0000000..e17715d
--- /dev/null
+++ b/gdb/common/agent.c
@@ -0,0 +1,268 @@
+/* Shared utility routines for GDB to interact with agent.
+
+   Copyright (C) 2012 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifdef GDBSERVER
+#include "server.h"
+#else
+#include "defs.h"
+#include "target.h"
+#endif
+
+#include <sys/socket.h>
+#include <linux/un.h>
+#include <string.h>
+#include <unistd.h>
+#include "agent.h"
+
+int debug_agent = 0;
+
+/* Global flag to determine using agent or not.  */
+int use_agent = 0;
+
+#define SOCK_DIR P_tmpdir
+
+/* Addresses of in-process agent's symbols both GDB and GDBserver cares
+   about.  */
+
+struct ipa_sym_addresses
+{
+  CORE_ADDR addr_helper_thread_id;
+  CORE_ADDR addr_cmd_buf;
+};
+
+/* Cache of the helper thread id.  */
+static unsigned int helper_thread_id = 0;
+
+static struct
+{
+  const char *name;
+  int offset;
+  int required;
+} symbol_list[] = {
+  IPA_SYM(helper_thread_id),
+  IPA_SYM(cmd_buf),
+};
+
+static struct ipa_sym_addresses ipa_sym_addrs;
+
+void
+agent_look_up_symbols (void)
+{
+  int i;
+
+  for (i = 0; i < sizeof (symbol_list) / sizeof (symbol_list[0]); i++)
+    {
+      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 minimal_symbol *sym = lookup_minimal_symbol (symbol_list[i].name,
+							  NULL, NULL);
+
+      if (sym != NULL)
+	*addrp = SYMBOL_VALUE_ADDRESS (sym);
+      else
+#endif
+	{
+	  if (debug_agent)
+	    fprintf (stderr, "symbol `%s' not found\n", symbol_list[i].name);
+	}
+    }
+}
+
+static unsigned int
+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);
+#endif
+	{
+	  warning ("Error reading helper thread's id in lib");
+	}
+    }
+
+  return helper_thread_id;
+}
+
+/* Connects to synchronization socket.  PID is the pid of inferior, which is
+   used to set up connection socket.  */
+
+static int
+gdb_ust_connect_sync_socket (int pid)
+{
+  struct sockaddr_un addr;
+  int res, fd;
+  char path[UNIX_PATH_MAX];
+
+  res = xsnprintf (path, UNIX_PATH_MAX, "%s/gdb_ust%d", P_tmpdir, pid);
+  if (res >= UNIX_PATH_MAX)
+    return -1;
+
+  res = fd = socket (PF_UNIX, SOCK_STREAM, 0);
+  if (res == -1)
+    {
+      warning ("error opening sync socket: %s\n", strerror (errno));
+      return -1;
+    }
+
+  addr.sun_family = AF_UNIX;
+
+  res = xsnprintf (addr.sun_path, UNIX_PATH_MAX, "%s", path);
+  if (res >= UNIX_PATH_MAX)
+    {
+      warning ("string overflow allocating socket name\n");
+      close (fd);
+      return -1;
+    }
+
+  res = connect (fd, (struct sockaddr *) &addr, sizeof (addr));
+  if (res == -1)
+    {
+      warning ("error connecting sync socket (%s): %s. "
+	       "Make sure the directory exists and that it is writable.",
+	       path, strerror (errno));
+      close (fd);
+      return -1;
+    }
+
+  return fd;
+}
+
+/* Execute an agent command in inferior.  PID is the value of pid of inferior.
+   CMD is the buffer for command, and LEN is length of command.  GDB or
+   GDBserver will store command into it and fetch return result from CMD.
+   The interaction between GDB/GDBserver and agent is synchronized by
+   synchronization socket.  Return zeor if success, otherwise return
+   non-zero.  */
+
+int
+agent_run_command (int pid, const char *cmd, int len)
+{
+  int fd;
+  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, cmd, len);
+#endif
+
+  if (ret != 0)
+    {
+      warning ("unable to write");
+      return -1;
+    }
+
+  /* Resume helper thread.  */
+#ifdef GDBSERVER
+{
+  struct thread_resume resume_info;
+
+  resume_info.thread = ptid;
+  resume_info.kind = resume_continue;
+  resume_info.sig = TARGET_SIGNAL_0;
+  (*the_target->resume) (&resume_info, 1);
+}
+#else
+ target_resume (ptid, 0, TARGET_SIGNAL_0);
+#endif
+
+  fd = gdb_ust_connect_sync_socket (pid);
+  if (fd >= 0)
+    {
+      char buf[1] = "";
+      int ret;
+
+      if (debug_agent)
+	fprintf (stderr, "agent: signalling helper thread\n");
+
+      do
+	{
+	  ret = write (fd, buf, 1);
+	} while (ret == -1 && errno == EINTR);
+
+      if (debug_agent)
+	fprintf (stderr, "agent:waiting for helper thread's response\n");
+
+      do
+	{
+	  ret = read (fd, buf, 1);
+	} while (ret == -1 && errno == EINTR);
+
+      close (fd);
+
+      if (debug_agent)
+	fprintf (stderr, "agent:helper thread's response received\n");
+    }
+  else
+    return -1;
+
+#ifdef GDBSERVER
+  /* Need to read response with the inferior stopped.  */
+  if (!ptid_equal (ptid, null_ptid))
+    {
+      int was_non_stop = non_stop;
+      struct target_waitstatus status;
+      struct thread_resume resume_info;
+
+      /* Stop thread PTID.  */
+      resume_info.thread = ptid;
+      resume_info.kind = resume_stop;
+      resume_info.sig = TARGET_SIGNAL_0;
+      (*the_target->resume) (&resume_info, 1);
+
+      non_stop = 1;
+      mywait (ptid, &status, 0, 0);
+      non_stop = was_non_stop;
+    }
+#endif
+
+  if (fd >= 0)
+    {
+#ifdef GDBSERVER
+      if (read_inferior_memory (ipa_sym_addrs.addr_cmd_buf,
+				(unsigned char *) cmd, CMD_BUF_SIZE))
+#else
+      if (target_read_memory (ipa_sym_addrs.addr_cmd_buf, (gdb_byte *) cmd,
+			      CMD_BUF_SIZE))
+#endif
+	{
+	  warning ("Error reading command response");
+	  return -1;
+	}
+    }
+
+  return 0;
+}
diff --git a/gdb/common/agent.h b/gdb/common/agent.h
new file mode 100644
index 0000000..3a415ba
--- /dev/null
+++ b/gdb/common/agent.h
@@ -0,0 +1,38 @@
+/* Shared utility routines for GDB to interact with agent.
+
+   Copyright (C) 2012 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/>.  */
+
+int agent_run_command (int pid, const char *cmd, int len);
+
+void agent_look_up_symbols (void);
+
+#define STRINGIZE_1(STR) #STR
+#define STRINGIZE(STR) STRINGIZE_1(STR)
+#define IPA_SYM(SYM)                                   \
+  {                                                    \
+    STRINGIZE (gdb_agent_ ## SYM),			\
+    offsetof (struct ipa_sym_addresses, addr_ ## SYM)	\
+  }
+
+/* The size in bytes of the buffer used to talk to the IPA helper
+   thread.  */
+#define CMD_BUF_SIZE 1024
+
+extern int debug_agent ;
+
+extern int use_agent;
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 6ccf5ae..ddca704 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -135,7 +135,7 @@ SOURCES = $(SFILES)
 TAGFILES = $(SOURCES) ${HFILES} ${ALLPARAM} ${POSSLIBS}
 
 OBS = inferiors.o regcache.o remote-utils.o server.o signals.o target.o \
-	utils.o version.o \
+	utils.o version.o agent.o \
 	mem-break.o hostio.o event-loop.o tracepoint.o \
 	xml-utils.o common-utils.o ptid.o buffer.o \
 	$(XML_BUILTIN) \
@@ -335,6 +335,7 @@ regcache_h = $(srcdir)/regcache.h
 signals_def = $(srcdir)/../../include/gdb/signals.def
 signals_h = $(srcdir)/../../include/gdb/signals.h $(signals_def)
 ptid_h = $(srcdir)/../common/ptid.h
+agent_h = $(srcdir)/../common/agent.h
 linux_osdata_h = $(srcdir)/../common/linux-osdata.h
 server_h = $(srcdir)/server.h $(regcache_h) config.h $(srcdir)/target.h \
 		$(srcdir)/mem-break.h $(srcdir)/../common/gdb_signals.h \
@@ -423,6 +424,9 @@ ptid.o: ../common/ptid.c $(ptid_h)
 buffer.o: ../common/buffer.c $(server_h)
 	$(CC) -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $< -DGDBSERVER
 
+agent.o: ../common/agent.c $(server_h) $(agent_h)
+	$(CC) -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $< -DGDBSERVER
+
 # We build memmem.c without -Werror because this file is not under
 # our control.  On LynxOS, the compiler generates some warnings
 # because str-two-way.h uses a constant (MAX_SIZE) whose definition
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 3e6dac0..bf4f25b 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -17,6 +17,8 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "server.h"
+#include "agent.h"
+
 #include <ctype.h>
 #include <fcntl.h>
 #include <unistd.h>
@@ -185,18 +187,8 @@ struct ipa_sym_addresses
   CORE_ADDR addr_get_trace_state_variable_value;
   CORE_ADDR addr_set_trace_state_variable_value;
   CORE_ADDR addr_ust_loaded;
-  CORE_ADDR addr_helper_thread_id;
-  CORE_ADDR addr_cmd_buf;
 };
 
-#define STRINGIZE_1(STR) #STR
-#define STRINGIZE(STR) STRINGIZE_1(STR)
-#define IPA_SYM(SYM)					\
-  {							\
-    STRINGIZE (gdb_agent_ ## SYM),			\
-    offsetof (struct ipa_sym_addresses, addr_ ## SYM)	\
-  }
-
 static struct
 {
   const char *name;
@@ -232,11 +224,9 @@ static struct
   IPA_SYM(get_trace_state_variable_value),
   IPA_SYM(set_trace_state_variable_value),
   IPA_SYM(ust_loaded),
-  IPA_SYM(helper_thread_id),
-  IPA_SYM(cmd_buf),
 };
 
-struct ipa_sym_addresses ipa_sym_addrs;
+static struct ipa_sym_addresses ipa_sym_addrs;
 
 int all_tracepoint_symbols_looked_up;
 
@@ -355,6 +345,8 @@ tracepoint_look_up_symbols (void)
 	}
     }
 
+  agent_look_up_symbols ();
+
   all_tracepoint_symbols_looked_up = 1;
 }
 
@@ -1312,10 +1304,6 @@ static LONGEST get_timestamp (void);
 #define cmpxchg(mem, oldval, newval) \
   __sync_val_compare_and_swap (mem, oldval, newval)
 
-/* The size in bytes of the buffer used to talk to the IPA helper
-   thread.  */
-#define CMD_BUF_SIZE 1024
-
 /* Record that an error occurred during expression evaluation.  */
 
 static void
@@ -7566,76 +7554,6 @@ static struct ltt_available_probe gdb_ust_probe =
 
 #ifdef HAVE_UST
 
-static int
-gdb_ust_connect_sync_socket (int pid)
-{
-  struct sockaddr_un addr;
-  int res, fd;
-  char path[UNIX_PATH_MAX];
-
-  res = xsnprintf (path, UNIX_PATH_MAX, "%s/gdb_ust%d", SOCK_DIR, pid);
-  if (res >= UNIX_PATH_MAX)
-    {
-      trace_debug ("string overflow allocating socket name");
-      return -1;
-    }
-
-  res = fd = socket (PF_UNIX, SOCK_STREAM, 0);
-  if (res == -1)
-    {
-      warning ("error opening sync socket: %s\n", strerror (errno));
-      return -1;
-    }
-
-  addr.sun_family = AF_UNIX;
-
-  res = xsnprintf (addr.sun_path, UNIX_PATH_MAX, "%s", path);
-  if (res >= UNIX_PATH_MAX)
-    {
-      warning ("string overflow allocating socket name\n");
-      close (fd);
-      return -1;
-    }
-
-  res = connect (fd, (struct sockaddr *) &addr, sizeof (addr));
-  if (res == -1)
-    {
-      warning ("error connecting sync socket (%s): %s. "
-	       "Make sure the directory exists and that it is writable.",
-	       path, strerror (errno));
-      close (fd);
-      return -1;
-    }
-
-  return fd;
-}
-
-/* Resume thread PTID.  */
-
-static void
-resume_thread (ptid_t ptid)
-{
-  struct thread_resume resume_info;
-
-  resume_info.thread = ptid;
-  resume_info.kind = resume_continue;
-  resume_info.sig = TARGET_SIGNAL_0;
-  (*the_target->resume) (&resume_info, 1);
-}
-
-/* Stop thread PTID.  */
-
-static void
-stop_thread (ptid_t ptid)
-{
-  struct thread_resume resume_info;
-
-  resume_info.thread = ptid;
-  resume_info.kind = resume_stop;
-  resume_info.sig = TARGET_SIGNAL_0;
-  (*the_target->resume) (&resume_info, 1);
-}
-
 /* 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
@@ -7647,91 +7565,14 @@ static int
 run_inferior_command (char *cmd)
 {
   int err = -1;
-  int fd = -1;
   int pid = ptid_get_pid (current_inferior->entry.id);
-  int tid;
-  ptid_t ptid = null_ptid;
 
   trace_debug ("run_inferior_command: running: %s", cmd);
 
   pause_all (0);
   uninsert_all_breakpoints ();
 
-  if (read_inferior_integer (ipa_sym_addrs.addr_helper_thread_id, &tid))
-    {
-      warning ("Error reading helper thread's id in lib");
-      goto out;
-    }
-
-  if (tid == 0)
-    {
-      warning ("helper thread not initialized yet");
-      goto out;
-    }
-
-  if (write_inferior_memory (ipa_sym_addrs.addr_cmd_buf,
-			     (unsigned char *) cmd, strlen (cmd) + 1))
-    {
-      warning ("Error writing command");
-      goto out;
-    }
-
-  ptid = ptid_build (pid, tid, 0);
-
-  resume_thread (ptid);
-
-  fd = gdb_ust_connect_sync_socket (pid);
-  if (fd >= 0)
-    {
-      char buf[1] = "";
-      int ret;
-
-      trace_debug ("signalling helper thread");
-
-      do
-	{
-	  ret = write (fd, buf, 1);
-	} while (ret == -1 && errno == EINTR);
-
-      trace_debug ("waiting for helper thread's response");
-
-      do
-	{
-	  ret = read (fd, buf, 1);
-	} while (ret == -1 && errno == EINTR);
-
-      close (fd);
-
-      trace_debug ("helper thread's response received");
-    }
-
- out:
-
-  /* Need to read response with the inferior stopped.  */
-  if (!ptid_equal (ptid, null_ptid))
-    {
-      int was_non_stop = non_stop;
-      struct target_waitstatus status;
-
-      stop_thread (ptid);
-      non_stop = 1;
-      mywait (ptid, &status, 0, 0);
-      non_stop = was_non_stop;
-    }
-
-  if (fd >= 0)
-    {
-      if (read_inferior_memory (ipa_sym_addrs.addr_cmd_buf,
-				(unsigned char *) cmd, CMD_BUF_SIZE))
-	{
-	  warning ("Error reading command response");
-	}
-      else
-	{
-	  err = 0;
-	  trace_debug ("run_inferior_command: response: %s", cmd);
-	}
-    }
+  err = agent_run_command (pid, (const char *) cmd, len);
 
   reinsert_all_breakpoints ();
   unpause_all (0);
-- 
1.7.0.4