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

Pedro Alves
2012-02-09T19:21:20Z

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