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

Yao Qi
2012-01-30T08:50:55Z

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