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

Pedro Alves
2012-02-23T21:01:44Z

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