Re: [PATCH 1/9] move agent related code from gdbserver to common/agent.c

[Pedro Alves <palves@redhat.com>]

On 02/17/2012 02:54 AM, Yao Qi wrote:

> 2012-02-13  Yao Qi  <yao@codesourcery.com>
> 
> 	* common/agent.c: New.
> 	* common/agent.h: New.
> 	* configure.ac: Add `sys/socket.h' and `linux/un.h' to
> 	AC_CHECK_HEADERS.
> 	* configure, configh.in: Regenerated.
> 
> gdb/gdbserver:
> 
> 2012-02-13  Yao Qi  <yao@codesourcery.com>
> 
>         * Makefile.in (OBS): Add agent.o.
>         Add new rule for agent.o.
> 	Track dependence of tracepoint.c on agent.h.
>         * 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_ust_socket_init): Renamed to ...
> 	(gdb_agent_socket_init): ... New.
> 	(gdb_ust_thread): Renamed to ...
>         (gdb_agent_helper_thread): ... New.
>         (gdb_ust_init): Move some code to ...
>         (gdb_agent_init): ... here.  New.
> 	[HAVE_UST]: Call gdb_ust_init.
>         (initialize_tracepoint_ftlib):  Call gdb_agent_init.
> 	* configure.ac: Add `linux/un.h' to AC_CHECK_HEADERS.
> 	* config.in, configure: Regenerated.

Something fishy with the indentation here.  Make entries and indented
with one tab.

> ---
>  gdb/common/agent.c         |  297 ++++++++++++++++++++++++++++++++++++++++++++
>  gdb/common/agent.h         |   37 ++++++
>  gdb/config.in              |    6 +
>  gdb/configure              |   20 +++-
>  gdb/configure.ac           |    9 ++-
>  gdb/gdbserver/Makefile.in  |    8 +-
>  gdb/gdbserver/config.in    |    3 +
>  gdb/gdbserver/configure    |   19 +++
>  gdb/gdbserver/configure.ac |    8 ++
>  gdb/gdbserver/tracepoint.c |  285 +++++++++---------------------------------
>  10 files changed, 464 insertions(+), 228 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..657bba6
> --- /dev/null
> +++ b/gdb/common/agent.c
> @@ -0,0 +1,297 @@
> +/* Shared utility routines for GDB to interact with agent.
> +
> +   Copyright (C) 2009-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"
> +#include "inferior.h" /* for non_stop */
> +#endif
> +
> +#include <string.h>
> +#include <unistd.h>
> +#include "agent.h"
> +
> +int debug_agent = 0;
> +
> +#ifdef GDBSERVER
> +#define DEBUG_AGENT(fmt, args...)	\
> +  if (debug_agent)			\
> +    fprintf (stderr, fmt, ##args);
> +#else
> +#define DEBUG_AGENT(fmt, args...)	\
> +  if (debug_agent)			\
> +    fprintf_unfiltered (gdb_stdlog, fmt, ##args);
> +#endif
> +
> +/* 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.  FIXME: this global should be made
> +   per-process.  */
> +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;
> +
> +/* Look up all symbols needed by agent.  Return 0 if all the symbols are
> +   found, return non-zero otherwise.  */
> +
> +int
> +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
> +	{
> +	  DEBUG_AGENT ("symbol `%s' not found\n", symbol_list[i].name);
> +	  return -1;
> +	}
> +    }
> +
> +  return 0;
> +}
> +
> +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);
> +      else
> +#endif
> +	{
> +	  warning ("Error reading helper thread's id in lib");
> +	}
> +    }
> +
> +  return helper_thread_id;
> +}
> +
> +#ifdef HAVE_LINUX_UN_H
> +#include <sys/socket.h>
> +#include <linux/un.h>
> +#define SOCK_DIR P_tmpdir
> +#endif
> +
> +/* Connects to synchronization socket.  PID is the pid of inferior, which is
> +   used to set up connection socket.  */

set up the connection socket.

> +
> +static int
> +gdb_connect_sync_socket (int pid)
> +{
> +#ifdef HAVE_LINUX_UN_H
> +  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;
> +#else
> +  return -1;
> +#endif
> +}
> +
> +/* Execute an agent command in inferior.  PID is the value of pid of inferior.

in the inferior.   of the inferior.

> +   CMD is the buffer for command.  GDB or GDBserver will store command into

store the command.

> +   it and fetch return result from CMD.  The interaction between GDB/GDBserver

fetch the return result.

> +   and agent is synchronized by synchronization socket.  Return zeor if success,

and the agent.  by a synchronization socket.  zero.

> +   otherwise return non-zero.  */
> +
> +int
> +agent_run_command (int pid, const char *cmd)
> +{

The code at the caller in gdbserver also stops all threads (try with non-stop),
and removes breakpoints , so that the helper thread doesn't trip on any
breakpoint (e.g., a breakpoint at write/read, or a catch system),
so this is easy to break on gdb as is, but okay.

> +  int fd;
> +  int tid = agent_get_helper_thread_id ();
> +  ptid_t ptid = ptid_build (pid, tid, 0);
> +  int len = strlen (cmd) + 1;
> +
> +#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;
> +    }
> +
> +  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 = TARGET_SIGNAL_0;
> +  (*the_target->resume) (&resume_info, 1);
> +}
> +#else
> + target_resume (ptid, 0, TARGET_SIGNAL_0);
> +#endif
> +
> +  fd = gdb_connect_sync_socket (pid);
> +  if (fd >= 0)
> +    {
> +      char buf[1] = "";
> +      int ret;
> +
> +      DEBUG_AGENT ("agent: signalling helper thread\n");
> +
> +      do
> +	{
> +	  ret = write (fd, buf, 1);
> +	} while (ret == -1 && errno == EINTR);
> +
> +	DEBUG_AGENT ("agent:waiting for helper thread's response\n");

missing space after colon.

> +
> +      do
> +	{
> +	  ret = read (fd, buf, 1);
> +	} while (ret == -1 && errno == EINTR);
> +
> +      close (fd);
> +
> +      DEBUG_AGENT ("agent:helper thread's response received\n");

missing space after colon.


> +    }
> +  else
> +    return -1;
> +
> +  /* 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 = TARGET_SIGNAL_0;
> +	(*the_target->resume) (&resume_info, 1);
> +      }
> +
> +      non_stop = 1;
> +      mywait (ptid, &status, 0, 0);
> +#else
> +      non_stop = 1;
> +      target_stop (ptid);
> +
> +      memset (&status, 0, sizeof (status));
> +      target_wait (ptid, &status, 0);
> +#endif
> +      non_stop = was_non_stop;
> +    }
> +
> +  if (fd >= 0)
> +    {
> +#ifdef GDBSERVER
> +      if (read_inferior_memory (ipa_sym_addrs.addr_cmd_buf,
> +				(unsigned char *) cmd, IPA_CMD_BUF_SIZE))
> +#else
> +      if (target_read_memory (ipa_sym_addrs.addr_cmd_buf, (gdb_byte *) cmd,
> +			      IPA_CMD_BUF_SIZE))
> +#endif
> +	{
> +	  warning ("Error reading command response");
> +	  return -1;
> +	}
> +    }
> +
> +  return 0;
> +}


> @@ -7746,24 +7572,22 @@ run_inferior_command (char *cmd)
>    return err;
>  }
>  
> -#else /* HAVE_UST */
> +#else /* !IN_PROCESS_AGENT */
>  
> -static int
> -run_inferior_command (char *cmd)
> -{
> -  return -1;
> -}
> +#include <sys/socket.h>
> +#include <sys/un.h>
>  

Hard to read, but are these now outside of HAVE_UST?  That'll break
mingw builds.


> -#endif /* HAVE_UST */
> +#ifndef UNIX_PATH_MAX
> +#define UNIX_PATH_MAX sizeof(((struct sockaddr_un *) NULL)->sun_path)
> +#endif
>  
> -#else /* !IN_PROCESS_AGENT */
> +/* Where we put the socked used for synchronization.  */
> +#define SOCK_DIR P_tmpdir



> @@ -8107,7 +7946,12 @@ gdb_ust_thread (void *arg)
>  
>  	  if (cmd_buf[0])
>  	    {
> -	      if (strcmp ("qTfSTM", cmd_buf) == 0)
> +	      if (strcmp (cmd_buf, "help") == 0)
> +		{
> +		  strcpy (cmd_buf, "for help, press F1\n");
> +		}

LOL, I remember writing that as a joke, but I didn't remember
that it had ended up in the tree.

> +#ifdef HAVE_UST
> +	      else if (strcmp ("qTfSTM", cmd_buf) == 0)
>  		{
>  		  cmd_qtfstm (cmd_buf);
>  		}
> @@ -8133,10 +7977,7 @@ gdb_ust_thread (void *arg)
>  		{
>  		  cmd_qtstmat (cmd_buf);
>  		}
> -	      else if (strcmp (cmd_buf, "help") == 0)
> -		{
> -		  strcpy (cmd_buf, "for help, press F1\n");
> -		}
> +#endif /* HAVE_UST */
>  	      else
>  		strcpy (cmd_buf, "");
>  	    }
> @@ -8151,18 +7992,16 @@ gdb_ust_thread (void *arg)
>  }

Otherwise okay.

-- 
Pedro Alves