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

[Pedro Alves <palves at redhat dot com>]

On 01/23/2012 01:38 PM, Yao Qi wrote:
> Existing gdbserver is able to communicate with agent via socket and
> command buffer.  We want to grow this way for a more general purpose --
> for gdb/gdbserver to communicate with agent for different types of
> operations.  This patch is to move this bits of code to common/agent.c,
> so that these routines can be used by gdb and gdbserver
> 
> Since this part of code needs to query symbol and access inferior's
> memory, these APIs are different between gdb and gdbserver.  This causes
> some #ifdef/#endif blocks in the code.
> 
> Previously, the communication channel is quite specific to gdbserver and
> tracepoint.  With this patch, the communication channel is more generic,
> so that gdb and gdbserver can use this channel to talk with agent.
> 
> -- Yao (éå)
> 
> 
> 0001-move-gdbserver-tracepoint-to-common-agent.patch
> 
> 
> 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         |  262 ++++++++++++++++++++++++++++++++++++++++++++
>  gdb/common/agent.h         |   38 +++++++
>  gdb/gdbserver/Makefile.in  |    6 +-
>  gdb/gdbserver/tracepoint.c |  171 +----------------------------
>  4 files changed, 311 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..5c66071
> --- /dev/null
> +++ b/gdb/common/agent.c
> @@ -0,0 +1,262 @@
> +/* Shared utility routines for GDB to interact with agent.
> +
> +   Copyright (C) 2012 Free Software Foundation, Inc.

I'm still not clear on if when we move code to a new file, we
can just drop the previous copyright years.  The way I think of it is,
if that were possible, then e.g., renaming a file would produce a file
with just the current year as copyright.

> +
> +   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;

All these globals should be make per-process at some point.

> +
> +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
> +

Spurious space.

> +      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)

"ust" no longer makes sense.


> +{
> +  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;
> +    }

All UNIX_PATH_MAX, PF_UNIX, P_tmpdir, etc business, doesn't compile or work
on all supported GDB hosts...  This used to be wrapped on HAVE_UST, so it
wasn't a problem when it lived in gdbserver.

> +
> +  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.  */
> +
> +void
> +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;
> +    }
> +
> +  /* 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");

"fprintf (stderr, " is not the proper way to output log messages on the gdb side.

> +
> +      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");
> +    }
> +
> +#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

Since there's no #else, I take it you haven't really tried using this
on GDB, without gdbserver?

> +
> +  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");
> +	}
> +    }
> +}
> diff --git a/gdb/common/agent.h b/gdb/common/agent.h
> new file mode 100644
> index 0000000..079b65e
> --- /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/>.  */
> +
> +void 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)

These should probably move somewhere more central.

> +#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

Would be a good idea to prefix this.  IPA_CMD_BUF_SIZE ?

> +
> +extern int debug_agent ;

Spurious space before `;'.

> +
> +extern int use_agent;

This doesn't seem to be used anywhere in this patch.  It'd be best if only
added in the patch that actually introduces uses.

> 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..7f462bc 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"

Needs tracepoint.o's dependencies updated.

> +
>  #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 ();

So what if some of the symbols required by agent_look_up_symbols
aren't found (meaning, if we have an old agent loaded?

> +
>    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);
> -	}
> -    }
> +  agent_run_command (pid, (const char *) cmd, len);
>  
>    reinsert_all_breakpoints ();
>    unpause_all (0);
> -- 1.7.0.4


-- 
Pedro Alves