* [patch 1/8] Generalize interaction with agent in gdb/gdbserver
2012-01-23 13:37 [patch 0/8] GDB/GDBserver talks with agents Yao Qi
@ 2012-01-23 13:48 ` Yao Qi
2012-01-30 11:25 ` Yao Qi
2012-02-09 19:21 ` Pedro Alves
2012-01-23 13:50 ` [patch 2/8] Add to_use_agent in target_ops Yao Qi
` (8 subsequent siblings)
9 siblings, 2 replies; 46+ messages in thread
From: Yao Qi @ 2012-01-23 13:48 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 754 bytes --]
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 (é½å°§)
[-- Attachment #2: 0001-move-gdbserver-tracepoint-to-common-agent.patch --]
[-- Type: text/x-patch, Size: 16097 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 | 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.
+
+ 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. */
+
+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");
+
+ 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
+
+ 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)
+#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..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"
+
#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);
- }
- }
+ agent_run_command (pid, (const char *) cmd, len);
reinsert_all_breakpoints ();
unpause_all (0);
--
1.7.0.4
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 1/8] Generalize interaction with agent in gdb/gdbserver
2012-01-23 13:48 ` [patch 1/8] Generalize interaction with agent in gdb/gdbserver Yao Qi
@ 2012-01-30 11:25 ` Yao Qi
2012-02-09 19:21 ` Pedro Alves
1 sibling, 0 replies; 46+ messages in thread
From: Yao Qi @ 2012-01-30 11:25 UTC (permalink / raw)
To: gdb-patches
[-- 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
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 1/8] Generalize interaction with agent in gdb/gdbserver
2012-01-23 13:48 ` [patch 1/8] Generalize interaction with agent in gdb/gdbserver Yao Qi
2012-01-30 11:25 ` Yao Qi
@ 2012-02-09 19:21 ` Pedro Alves
2012-02-14 2:41 ` Yao Qi
1 sibling, 1 reply; 46+ messages in thread
From: Pedro Alves @ 2012-02-09 19:21 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 1/8] Generalize interaction with agent in gdb/gdbserver
2012-02-09 19:21 ` Pedro Alves
@ 2012-02-14 2:41 ` Yao Qi
2012-02-14 10:16 ` Pedro Alves
0 siblings, 1 reply; 46+ messages in thread
From: Yao Qi @ 2012-02-14 2:41 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 02/10/2012 03:20 AM, Pedro Alves wrote:
>> > +#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?
>
I thought this part can be removed. The intention of this part is to
really stop "debugging thread", so it is safe to read from command
buffer later. We don't have "debugging thread" stopped, because the
synchronization of read and write is controlled by socket. When we get
here, after reading one byte from socket, "debugging thread" has
finished executing command, and write return result in command buffer.
It is being blocked by reading from socket, even it is not stopped.
GDB/GDBserver can safely read contents out of command buffer without
having to stop "debugging thread". Am I missing something here?
I get rid of this part from its original place (gdbserver/tracepoint.c),
and run gdb.trace/strace.exp. Results look unchanged.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 1/8] Generalize interaction with agent in gdb/gdbserver
2012-02-14 2:41 ` Yao Qi
@ 2012-02-14 10:16 ` Pedro Alves
0 siblings, 0 replies; 46+ messages in thread
From: Pedro Alves @ 2012-02-14 10:16 UTC (permalink / raw)
To: Yao Qi; +Cc: Pedro Alves, gdb-patches
On 02/14/2012 02:40 AM, Yao Qi wrote:
> On 02/10/2012 03:20 AM, Pedro Alves wrote:
>>>> +#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?
>>
>
> I thought this part can be removed.
Then that would be a separate change.
> The intention of this part is to really stop "debugging thread", so it is safe
> to read from command buffer later. We don't have "debugging thread" stopped,
> because the synchronization of read and write is controlled by socket. When
> we get here, after reading one byte from socket, "debugging thread" has
> finished executing command, and write return result in command buffer.
> It is being blocked by reading from socket, even it is not stopped.
> GDB/GDBserver can safely read contents out of command buffer without
> having to stop "debugging thread". Am I missing something here?
It's not really about "safeness". With ptrace or /proc/PID/mem, you can only read
memory from the inferior is it is ptrace-stopped (stopped by a signal, breakpoint, etc.).
From ptrace's perpective, even if the thread is blocked reading from a socket,
the thread is running. Trying to read memory while the thread is running results in
error. Now, what is happening is that we're actually not reading the command
buffer with the helper thread selected as current thread, but instead we're reading
memory through the thread that was selected at run_inferior_command entry (which is
the thread you have selected in GDB). And that happens to be stopped. Try selecting
the helper thread instead:
(gdb) t 3
[Switching to thread 3 (Thread 22463)]
#0 0x0000000000396223 in ?? ()
(gdb) info static-tracepoint-markers
Cnt ID Enb Address What
Remote failure reply: E.UST library not loaded in process. Static tracepoints unavailable.
(gdb)
and things break.
Even if you don't do that, removing that bit breaks gdbserver's track of what is stopped
or not. Vis:
(gdb) b main
Breakpoint 1 at 0x400aa6: file ../../../src/gdb/testsuite/gdb.trace/strace.c, line 29.
(gdb) c
Continuing.
Breakpoint 1,
main () at ../../../src/gdb/testsuite/gdb.trace/strace.c:29
29 int a = 0;
(gdb) info static-tracepoint-markers
Cnt ID Enb Address What
1 metadata/core_marker_format n 0x00007ffff7bb7692
Data: "channel %s name %s format %s"
2 metadata/core_marker_format n 0x00007ffff7bb8897
Data: "channel %s name %s format %s"
3 metadata/core_marker_id n 0x00007ffff7bb957d
Data: "channel %s name %s event_id %hu int #1u%zu long #1u%zu pointer #1u%zu size_t #1u%zu alignment #1u%u"
4 metadata/core_marker_id n 0x00007ffff7bb9a04
Data: "channel %s name %s event_id %hu int #1u%zu long #1u%zu pointer #1u%zu size_t #1u%zu alignment #1u%u"
5 metadata/core_marker_id n 0x00007ffff7bbb006
Data: "channel %s name %s event_id %hu int #1u%zu long #1u%zu pointer #1u%zu size_t #1u%zu alignment #1u%u"
6 metadata/core_marker_format n 0x00007ffff7bbb235
Data: "channel %s name %s format %s"
7 ust/potential_exec n 0x00007ffff7bd8240
Data: " "
8 ust/bar n 0x0000000000400ab3 in main at ../../../src/gdb/testsuite/gdb.trace/strace.c:32
Data: "str %s"
9 ust/bar2 n 0x0000000000400afb in main at ../../../src/gdb/testsuite/gdb.trace/strace.c:33
Data: "number1 %d number2 %d"
10 ust/dummymark n 0x0000000000400b6c
Data: " "
(gdb) info threads
[New Thread 22690]
[New Thread 22691]
Id Target Id Frame
3 Thread 22691 0x0000000000653023 in ?? ()
2 Thread 22690 0x000000339e6e6af3 in __GI___poll (fds=<optimized out>, nfds=<optimized out>, timeout=<optimized out>) at ../sysdeps/unix/sysv/linux/poll.c:87
* 1 Thread 22681 main () at ../../../src/gdb/testsuite/gdb.trace/strace.c:29
(gdb)
Look at what's shown in gdbserver's console:
Listening on port 9999
Remote debugging from host 127.0.0.1
ptrace(regsets_fetch_inferior_registers) PID=22691: No such process
ptrace(regsets_fetch_inferior_registers) PID=22691: No such process
"No such process" also means "the process exists but was not stopped".
$ cat /proc/22681/task/*/status | grep State
State: t (tracing stop)
State: t (tracing stop)
State: S (sleeping)
That is, gdbserver got confused and tried to read registers from 22691 thinking it was
stopped, but it wasn't. And 22691 is thread 3, which is the helper thread.
Things go downhill from here.
> I get rid of this part from its original place (gdbserver/tracepoint.c),
> and run gdb.trace/strace.exp. Results look unchanged.
For the reasons explained above.
--
Pedro Alves
^ permalink raw reply [flat|nested] 46+ messages in thread
* [patch 2/8] Add to_use_agent in target_ops
2012-01-23 13:37 [patch 0/8] GDB/GDBserver talks with agents Yao Qi
2012-01-23 13:48 ` [patch 1/8] Generalize interaction with agent in gdb/gdbserver Yao Qi
@ 2012-01-23 13:50 ` Yao Qi
2012-02-09 19:36 ` Pedro Alves
2012-01-23 13:54 ` [patch 3/8] Command `set agent on|off' Yao Qi
` (7 subsequent siblings)
9 siblings, 1 reply; 46+ messages in thread
From: Yao Qi @ 2012-01-23 13:50 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 237 bytes --]
As agent goes in, we need to control the agent to perform debugging
operations. This new field in target_ops is used to control this, so
that we can implement different for remote debugging and native debugging.
--
Yao (é½å°§)
[-- Attachment #2: 0002-Add-to_use_agent-in-target.patch --]
[-- Type: text/x-patch, Size: 1887 bytes --]
2012-01-13 Yao Qi <yao@codesourcery.com>
* target.h (struct target_ops): New field `to_use_agent'.
(target_use_agent): New macro.
* target.c (update_current_target): Update.
---
gdb/target.c | 4 ++++
gdb/target.h | 7 +++++++
2 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/gdb/target.c b/gdb/target.c
index 9aaa0ea..e1955fa 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -697,6 +697,7 @@ update_current_target (void)
INHERIT (to_static_tracepoint_marker_at, t);
INHERIT (to_static_tracepoint_markers_by_strid, t);
INHERIT (to_traceframe_info, t);
+ INHERIT (to_use_agent, t);
INHERIT (to_magic, t);
/* Do not inherit to_memory_map. */
/* Do not inherit to_flash_erase. */
@@ -924,6 +925,9 @@ update_current_target (void)
de_fault (to_traceframe_info,
(struct traceframe_info * (*) (void))
tcomplain);
+ de_fault (to_use_agent,
+ (int (*) (int))
+ tcomplain);
de_fault (to_execution_direction, default_execution_direction);
#undef de_fault
diff --git a/gdb/target.h b/gdb/target.h
index 7d0bed1..cd3f3d4 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -795,6 +795,10 @@ struct target_ops
re-fetching when necessary. */
struct traceframe_info *(*to_traceframe_info) (void);
+ /* Ask the target to use or not to use agent according to USE. Return the
+ actual value of updated flag. */
+ int (*to_use_agent) (int use);
+
int to_magic;
/* Need sub-structure for target machine related rather than comm related?
*/
@@ -1569,6 +1573,9 @@ extern int target_search_memory (CORE_ADDR start_addr,
#define target_traceframe_info() \
(*current_target.to_traceframe_info) ()
+#define target_use_agent(use) \
+ (*current_target.to_use_agent) (use)
+
/* Command logging facility. */
#define target_log_command(p) \
--
1.7.0.4
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 2/8] Add to_use_agent in target_ops
2012-01-23 13:50 ` [patch 2/8] Add to_use_agent in target_ops Yao Qi
@ 2012-02-09 19:36 ` Pedro Alves
0 siblings, 0 replies; 46+ messages in thread
From: Pedro Alves @ 2012-02-09 19:36 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 01/23/2012 01:47 PM, Yao Qi wrote:
> As agent goes in, we need to control the agent to perform debugging
> operations. This new field in target_ops is used to control this,
> so that we can implement different for remote debugging and native debugging.
Damn, you've oversplit. :-) Mechanically looks fine...
I don't understand what you mean by "different for remote debugging and
native debugging", though, but I guess I'll find out in the next patches.
--
Pedro Alves
^ permalink raw reply [flat|nested] 46+ messages in thread
* [patch 3/8] Command `set agent on|off'
2012-01-23 13:37 [patch 0/8] GDB/GDBserver talks with agents Yao Qi
2012-01-23 13:48 ` [patch 1/8] Generalize interaction with agent in gdb/gdbserver Yao Qi
2012-01-23 13:50 ` [patch 2/8] Add to_use_agent in target_ops Yao Qi
@ 2012-01-23 13:54 ` Yao Qi
2012-01-23 17:14 ` Eli Zaretskii
2012-02-09 20:19 ` Pedro Alves
2012-01-23 13:58 ` [patch 4/8] `use_agent' for remote and QAgent Yao Qi
` (6 subsequent siblings)
9 siblings, 2 replies; 46+ messages in thread
From: Yao Qi @ 2012-01-23 13:54 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 154 bytes --]
We want to leave users to decide whether they want to use agent or not,
so we add a new command `set agent on|off' in this patch.
--
Yao (é½å°§)
[-- Attachment #2: 0003-add-agent.c-in-gdb.patch --]
[-- Type: text/x-patch, Size: 4346 bytes --]
2012-01-23 Yao Qi <yao@codesourcery.com>
* Makefile.in (SFILES): Add agent.c and common/agent.c.
(COMMON_OBS): Add agent.o and common/agent.o.
(common-agent.o): New rule.
* agent.c: New.
---
gdb/Makefile.in | 10 ++++++-
gdb/agent.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 76 insertions(+), 2 deletions(-)
create mode 100644 gdb/agent.c
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 25067f1..1d5cf22 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -686,6 +686,7 @@ TARGET_FLAGS_TO_PASS = \
SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
addrmap.c \
auxv.c ax-general.c ax-gdb.c \
+ agent.c \
bcache.c \
bfd-target.c \
block.c blockframe.c breakpoint.c buildsym.c \
@@ -742,7 +743,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
annotate.c common/signals.c copying.c dfp.c gdb.c inf-child.c \
regset.c sol-thread.c windows-termcap.c \
common/common-utils.c common/xml-utils.c \
- common/ptid.c common/buffer.c gdb-dlfcn.c
+ common/ptid.c common/buffer.c gdb-dlfcn.c common/agent.c
LINTFILES = $(SFILES) $(YYFILES) $(CONFIG_SRCS) init.c
@@ -856,6 +857,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
annotate.o \
addrmap.o \
auxv.o \
+ agent.o \
bfd-target.o \
blockframe.o breakpoint.o findvar.o regcache.o \
charset.o continuations.o corelow.o disasm.o dummy-frame.o dfp.o \
@@ -910,7 +912,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
target-descriptions.o target-memory.o xml-tdesc.o xml-builtin.o \
inferior.o osdata.o gdb_usleep.o record.o gcore.o \
jit.o progspace.o skip.o \
- common-utils.o buffer.o ptid.o gdb-dlfcn.o
+ common-utils.o buffer.o ptid.o gdb-dlfcn.o common-agent.o
TSOBS = inflow.o
@@ -1932,6 +1934,10 @@ linux-procfs.o: $(srcdir)/common/linux-procfs.c
$(COMPILE) $(srcdir)/common/linux-procfs.c
$(POSTCOMPILE)
+common-agent.o: $(srcdir)/common/agent.c
+ $(COMPILE) $(srcdir)/common/agent.c
+ $(POSTCOMPILE)
+
#
# gdb/tui/ dependencies
#
diff --git a/gdb/agent.c b/gdb/agent.c
new file mode 100644
index 0000000..67862c5
--- /dev/null
+++ b/gdb/agent.c
@@ -0,0 +1,68 @@
+/* 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/>. */
+
+#include "defs.h"
+#include "command.h"
+#include "gdbcmd.h"
+#include "target.h"
+#include "agent.h"
+
+/* Enum strings for "set|show agent". */
+
+static const char can_use_agent_on[] = "on";
+static const char can_use_agent_off[] = "off";
+static const char *can_use_agent_enum[] =
+{
+ can_use_agent_on,
+ can_use_agent_off,
+ NULL,
+};
+
+static const char *can_use_agent = can_use_agent_off;
+
+static void
+show_can_use_agent (struct ui_file *file, int from_tty,
+ struct cmd_list_element *c, const char *value)
+{
+ fprintf_filtered (file,
+ _("Debugger's willingness to use agent in inferior "
+ "as a helper is %s.\n"), value);
+}
+
+static void
+set_can_use_agent (char *args, int from_tty, struct cmd_list_element *c)
+{
+ if (target_use_agent (can_use_agent == can_use_agent_on))
+ can_use_agent = can_use_agent_on;
+ else
+ can_use_agent = can_use_agent_off;
+}
+
+void
+_initialize_agent (void)
+{
+ add_setshow_enum_cmd ("agent", class_run,
+ can_use_agent_enum,
+ &can_use_agent, _("\
+Set debugger's willingness to use agent as a helper."), _("\
+Show debugger's willingness to use agent as a helper."), _("\
+If on, gdb will use agent as a helper if it is supported by the target.\n\
+If off, gdb will not use agent, even if such is supported by the target\n"),
+ set_can_use_agent,
+ show_can_use_agent,
+ &setlist, &showlist);
+}
--
1.7.0.4
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 3/8] Command `set agent on|off'
2012-01-23 13:54 ` [patch 3/8] Command `set agent on|off' Yao Qi
@ 2012-01-23 17:14 ` Eli Zaretskii
2012-01-24 0:28 ` Yao Qi
2012-02-09 20:19 ` Pedro Alves
1 sibling, 1 reply; 46+ messages in thread
From: Eli Zaretskii @ 2012-01-23 17:14 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
> Date: Mon, 23 Jan 2012 21:50:23 +0800
> From: Yao Qi <yao@codesourcery.com>
>
> We want to leave users to decide whether they want to use agent or not,
> so we add a new command `set agent on|off' in this patch.
Thanks.
> + add_setshow_enum_cmd ("agent", class_run,
> + can_use_agent_enum,
> + &can_use_agent, _("\
> +Set debugger's willingness to use agent as a helper."), _("\
> +Show debugger's willingness to use agent as a helper."), _("\
> +If on, gdb will use agent as a helper if it is supported by the target.\n\
> +If off, gdb will not use agent, even if such is supported by the target\n"),
This doc string should say a little more. In particular, what is the
purpose of using "agent as a helper"? helper to do what?
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 3/8] Command `set agent on|off'
2012-01-23 17:14 ` Eli Zaretskii
@ 2012-01-24 0:28 ` Yao Qi
2012-01-24 5:54 ` Eli Zaretskii
0 siblings, 1 reply; 46+ messages in thread
From: Yao Qi @ 2012-01-24 0:28 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
On 01/24/2012 01:13 AM, Eli Zaretskii wrote:
>> > + add_setshow_enum_cmd ("agent", class_run,
>> > + can_use_agent_enum,
>> > + &can_use_agent, _("\
>> > +Set debugger's willingness to use agent as a helper."), _("\
>> > +Show debugger's willingness to use agent as a helper."), _("\
>> > +If on, gdb will use agent as a helper if it is supported by the target.\n\
>> > +If off, gdb will not use agent, even if such is supported by the target\n"),
> This doc string should say a little more. In particular, what is the
> purpose of using "agent as a helper"? helper to do what?
This question made some troubles when I was writing document. The idea
of agent work is to delegate some debugging works from gdb/gdbserver to
agent, to improve the performance of debugging, because we think some
debugging operations are faster executed in agent than in gdb/gdbserver.
What sort of debugging works agent can do "as a helper" depends on the
agent's capability, so agents can do various things. Current agent
libinproctrace.so is a helper for static tracepoint operations. The new
agent library we are writing is a helper for 1) operating static
tracepoint, 2) installing fast tracepoint, and 3) evaluating breakpoint
conditions.
How about this,
Set debugger's willingness to use agent as a helper."), _("\
Show debugger's willingness to use agent as a helper."), _("\
Agent, a shared library running in the same process of inferior, takes
some delegated debugging works from GDB/GDBserver. Since it is faster
to execute some debugging operations in agent than in GDB/GDBserver, the
performance of debugging can be improved. The delegated debugging works
vary on agents, depend on the actual capability of agents.
If on, gdb will use agent as a helper if it is supported by the target.\n\
If off, gdb will not use agent, even if such is supported by the target\n"),
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 3/8] Command `set agent on|off'
2012-01-24 0:28 ` Yao Qi
@ 2012-01-24 5:54 ` Eli Zaretskii
2012-01-26 1:32 ` Yao Qi
0 siblings, 1 reply; 46+ messages in thread
From: Eli Zaretskii @ 2012-01-24 5:54 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
> Date: Tue, 24 Jan 2012 07:46:46 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <gdb-patches@sourceware.org>
>
> How about this,
>
> Set debugger's willingness to use agent as a helper."), _("\
> Show debugger's willingness to use agent as a helper."), _("\
> Agent, a shared library running in the same process of inferior, takes
> some delegated debugging works from GDB/GDBserver. Since it is faster
> to execute some debugging operations in agent than in GDB/GDBserver, the
> performance of debugging can be improved. The delegated debugging works
> vary on agents, depend on the actual capability of agents.
> If on, gdb will use agent as a helper if it is supported by the target.\n\
> If off, gdb will not use agent, even if such is supported by the target\n"),
Thanks, but I think this goes too far. I suggest this shorter
description:
If ON, GDB will delegate some of the debugging operations to the
agent, if the target supports it. This will speed up those
operations that are supported by the agent.
If OFF GDB will not use agent, even if such is supported by the
target.
Also note that your last sentence lacks a period at its end.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 3/8] Command `set agent on|off'
2012-01-24 5:54 ` Eli Zaretskii
@ 2012-01-26 1:32 ` Yao Qi
0 siblings, 0 replies; 46+ messages in thread
From: Yao Qi @ 2012-01-26 1:32 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 547 bytes --]
On 01/24/2012 01:49 PM, Eli Zaretskii wrote:
> Thanks, but I think this goes too far. I suggest this shorter
> description:
>
> If ON, GDB will delegate some of the debugging operations to the
> agent, if the target supports it. This will speed up those
> operations that are supported by the agent.
> If OFF GDB will not use agent, even if such is supported by the
> target.
>
> Also note that your last sentence lacks a period at its end.
Your description is quite simply and clear. I put it in my patch.
--
Yao (é½å°§)
[-- Attachment #2: 0003-add-agent.c-in-gdb.patch --]
[-- Type: text/x-patch, Size: 4453 bytes --]
2012-01-12 Yao Qi <yao@codesourcery.com>
* Makefile.in (SFILES): Add agent.c and common/agent.c.
(COMMON_OBS): Add agent.o and common/agent.o.
(common-agent.o): New rule.
* agent.c: New.
---
gdb/Makefile.in | 10 ++++++-
gdb/agent.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 79 insertions(+), 2 deletions(-)
create mode 100644 gdb/agent.c
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 25067f1..1d5cf22 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -686,6 +686,7 @@ TARGET_FLAGS_TO_PASS = \
SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
addrmap.c \
auxv.c ax-general.c ax-gdb.c \
+ agent.c \
bcache.c \
bfd-target.c \
block.c blockframe.c breakpoint.c buildsym.c \
@@ -742,7 +743,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \
annotate.c common/signals.c copying.c dfp.c gdb.c inf-child.c \
regset.c sol-thread.c windows-termcap.c \
common/common-utils.c common/xml-utils.c \
- common/ptid.c common/buffer.c gdb-dlfcn.c
+ common/ptid.c common/buffer.c gdb-dlfcn.c common/agent.c
LINTFILES = $(SFILES) $(YYFILES) $(CONFIG_SRCS) init.c
@@ -856,6 +857,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
annotate.o \
addrmap.o \
auxv.o \
+ agent.o \
bfd-target.o \
blockframe.o breakpoint.o findvar.o regcache.o \
charset.o continuations.o corelow.o disasm.o dummy-frame.o dfp.o \
@@ -910,7 +912,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
target-descriptions.o target-memory.o xml-tdesc.o xml-builtin.o \
inferior.o osdata.o gdb_usleep.o record.o gcore.o \
jit.o progspace.o skip.o \
- common-utils.o buffer.o ptid.o gdb-dlfcn.o
+ common-utils.o buffer.o ptid.o gdb-dlfcn.o common-agent.o
TSOBS = inflow.o
@@ -1932,6 +1934,10 @@ linux-procfs.o: $(srcdir)/common/linux-procfs.c
$(COMPILE) $(srcdir)/common/linux-procfs.c
$(POSTCOMPILE)
+common-agent.o: $(srcdir)/common/agent.c
+ $(COMPILE) $(srcdir)/common/agent.c
+ $(POSTCOMPILE)
+
#
# gdb/tui/ dependencies
#
diff --git a/gdb/agent.c b/gdb/agent.c
new file mode 100644
index 0000000..5eda952
--- /dev/null
+++ b/gdb/agent.c
@@ -0,0 +1,71 @@
+/* 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/>. */
+
+#include "defs.h"
+#include "command.h"
+#include "gdbcmd.h"
+#include "target.h"
+#include "agent.h"
+
+/* Enum strings for "set|show agent". */
+
+static const char can_use_agent_on[] = "on";
+static const char can_use_agent_off[] = "off";
+static const char *can_use_agent_enum[] =
+{
+ can_use_agent_on,
+ can_use_agent_off,
+ NULL,
+};
+
+static const char *can_use_agent = can_use_agent_off;
+
+static void
+show_can_use_agent (struct ui_file *file, int from_tty,
+ struct cmd_list_element *c, const char *value)
+{
+ fprintf_filtered (file,
+ _("Debugger's willingness to use agent in inferior "
+ "as a helper is %s.\n"), value);
+}
+
+static void
+set_can_use_agent (char *args, int from_tty, struct cmd_list_element *c)
+{
+ if (target_use_agent (can_use_agent == can_use_agent_on))
+ can_use_agent = can_use_agent_on;
+ else
+ can_use_agent = can_use_agent_off;
+}
+
+void
+_initialize_agent (void)
+{
+ add_setshow_enum_cmd ("agent", class_run,
+ can_use_agent_enum,
+ &can_use_agent, _("\
+Set debugger's willingness to use agent as a helper."), _("\
+Show debugger's willingness to use agent as a helper."), _("\
+If on, GDB will delegate some of the debugging operations to the\n\
+agent, if the target supports it. This will speed up those\n\
+operations that are supported by the agent.\n\
+If off, GDB will not use agent, even if such is supported by the\n\
+target."),
+ set_can_use_agent,
+ show_can_use_agent,
+ &setlist, &showlist);
+}
--
1.7.0.4
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [patch 3/8] Command `set agent on|off'
2012-01-23 13:54 ` [patch 3/8] Command `set agent on|off' Yao Qi
2012-01-23 17:14 ` Eli Zaretskii
@ 2012-02-09 20:19 ` Pedro Alves
1 sibling, 0 replies; 46+ messages in thread
From: Pedro Alves @ 2012-02-09 20:19 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 01/23/2012 01:50 PM, Yao Qi wrote:
> We want to leave users to decide whether they want to use agent or not,
> so we add a new command `set agent on|off' in this patch.
>
Looks fine to me, modulo the consideration for what happens on error,
pointed in the other patch.
--
Pedro Alves
^ permalink raw reply [flat|nested] 46+ messages in thread
* [patch 4/8] `use_agent' for remote and QAgent
2012-01-23 13:37 [patch 0/8] GDB/GDBserver talks with agents Yao Qi
` (2 preceding siblings ...)
2012-01-23 13:54 ` [patch 3/8] Command `set agent on|off' Yao Qi
@ 2012-01-23 13:58 ` Yao Qi
2012-01-23 17:17 ` Eli Zaretskii
2012-02-09 19:55 ` Pedro Alves
2012-01-23 14:03 ` [patch 5/8] Doc for agent Yao Qi
` (5 subsequent siblings)
9 siblings, 2 replies; 46+ messages in thread
From: Yao Qi @ 2012-01-23 13:58 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 164 bytes --]
This patch is to implement the to_use_agent in target_ops in remote
target. A new packet `QAgent' is added to set the flag in remote stub.
--
Yao (é½å°§)
[-- Attachment #2: 0004-impl-of-to_use_agent-for-remote.patch --]
[-- Type: text/x-patch, Size: 3729 bytes --]
gdb:
2012-01-23 Yao Qi <yao@codesourcery.com>
* remote.c (remote_use_agent): New.
(init_remote_ops): Install `remote_use_agent'.
gdb/gdbserver:
2012-01-23 Yao Qi <yao@codesourcery.com>
* server.c (handle_general_set): Handle packet 'QAgent'.
gdb/doc:
2012-01-23 Yao Qi <yao@codesourcery.com>
* gdb.texinfo (General Query Packets): Add packet `QAgent'.
---
gdb/doc/gdb.texinfo | 12 ++++++++++++
gdb/gdbserver/server.c | 26 ++++++++++++++++++++++++++
gdb/remote.c | 18 ++++++++++++++++++
3 files changed, 56 insertions(+), 0 deletions(-)
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 49db189..c4763d4 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -34486,6 +34486,10 @@ Here are the currently defined query and set packets:
@table @samp
+@item QAgent:1
+@item QAgent:0
+Turn on or off agent as a helper.
+
@item QAllow:@var{op}:@var{val}@dots{}
@cindex @samp{QAllow} packet
Specify which operations @value{GDBN} expects to request of the
@@ -35065,6 +35069,11 @@ These are the currently defined stub features and their properties:
@tab @samp{-}
@tab No
+@item @samp{QAgent}
+@tab No
+@tab @samp{-}
+@tab No
+
@item @samp{QAllow}
@tab No
@tab @samp{-}
@@ -35198,6 +35207,9 @@ The remote stub accepts and implements the reverse step packet
The remote stub understands the @samp{QTDPsrc} packet that supplies
the source form of tracepoint definitions.
+@item QAgent
+The remote stub understaqnds the @samp{QAgent} packet.
+
@item QAllow
The remote stub understands the @samp{QAllow} packet.
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index bebccf5..a5e5bbb 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -424,6 +424,8 @@ write_qxfer_response (char *buf, const void *data, int len, int is_more)
PBUFSIZ - 2) + 1;
}
+extern int use_agent;
+
/* Handle all of the extended 'Q' packets. */
static void
@@ -529,6 +531,30 @@ handle_general_set (char *own_buf)
&& handle_tracepoint_general_set (own_buf))
return;
+ if (strncmp ("QAgent:", own_buf, strlen ("QAgent:")) == 0)
+ {
+ char *mode = own_buf + strlen ("QAgent:");
+ int req = 0;
+
+ if (strcmp (mode, "0") == 0)
+ req = 0;
+ else if (strcmp (mode, "1") == 0)
+ req = 1;
+ else
+ {
+ /* We don't know what this value is, so complain to GDB. */
+ fprintf (stderr, "Unknown QAgent value requested: %s\n", own_buf);
+ write_enn (own_buf);
+ return;
+ }
+
+ /* Update the flag. */
+ use_agent = req;
+ if (remote_debug)
+ fprintf (stderr, "[%s agent]\n", req ? "Enable" : "Disable");
+ write_ok (own_buf);
+ }
+
/* Otherwise we didn't know what packet it was. Say we didn't
understand it. */
own_buf[0] = 0;
diff --git a/gdb/remote.c b/gdb/remote.c
index 60d7ecd..50bb90a 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10581,6 +10581,23 @@ remote_set_trace_notes (char *user, char *notes, char *stop_notes)
return 1;
}
+extern int use_agent;
+
+static int
+remote_use_agent (int use)
+{
+ struct remote_state *rs = get_remote_state ();
+
+ sprintf (rs->buf, "QAgent:%d", use);
+ putpkt (rs->buf);
+ getpkt (&rs->buf, &rs->buf_size, 0);
+
+ if (strcmp (rs->buf, "OK") == 0)
+ use_agent = use;
+
+ return use_agent;
+}
+
static void
init_remote_ops (void)
{
@@ -10684,6 +10701,7 @@ Specify the serial device it is connected to\n\
remote_ops.to_static_tracepoint_markers_by_strid
= remote_static_tracepoint_markers_by_strid;
remote_ops.to_traceframe_info = remote_traceframe_info;
+ remote_ops.to_use_agent = remote_use_agent;
}
/* Set up the extended remote vector by making a copy of the standard
--
1.7.0.4
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 4/8] `use_agent' for remote and QAgent
2012-01-23 13:58 ` [patch 4/8] `use_agent' for remote and QAgent Yao Qi
@ 2012-01-23 17:17 ` Eli Zaretskii
2012-01-26 2:17 ` Yao Qi
2012-02-09 19:55 ` Pedro Alves
1 sibling, 1 reply; 46+ messages in thread
From: Eli Zaretskii @ 2012-01-23 17:17 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
> Date: Mon, 23 Jan 2012 21:53:50 +0800
> From: Yao Qi <yao@codesourcery.com>
>
> +@item QAgent:1
> +@item QAgent:0
> +Turn on or off agent as a helper.
Same here: it's not clear what does "helper" mean. At the very least,
there should be a cross-reference to where this is described in the
manual.
Thanks.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [patch 4/8] `use_agent' for remote and QAgent
2012-01-23 17:17 ` Eli Zaretskii
@ 2012-01-26 2:17 ` Yao Qi
2012-01-26 17:43 ` Eli Zaretskii
0 siblings, 1 reply; 46+ messages in thread
From: Yao Qi @ 2012-01-26 2:17 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 293 bytes --]
On 01/24/2012 01:15 AM, Eli Zaretskii wrote:
> Same here: it's not clear what does "helper" mean. At the very least,
> there should be a cross-reference to where this is described in the
> manual.
I add some words here, and a cross-reference to node "Control Agent".
--
Yao (é½å°§)
[-- Attachment #2: 0004-impl-of-to_use_agent-for-remote.patch --]
[-- Type: text/x-patch, Size: 3825 bytes --]
gdb:
2012-01-25 Yao Qi <yao@codesourcery.com>
* remote.c (remote_use_agent): New.
(init_remote_ops): Install `remote_use_agent'.
gdb/gdbserver:
2012-01-25 Yao Qi <yao@codesourcery.com>
* server.c (handle_general_set): Handle packet 'QAgent'.
gdb/doc:
2012-01-25 Yao Qi <yao@codesourcery.com>
* gdb.texinfo (General Query Packets): Add packet `QAgent'.
---
gdb/doc/gdb.texinfo | 13 +++++++++++++
gdb/gdbserver/server.c | 26 ++++++++++++++++++++++++++
gdb/remote.c | 18 ++++++++++++++++++
3 files changed, 57 insertions(+), 0 deletions(-)
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 49db189..3cd8398 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -34486,6 +34486,11 @@ Here are the currently defined query and set packets:
@table @samp
+@item QAgent:1
+@item QAgent:0
+Turn on or off the agent as a helper to perform some debugging operations
+delegated from @value{GDBN}. (@pxref{Control Agent})
+
@item QAllow:@var{op}:@var{val}@dots{}
@cindex @samp{QAllow} packet
Specify which operations @value{GDBN} expects to request of the
@@ -35065,6 +35070,11 @@ These are the currently defined stub features and their properties:
@tab @samp{-}
@tab No
+@item @samp{QAgent}
+@tab No
+@tab @samp{-}
+@tab No
+
@item @samp{QAllow}
@tab No
@tab @samp{-}
@@ -35198,6 +35208,9 @@ The remote stub accepts and implements the reverse step packet
The remote stub understands the @samp{QTDPsrc} packet that supplies
the source form of tracepoint definitions.
+@item QAgent
+The remote stub understaqnds the @samp{QAgent} packet.
+
@item QAllow
The remote stub understands the @samp{QAllow} packet.
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index bebccf5..a5e5bbb 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -424,6 +424,8 @@ write_qxfer_response (char *buf, const void *data, int len, int is_more)
PBUFSIZ - 2) + 1;
}
+extern int use_agent;
+
/* Handle all of the extended 'Q' packets. */
static void
@@ -529,6 +531,30 @@ handle_general_set (char *own_buf)
&& handle_tracepoint_general_set (own_buf))
return;
+ if (strncmp ("QAgent:", own_buf, strlen ("QAgent:")) == 0)
+ {
+ char *mode = own_buf + strlen ("QAgent:");
+ int req = 0;
+
+ if (strcmp (mode, "0") == 0)
+ req = 0;
+ else if (strcmp (mode, "1") == 0)
+ req = 1;
+ else
+ {
+ /* We don't know what this value is, so complain to GDB. */
+ fprintf (stderr, "Unknown QAgent value requested: %s\n", own_buf);
+ write_enn (own_buf);
+ return;
+ }
+
+ /* Update the flag. */
+ use_agent = req;
+ if (remote_debug)
+ fprintf (stderr, "[%s agent]\n", req ? "Enable" : "Disable");
+ write_ok (own_buf);
+ }
+
/* Otherwise we didn't know what packet it was. Say we didn't
understand it. */
own_buf[0] = 0;
diff --git a/gdb/remote.c b/gdb/remote.c
index 60d7ecd..50bb90a 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10581,6 +10581,23 @@ remote_set_trace_notes (char *user, char *notes, char *stop_notes)
return 1;
}
+extern int use_agent;
+
+static int
+remote_use_agent (int use)
+{
+ struct remote_state *rs = get_remote_state ();
+
+ sprintf (rs->buf, "QAgent:%d", use);
+ putpkt (rs->buf);
+ getpkt (&rs->buf, &rs->buf_size, 0);
+
+ if (strcmp (rs->buf, "OK") == 0)
+ use_agent = use;
+
+ return use_agent;
+}
+
static void
init_remote_ops (void)
{
@@ -10684,6 +10701,7 @@ Specify the serial device it is connected to\n\
remote_ops.to_static_tracepoint_markers_by_strid
= remote_static_tracepoint_markers_by_strid;
remote_ops.to_traceframe_info = remote_traceframe_info;
+ remote_ops.to_use_agent = remote_use_agent;
}
/* Set up the extended remote vector by making a copy of the standard
--
1.7.0.4
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 4/8] `use_agent' for remote and QAgent
2012-01-26 2:17 ` Yao Qi
@ 2012-01-26 17:43 ` Eli Zaretskii
0 siblings, 0 replies; 46+ messages in thread
From: Eli Zaretskii @ 2012-01-26 17:43 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
> Date: Thu, 26 Jan 2012 09:53:26 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <gdb-patches@sourceware.org>
>
> On 01/24/2012 01:15 AM, Eli Zaretskii wrote:
> > Same here: it's not clear what does "helper" mean. At the very least,
> > there should be a cross-reference to where this is described in the
> > manual.
>
> I add some words here, and a cross-reference to node "Control Agent".
Thanks.
> +Turn on or off the agent as a helper to perform some debugging operations
> +delegated from @value{GDBN}. (@pxref{Control Agent})
Please move the parentheses with @pxref before the period:
delegated from @value{GDBN} (@pxref{Control Agent}).
> +The remote stub understaqnds the @samp{QAgent} packet.
^^^^^^^^^^^^
A typo.
OK with these changes.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [patch 4/8] `use_agent' for remote and QAgent
2012-01-23 13:58 ` [patch 4/8] `use_agent' for remote and QAgent Yao Qi
2012-01-23 17:17 ` Eli Zaretskii
@ 2012-02-09 19:55 ` Pedro Alves
1 sibling, 0 replies; 46+ messages in thread
From: Pedro Alves @ 2012-02-09 19:55 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 01/23/2012 01:53 PM, Yao Qi wrote:
> This patch is to implement the to_use_agent in target_ops in remote
> target. A new packet `QAgent' is added to set the flag in remote stub.
>
> -- Yao (é½å°§)
>
>
> 0004-impl-of-to_use_agent-for-remote.patch
>
>
> gdb:
>
> 2012-01-23 Yao Qi <yao@codesourcery.com>
>
> * remote.c (remote_use_agent): New.
> (init_remote_ops): Install `remote_use_agent'.
>
> gdb/gdbserver:
>
> 2012-01-23 Yao Qi <yao@codesourcery.com>
>
> * server.c (handle_general_set): Handle packet 'QAgent'.
>
> gdb/doc:
>
> 2012-01-23 Yao Qi <yao@codesourcery.com>
>
> * gdb.texinfo (General Query Packets): Add packet `QAgent'.
> ---
> gdb/doc/gdb.texinfo | 12 ++++++++++++
> gdb/gdbserver/server.c | 26 ++++++++++++++++++++++++++
> gdb/remote.c | 18 ++++++++++++++++++
> 3 files changed, 56 insertions(+), 0 deletions(-)
>
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 49db189..c4763d4 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -34486,6 +34486,10 @@ Here are the currently defined query and set packets:
>
> @table @samp
>
> +@item QAgent:1
> +@item QAgent:0
> +Turn on or off agent as a helper.
> +
> @item QAllow:@var{op}:@var{val}@dots{}
> @cindex @samp{QAllow} packet
> Specify which operations @value{GDBN} expects to request of the
> @@ -35065,6 +35069,11 @@ These are the currently defined stub features and their properties:
> @tab @samp{-}
> @tab No
>
> +@item @samp{QAgent}
> +@tab No
> +@tab @samp{-}
> +@tab No
> +
> @item @samp{QAllow}
> @tab No
> @tab @samp{-}
> @@ -35198,6 +35207,9 @@ The remote stub accepts and implements the reverse step packet
> The remote stub understands the @samp{QTDPsrc} packet that supplies
> the source form of tracepoint definitions.
>
> +@item QAgent
> +The remote stub understaqnds the @samp{QAgent} packet.
> +
> @item QAllow
> The remote stub understands the @samp{QAllow} packet.
>
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index bebccf5..a5e5bbb 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -424,6 +424,8 @@ write_qxfer_response (char *buf, const void *data, int len, int is_more)
> PBUFSIZ - 2) + 1;
> }
>
> +extern int use_agent;
Wasn't this declared in some header?
> +
> /* Handle all of the extended 'Q' packets. */
>
> static void
> @@ -529,6 +531,30 @@ handle_general_set (char *own_buf)
> && handle_tracepoint_general_set (own_buf))
> return;
>
> + if (strncmp ("QAgent:", own_buf, strlen ("QAgent:")) == 0)
> + {
> + char *mode = own_buf + strlen ("QAgent:");
> + int req = 0;
> +
> + if (strcmp (mode, "0") == 0)
> + req = 0;
> + else if (strcmp (mode, "1") == 0)
> + req = 1;
> + else
> + {
> + /* We don't know what this value is, so complain to GDB. */
> + fprintf (stderr, "Unknown QAgent value requested: %s\n", own_buf);
> + write_enn (own_buf);
You can't send the error message back to gdb, with the E.foo form.
WDYT?
> + return;
> + }
> +
> + /* Update the flag. */
> + use_agent = req;
> + if (remote_debug)
> + fprintf (stderr, "[%s agent]\n", req ? "Enable" : "Disable");
> + write_ok (own_buf);
> + }
> +
> /* Otherwise we didn't know what packet it was. Say we didn't
> understand it. */
> own_buf[0] = 0;
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 60d7ecd..50bb90a 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -10581,6 +10581,23 @@ remote_set_trace_notes (char *user, char *notes, char *stop_notes)
> return 1;
> }
>
> +extern int use_agent;
Likewise.
> +
> +static int
> +remote_use_agent (int use)
> +{
> + struct remote_state *rs = get_remote_state ();
> +
> + sprintf (rs->buf, "QAgent:%d", use);
> + putpkt (rs->buf);
> + getpkt (&rs->buf, &rs->buf_size, 0);
> +
> + if (strcmp (rs->buf, "OK") == 0)
> + use_agent = use;
> +
> + return use_agent;
> +}
So what is supposed to happen if you do "set agent on" and the remote
errors, or doesn't support the packet? At least errors should be handled.
Please consider what happens to the command's setting in those cases
as well.
> +
> static void
> init_remote_ops (void)
> {
> @@ -10684,6 +10701,7 @@ Specify the serial device it is connected to\n\
> remote_ops.to_static_tracepoint_markers_by_strid
> = remote_static_tracepoint_markers_by_strid;
> remote_ops.to_traceframe_info = remote_traceframe_info;
> + remote_ops.to_use_agent = remote_use_agent;
> }
>
> /* Set up the extended remote vector by making a copy of the standard
> -- 1.7.0.4
--
Pedro Alves
^ permalink raw reply [flat|nested] 46+ messages in thread
* [patch 5/8] Doc for agent
2012-01-23 13:37 [patch 0/8] GDB/GDBserver talks with agents Yao Qi
` (3 preceding siblings ...)
2012-01-23 13:58 ` [patch 4/8] `use_agent' for remote and QAgent Yao Qi
@ 2012-01-23 14:03 ` Yao Qi
2012-01-23 18:12 ` Eli Zaretskii
2012-02-09 19:55 ` Pedro Alves
2012-01-23 14:07 ` [patch 6/8] Agent's capability Yao Qi
` (4 subsequent siblings)
9 siblings, 2 replies; 46+ messages in thread
From: Yao Qi @ 2012-01-23 14:03 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 251 bytes --]
In this patch, we create a new chapter for agent, and put "Agent
Expression" under this chapter. One section "Control Agent" is added to
document command in patch 3/8, and the rest of them is from original
"Agent Expression".
--
Yao (é½å°§)
[-- Attachment #2: 0005-agent-doc.patch --]
[-- Type: text/x-patch, Size: 9114 bytes --]
gdb/doc/
2012-01-21 Yao Qi <yao@codesourcery.com>
* agentexpr.texi: Re-structure doc for agent.
* gdb.texinfo: Move chapeter `Agent Expression' to `Agent'.
---
gdb/doc/agentexpr.texi | 159 ++++++++++++++++++++++++++++--------------------
gdb/doc/gdb.texinfo | 2 +-
2 files changed, 94 insertions(+), 67 deletions(-)
diff --git a/gdb/doc/agentexpr.texi b/gdb/doc/agentexpr.texi
index d0f6f15..e06ab20 100644
--- a/gdb/doc/agentexpr.texi
+++ b/gdb/doc/agentexpr.texi
@@ -11,34 +11,43 @@
@c
@c See the file gdb.texinfo for copying conditions.
+@node Agent
+@chapter Debugging Agent
+The traditional debugging model is conceptually low-speed, but works fine,
+because most of bugs can be reproduced in low-speed execution. However,
+as multi-core or many-core processor is becoming mainstream, and
+multi-threaded program becomes more and more popular, there should be more
+and more bugs that only manifest themselves at full speed, for example, thread
+races. Therefore, traditional debugging model is too intrusive to reproduce
+the bug. In order to overcome the interference, we should reduce the number of
+operations debugger performed. @dfn{Agent}, a shared library, is running within
+the same process with inferior, and is able to perform some debugging operations
+itself. As a result, debugger is only involved when necessary, and performance
+of debugging can be improved accordingly.
+
+
+@menu
+* Agent Expressions:: The GDB Agent Expression Mechanism
+* Control Agent:: Turn agent on and off
+* Varying Target Capabilities:: How to discover what the target can do
+@end menu
+
@node Agent Expressions
-@appendix The GDB Agent Expression Mechanism
-
-In some applications, it is not feasible for the debugger to interrupt
-the program's execution long enough for the developer to learn anything
-helpful about its behavior. If the program's correctness depends on its
-real-time behavior, delays introduced by a debugger might cause the
-program to fail, even when the code itself is correct. It is useful to
-be able to observe the program's behavior without interrupting it.
-
-Using GDB's @code{trace} and @code{collect} commands, the user can
-specify locations in the program, and arbitrary expressions to evaluate
-when those locations are reached. Later, using the @code{tfind}
-command, she can examine the values those expressions had when the
-program hit the trace points. The expressions may also denote objects
-in memory --- structures or arrays, for example --- whose values GDB
-should record; while visiting a particular tracepoint, the user may
-inspect those objects as if they were in memory at that moment.
-However, because GDB records these values without interacting with the
-user, it can do so quickly and unobtrusively, hopefully not disturbing
-the program's behavior.
-
-When GDB is debugging a remote target, the GDB @dfn{agent} code running
+@section The GDB Agent Expression Mechanism
+
+When using agent with @value{GDBN}, expression in agent will be used in many cases,
+such as the expressions used in tracepoint for data collection, and expressions used
+in breakpoint condition evaluation. The expressions may also denote registers and
+objects in memory --- structures or arrays, for example --- whose values @value{GDBN}
+should record.
+
+When GDB is debugging a target, the GDB @dfn{agent} code running
on the target computes the values of the expressions itself. To avoid
having a full symbolic expression evaluator on the agent, GDB translates
expressions in the source language into a simpler bytecode language, and
then sends the bytecode to the agent; the agent then executes the
-bytecode, and records the values for GDB to retrieve later.
+bytecode, and records the values for GDB to retrieve later. We call these bytecode
+@dfn{Agent Expression}.
The bytecode language is simple; there are forty-odd opcodes, the bulk
of which are the usual vocabulary of C operands (addition, subtraction,
@@ -56,7 +65,6 @@ debugging agent in real-time applications.
* General Bytecode Design:: Overview of the interpreter.
* Bytecode Descriptions:: What each one does.
* Using Agent Expressions:: How agent expressions fit into the big picture.
-* Varying Target Capabilities:: How to discover what the target can do.
* Rationale:: Why we did it this way.
@end menu
@@ -66,7 +74,7 @@ debugging agent in real-time applications.
@node General Bytecode Design
-@section General Bytecode Design
+@subsection General Bytecode Design
The agent represents bytecode expressions as an array of bytes. Each
instruction is one byte long (thus the term @dfn{bytecode}). Some
@@ -201,7 +209,7 @@ recorded.
@node Bytecode Descriptions
-@section Bytecode Descriptions
+@subsection Bytecode Descriptions
Each bytecode description has the following form:
@@ -503,7 +511,7 @@ address, and the top of the stack is the lvalue's size, in bytes.
@node Using Agent Expressions
-@section Using Agent Expressions
+@subsection Using Agent Expressions
Agent expressions can be used in several different ways by @value{GDBN},
and the debugger can generate different bytecode sequences as appropriate.
@@ -553,46 +561,8 @@ reports an error.
@end itemize
-
-@node Varying Target Capabilities
-@section Varying Target Capabilities
-
-Some targets don't support floating-point, and some would rather not
-have to deal with @code{long long} operations. Also, different targets
-will have different stack sizes, and different bytecode buffer lengths.
-
-Thus, GDB needs a way to ask the target about itself. We haven't worked
-out the details yet, but in general, GDB should be able to send the
-target a packet asking it to describe itself. The reply should be a
-packet whose length is explicit, so we can add new information to the
-packet in future revisions of the agent, without confusing old versions
-of GDB, and it should contain a version number. It should contain at
-least the following information:
-
-@itemize @bullet
-
-@item
-whether floating point is supported
-
-@item
-whether @code{long long} is supported
-
-@item
-maximum acceptable size of bytecode stack
-
-@item
-maximum acceptable length of bytecode expressions
-
-@item
-which registers are actually available for collection
-
-@item
-whether the target supports disabled tracepoints
-
-@end itemize
-
@node Rationale
-@section Rationale
+@subsection Rationale
Some of the design decisions apparent above are arguable.
@@ -748,3 +718,60 @@ opcode 0x30 reserved, to remain compatible with the customer who added
it.
@end table
+
+@node Control Agent
+@section Turn agent on and off
+
+As a helper of debugging, agent should be turned on and off by user, which
+can be done with the following commands:
+
+@table @code
+@item set agent on
+Causes agent may execute some debugging operations, which is determined by
+actual debugging request from users and by the capability of agent. For
+example, if user request to evaluate breakpoint conditions in agent, and
+agent has such capability as well, then breakpoint conditions will be
+evaluated in agent.
+
+@item set agent off
+Causes agent not execute any debugging operations. All of them should be
+performed in @value{GDBN}.
+
+@end table
+
+@node Varying Target Capabilities
+@section Varying Target Capabilities
+
+Some targets don't support floating-point, and some would rather not
+have to deal with @code{long long} operations. Also, different targets
+will have different stack sizes, and different bytecode buffer lengths.
+
+Thus, GDB needs a way to ask the target about itself. We haven't worked
+out the details yet, but in general, GDB should be able to send the
+target a packet asking it to describe itself. The reply should be a
+packet whose length is explicit, so we can add new information to the
+packet in future revisions of the agent, without confusing old versions
+of GDB, and it should contain a version number. It should contain at
+least the following information:
+
+@itemize @bullet
+
+@item
+whether floating point is supported
+
+@item
+whether @code{long long} is supported
+
+@item
+maximum acceptable size of bytecode stack
+
+@item
+maximum acceptable length of bytecode expressions
+
+@item
+which registers are actually available for collection
+
+@item
+whether the target supports disabled tracepoints
+
+@end itemize
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index c4763d4..810dc3c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -170,7 +170,7 @@ software in general. We will miss him.
* Installing GDB:: Installing GDB
* Maintenance Commands:: Maintenance Commands
* Remote Protocol:: GDB Remote Serial Protocol
-* Agent Expressions:: The GDB Agent Expression Mechanism
+* Agent:: Debugging Agent
* Target Descriptions:: How targets can describe themselves to
@value{GDBN}
* Operating System Information:: Getting additional information from
--
1.7.0.4
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 5/8] Doc for agent
2012-01-23 14:03 ` [patch 5/8] Doc for agent Yao Qi
@ 2012-01-23 18:12 ` Eli Zaretskii
2012-01-24 0:51 ` Yao Qi
2012-02-09 19:55 ` Pedro Alves
1 sibling, 1 reply; 46+ messages in thread
From: Eli Zaretskii @ 2012-01-23 18:12 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
> Date: Mon, 23 Jan 2012 21:58:35 +0800
> From: Yao Qi <yao@codesourcery.com>
>
> In this patch, we create a new chapter for agent, and put "Agent
> Expression" under this chapter. One section "Control Agent" is added to
> document command in patch 3/8, and the rest of them is from original
> "Agent Expression".
Thanks.
> +@node Agent
> +@chapter Debugging Agent
Please add here
@cindex debugging agent
> +The traditional debugging model is conceptually low-speed, but works fine,
> +because most of bugs can be reproduced in low-speed execution. However,
^^^^^^^^^^^^
"most bugs", without "of".
> +as multi-core or many-core processor is becoming mainstream, and
^^^^^^^^^^^^
"processors are"
> +multi-threaded program becomes more and more popular, there should be more
^^^^^^^^^^^^^^^
"programs become"
> +and more bugs that only manifest themselves at full speed, for example, thread
> +races.
I don't really agree that the issue here is "low speed" vs "full
speed". Rather, the issue is that the timeline of the running program
is modified by the debugger, when it stops some or all of the threads,
single-steps some of them, etc. Disrupting the original timeline can
completely modify the behavior and conceal bugs that depend on timing.
> +the bug. In order to overcome the interference, we should reduce the number of
> +operations debugger performed.
^^^^^^^^^^^^^^^^^^
"preformed by the debugger".
Again, I think the issue is not the number of operations, but
interference with the program's timing.
> @dfn{Agent}, a shared library, is running within
> +the same process with inferior, and is able to perform some debugging operations
> +itself. As a result, debugger is only involved when necessary, and performance
> +of debugging can be improved accordingly.
But this is a half-measure, isn't it? The agent will still stop or
slow down the program's threads, and so still influences its timeline,
right?
> +@menu
> +* Agent Expressions:: The GDB Agent Expression Mechanism
> +* Control Agent:: Turn agent on and off
> +* Varying Target Capabilities:: How to discover what the target can do
Please don't use TAB characters in Texinfo sources. (If you use
Emacs, it will take care of that for you; if not, please untabify the
Texinfo sources after editing.) Using TABs in Texinfo can produce
misaligned text in the manual.
> -In some applications, it is not feasible for the debugger to interrupt
> -the program's execution long enough for the developer to learn anything
> -helpful about its behavior. If the program's correctness depends on its
> -real-time behavior, delays introduced by a debugger might cause the
> -program to fail, even when the code itself is correct. It is useful to
> -be able to observe the program's behavior without interrupting it.
See, this text describes the issue more correctly, IMO, but it looks
like you deleted it and not moved it to the new chapter.
> +When using agent with @value{GDBN}, expression in agent will be used in many cases,
^^^^^ ^^^^^^^^^^
"the agent", and "expressions", in plural.
> +such as the expressions used in tracepoint for data collection, and expressions used
^^^^^^^^^^
"tracepoints", in plural.
> +in breakpoint condition evaluation. The expressions may also denote registers and
> +objects in memory --- structures or arrays, for example --- whose values @value{GDBN}
No blanks around "---", please.
> +When GDB is debugging a target, the GDB @dfn{agent} code running
> on the target computes the values of the expressions itself. To avoid
> having a full symbolic expression evaluator on the agent, GDB translates
> expressions in the source language into a simpler bytecode language, and
> then sends the bytecode to the agent; the agent then executes the
> -bytecode, and records the values for GDB to retrieve later.
> +bytecode, and records the values for GDB to retrieve later. We call these bytecode
> +@dfn{Agent Expression}.
@value{GDBN} instead of "GDB".
> +@node Control Agent
> +@section Turn agent on and off
@cindex here. And please capitalize the words in the section name.
> +As a helper of debugging, agent should be turned on and off by user, which
> +can be done with the following commands:
Suggest to reword:
You can control whether the agent is used as an aid for debugging
with the following commands:
> +@table @code
> +@item set agent on
> +Causes agent may execute some debugging operations, which is determined by
> +actual debugging request from users and by the capability of agent. For
> +example, if user request to evaluate breakpoint conditions in agent, and
> +agent has such capability as well, then breakpoint conditions will be
> +evaluated in agent.
Causes the agent to perform some operations on behalf of the
debugger. Just which operations requested by the user will be done
by the agent depends on the agent's capabilities.
> +@item set agent off
> +Causes agent not execute any debugging operations. All of them should be
> +performed in @value{GDBN}.
Disables execution of debugging operations by the agent. All of the
operations will be performed by @value{GDBN}.
> +Thus, GDB needs a way to ask the target about itself. We haven't worked
> +out the details yet, but in general, GDB should be able to send the
> +target a packet asking it to describe itself. The reply should be a
> +packet whose length is explicit, so we can add new information to the
> +packet in future revisions of the agent, without confusing old versions
> +of GDB, and it should contain a version number. It should contain at
> +least the following information:
@value{GDBN}, please.
OK with these changes.
Thanks.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 5/8] Doc for agent
2012-01-23 18:12 ` Eli Zaretskii
@ 2012-01-24 0:51 ` Yao Qi
2012-01-24 8:04 ` Eli Zaretskii
0 siblings, 1 reply; 46+ messages in thread
From: Yao Qi @ 2012-01-24 0:51 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
On 01/24/2012 01:35 AM, Eli Zaretskii wrote:
Eli,
Thanks for the review.
>
>> +multi-threaded program becomes more and more popular, there should be more
> ^^^^^^^^^^^^^^^
> "programs become"
>
>> +and more bugs that only manifest themselves at full speed, for example, thread
>> +races.
>
> I don't really agree that the issue here is "low speed" vs "full
> speed". Rather, the issue is that the timeline of the running program
> is modified by the debugger, when it stops some or all of the threads,
> single-steps some of them, etc. Disrupting the original timeline can
> completely modify the behavior and conceal bugs that depend on timing.
>
Agreed.
>> +the bug. In order to overcome the interference, we should reduce the number of
>> +operations debugger performed.
> ^^^^^^^^^^^^^^^^^^
> "preformed by the debugger".
>
> Again, I think the issue is not the number of operations, but
> interference with the program's timing.
>
Yes, the issue is the interference with the program's timing. We want
to fix this issue, to some extent, by reducing the number of operations
here. Of course, there are other approaches to reduce interference.
How about change this sentence like this,
"In order to reduce the interference with the program, we can reduce the
number of operations performed by the debugger."
>> @dfn{Agent}, a shared library, is running within
>> +the same process with inferior, and is able to perform some debugging operations
>> +itself. As a result, debugger is only involved when necessary, and performance
>> +of debugging can be improved accordingly.
>
> But this is a half-measure, isn't it? The agent will still stop or
> slow down the program's threads, and so still influences its timeline,
> right?
>
Right. Interference can be reduced, but can't be removed completely.
>> -In some applications, it is not feasible for the debugger to interrupt
>> -the program's execution long enough for the developer to learn anything
>> -helpful about its behavior. If the program's correctness depends on its
>> -real-time behavior, delays introduced by a debugger might cause the
>> -program to fail, even when the code itself is correct. It is useful to
>> -be able to observe the program's behavior without interrupting it.
>
> See, this text describes the issue more correctly, IMO, but it looks
> like you deleted it and not moved it to the new chapter.
I remove this paragraph because it fits to the concept of tracepoint
("observer the program's behavior without interrupting it"), and the
agent work can do more than that. I don't mind putting it back at the
the beginning of chapter "Debugging Agent" as another problem statement.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 5/8] Doc for agent
2012-01-24 0:51 ` Yao Qi
@ 2012-01-24 8:04 ` Eli Zaretskii
2012-01-26 1:53 ` Yao Qi
0 siblings, 1 reply; 46+ messages in thread
From: Eli Zaretskii @ 2012-01-24 8:04 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
> Date: Tue, 24 Jan 2012 08:33:37 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <gdb-patches@sourceware.org>
>
> >> +the bug. In order to overcome the interference, we should reduce the number of
> >> +operations debugger performed.
> > ^^^^^^^^^^^^^^^^^^
> > "preformed by the debugger".
> >
> > Again, I think the issue is not the number of operations, but
> > interference with the program's timing.
> >
>
> Yes, the issue is the interference with the program's timing. We want
> to fix this issue, to some extent, by reducing the number of operations
> here. Of course, there are other approaches to reduce interference.
>
> How about change this sentence like this,
>
> "In order to reduce the interference with the program, we can reduce the
> number of operations performed by the debugger."
OK.
>
> >> @dfn{Agent}, a shared library, is running within
> >> +the same process with inferior, and is able to perform some debugging operations
> >> +itself. As a result, debugger is only involved when necessary, and performance
> >> +of debugging can be improved accordingly.
> >
> > But this is a half-measure, isn't it? The agent will still stop or
> > slow down the program's threads, and so still influences its timeline,
> > right?
> >
>
> Right. Interference can be reduced, but can't be removed completely.
It might be a good idea to say that. Otherwise, the wording sounds as
if using the agent solves this problem completely.
> >> -In some applications, it is not feasible for the debugger to interrupt
> >> -the program's execution long enough for the developer to learn anything
> >> -helpful about its behavior. If the program's correctness depends on its
> >> -real-time behavior, delays introduced by a debugger might cause the
> >> -program to fail, even when the code itself is correct. It is useful to
> >> -be able to observe the program's behavior without interrupting it.
> >
> > See, this text describes the issue more correctly, IMO, but it looks
> > like you deleted it and not moved it to the new chapter.
>
> I remove this paragraph because it fits to the concept of tracepoint
> ("observer the program's behavior without interrupting it"), and the
> agent work can do more than that.
What can it do in addition? I don't think your new text said anything
about that.
> I don't mind putting it back at the
> the beginning of chapter "Debugging Agent" as another problem statement.
I think it should be the only statement of the problem. If the agent
has additional benefits, their description should be added to the
above text.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 5/8] Doc for agent
2012-01-24 8:04 ` Eli Zaretskii
@ 2012-01-26 1:53 ` Yao Qi
2012-01-26 17:15 ` Eli Zaretskii
0 siblings, 1 reply; 46+ messages in thread
From: Yao Qi @ 2012-01-26 1:53 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]
On 01/24/2012 01:54 PM, Eli Zaretskii wrote:
>> > I remove this paragraph because it fits to the concept of tracepoint
>> > ("observer the program's behavior without interrupting it"), and the
>> > agent work can do more than that.
> What can it do in addition? I don't think your new text said anything
> about that.
>
>> > I don't mind putting it back at the
>> > the beginning of chapter "Debugging Agent" as another problem statement.
> I think it should be the only statement of the problem. If the agent
> has additional benefits, their description should be added to the
> above text.
The agent has only one benefit so far, that is, reducing interference
with program during debugging. However, I gave two problems at the
beginning, one is that program is wrong, but interference prevent
reproducing it, for example, races in threads. The other is that
program is right, but interference changes its behavior and program
fails finally. These two problems can be treated as a single problem,
from some point of view. My previous reply might be misleading.
Here is a new doc patch.
--
Yao (é½å°§)
[-- Attachment #2: 0005-agent-doc.patch --]
[-- Type: text/x-patch, Size: 10034 bytes --]
gdb/doc/
2012-01-26 Yao Qi <yao@codesourcery.com>
* agentexpr.texi: Re-structure doc for agent.
* gdb.texinfo: Move chapeter `Agent Expression' to `Agent'.
---
gdb/doc/agentexpr.texi | 173 +++++++++++++++++++++++++++++-------------------
gdb/doc/gdb.texinfo | 2 +-
2 files changed, 107 insertions(+), 68 deletions(-)
diff --git a/gdb/doc/agentexpr.texi b/gdb/doc/agentexpr.texi
index d0f6f15..0cb0f1b 100644
--- a/gdb/doc/agentexpr.texi
+++ b/gdb/doc/agentexpr.texi
@@ -11,34 +11,55 @@
@c
@c See the file gdb.texinfo for copying conditions.
+@node Agent
+@chapter Debugging Agent
+@cindex debugging agent
+The traditional debugging model is conceptually low-speed, but works fine,
+because most bugs can be reproduced in debugging-mode execution. However,
+as multi-core or many-core processors are becoming mainstream, and
+multi-threaded programs become more and more popular, there should be more
+and more bugs that only manifest themselves at normal-mode execution, for
+example, thread races, because debugger's interference with the program's
+timing may conceal the bugs. On the other hand, in some applications,
+it is not feasible for the debugger to interrupt the program's execution
+long enough for the developer to learn anything helpful about its behavior.
+If the program's correctness depends on its real-time behavior, delays
+introduced by a debugger might cause the program to fail, even when the
+code itself is correct. It is useful to be able to observe the program's
+behavior without interrupting it.
+
+Therefore, traditional debugging model is too intrusive to reproduce
+the bug. In order to reduce the interference with the program, we can
+reduce the number of operations performed by debugger. @dfn{Agent},
+a shared library, is running within the same process with inferior, and is
+able to perform some debugging operations itself. As a result, debugger
+is only involved when necessary, and performance of debugging can be
+improved accordingly. Note that interference with program can be
+reduced but can't be removed completely, because the agent will still stop
+or slow down the program.
+
+@menu
+* Agent Expressions:: The GDB Agent Expression Mechanism
+* Control Agent:: Turn agent on and off
+* Varying Target Capabilities:: How to discover what the target can do
+@end menu
+
@node Agent Expressions
-@appendix The GDB Agent Expression Mechanism
-
-In some applications, it is not feasible for the debugger to interrupt
-the program's execution long enough for the developer to learn anything
-helpful about its behavior. If the program's correctness depends on its
-real-time behavior, delays introduced by a debugger might cause the
-program to fail, even when the code itself is correct. It is useful to
-be able to observe the program's behavior without interrupting it.
-
-Using GDB's @code{trace} and @code{collect} commands, the user can
-specify locations in the program, and arbitrary expressions to evaluate
-when those locations are reached. Later, using the @code{tfind}
-command, she can examine the values those expressions had when the
-program hit the trace points. The expressions may also denote objects
-in memory --- structures or arrays, for example --- whose values GDB
-should record; while visiting a particular tracepoint, the user may
-inspect those objects as if they were in memory at that moment.
-However, because GDB records these values without interacting with the
-user, it can do so quickly and unobtrusively, hopefully not disturbing
-the program's behavior.
-
-When GDB is debugging a remote target, the GDB @dfn{agent} code running
+@section The GDB Agent Expression Mechanism
+
+When using the agent with @value{GDBN}, expressions in agent will be used in many cases,
+such as the expressions used in tracepoints for data collection, and expressions used
+in breakpoint condition evaluation. The expressions may also denote registers and
+objects in memory---structures or arrays, for example---whose values @value{GDBN}
+should record.
+
+When @value{GDBN} is debugging a target, the @value{GDBN} @dfn{agent} code running
on the target computes the values of the expressions itself. To avoid
-having a full symbolic expression evaluator on the agent, GDB translates
+having a full symbolic expression evaluator on the agent, @value{GDBN} translates
expressions in the source language into a simpler bytecode language, and
then sends the bytecode to the agent; the agent then executes the
-bytecode, and records the values for GDB to retrieve later.
+bytecode, and records the values for @value{GDBN} to retrieve later. We call these
+bytecode @dfn{Agent Expression}.
The bytecode language is simple; there are forty-odd opcodes, the bulk
of which are the usual vocabulary of C operands (addition, subtraction,
@@ -56,7 +77,6 @@ debugging agent in real-time applications.
* General Bytecode Design:: Overview of the interpreter.
* Bytecode Descriptions:: What each one does.
* Using Agent Expressions:: How agent expressions fit into the big picture.
-* Varying Target Capabilities:: How to discover what the target can do.
* Rationale:: Why we did it this way.
@end menu
@@ -66,7 +86,7 @@ debugging agent in real-time applications.
@node General Bytecode Design
-@section General Bytecode Design
+@subsection General Bytecode Design
The agent represents bytecode expressions as an array of bytes. Each
instruction is one byte long (thus the term @dfn{bytecode}). Some
@@ -201,7 +221,7 @@ recorded.
@node Bytecode Descriptions
-@section Bytecode Descriptions
+@subsection Bytecode Descriptions
Each bytecode description has the following form:
@@ -503,7 +523,7 @@ address, and the top of the stack is the lvalue's size, in bytes.
@node Using Agent Expressions
-@section Using Agent Expressions
+@subsection Using Agent Expressions
Agent expressions can be used in several different ways by @value{GDBN},
and the debugger can generate different bytecode sequences as appropriate.
@@ -553,46 +573,8 @@ reports an error.
@end itemize
-
-@node Varying Target Capabilities
-@section Varying Target Capabilities
-
-Some targets don't support floating-point, and some would rather not
-have to deal with @code{long long} operations. Also, different targets
-will have different stack sizes, and different bytecode buffer lengths.
-
-Thus, GDB needs a way to ask the target about itself. We haven't worked
-out the details yet, but in general, GDB should be able to send the
-target a packet asking it to describe itself. The reply should be a
-packet whose length is explicit, so we can add new information to the
-packet in future revisions of the agent, without confusing old versions
-of GDB, and it should contain a version number. It should contain at
-least the following information:
-
-@itemize @bullet
-
-@item
-whether floating point is supported
-
-@item
-whether @code{long long} is supported
-
-@item
-maximum acceptable size of bytecode stack
-
-@item
-maximum acceptable length of bytecode expressions
-
-@item
-which registers are actually available for collection
-
-@item
-whether the target supports disabled tracepoints
-
-@end itemize
-
@node Rationale
-@section Rationale
+@subsection Rationale
Some of the design decisions apparent above are arguable.
@@ -748,3 +730,60 @@ opcode 0x30 reserved, to remain compatible with the customer who added
it.
@end table
+
+@node Control Agent
+@section Turn Agent On And Off
+
+You can control whether the agent is used as an aid for debugging
+with the following commands:
+
+@table @code
+@item set agent on
+Causes the agent to perform some operations on behalf of the
+debugger. Just which operations requested by the user will be done
+by the agent depends on the agent's capabilities. For example, if
+you request to evaluate breakpoint conditions in the agent, and the
+agent has such capability as well, then breakpoint conditions will be
+evaluated in the agent.
+
+@item set agent off
+Disables execution of debugging operations by the agent. All of the
+operations will be performed by @value{GDBN}.
+@end table
+
+@node Varying Target Capabilities
+@section Varying Target Capabilities
+
+Some targets don't support floating-point, and some would rather not
+have to deal with @code{long long} operations. Also, different targets
+will have different stack sizes, and different bytecode buffer lengths.
+
+Thus, @value{GDBN} needs a way to ask the target about itself. We haven't worked
+out the details yet, but in general, @value{GDBN} should be able to send the
+target a packet asking it to describe itself. The reply should be a
+packet whose length is explicit, so we can add new information to the
+packet in future revisions of the agent, without confusing old versions
+of GDB, and it should contain a version number. It should contain at
+least the following information:
+
+@itemize @bullet
+
+@item
+whether floating point is supported
+
+@item
+whether @code{long long} is supported
+
+@item
+maximum acceptable size of bytecode stack
+
+@item
+maximum acceptable length of bytecode expressions
+
+@item
+which registers are actually available for collection
+
+@item
+whether the target supports disabled tracepoints
+
+@end itemize
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index c4763d4..810dc3c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -170,7 +170,7 @@ software in general. We will miss him.
* Installing GDB:: Installing GDB
* Maintenance Commands:: Maintenance Commands
* Remote Protocol:: GDB Remote Serial Protocol
-* Agent Expressions:: The GDB Agent Expression Mechanism
+* Agent:: Debugging Agent
* Target Descriptions:: How targets can describe themselves to
@value{GDBN}
* Operating System Information:: Getting additional information from
--
1.7.0.4
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 5/8] Doc for agent
2012-01-26 1:53 ` Yao Qi
@ 2012-01-26 17:15 ` Eli Zaretskii
0 siblings, 0 replies; 46+ messages in thread
From: Eli Zaretskii @ 2012-01-26 17:15 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
> Date: Thu, 26 Jan 2012 09:31:42 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <gdb-patches@sourceware.org>
>
> Here is a new doc patch.
Thanks.
> +Therefore, traditional debugging model is too intrusive to reproduce
> +the bug.
^^^^^^^
"some bugs"
> +bytecode, and records the values for @value{GDBN} to retrieve later. We call these
> +bytecode @dfn{Agent Expression}.
^^^^^^^^
"bytecodes", in plural.
OK with these changes.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [patch 5/8] Doc for agent
2012-01-23 14:03 ` [patch 5/8] Doc for agent Yao Qi
2012-01-23 18:12 ` Eli Zaretskii
@ 2012-02-09 19:55 ` Pedro Alves
2012-02-10 13:30 ` Yao Qi
1 sibling, 1 reply; 46+ messages in thread
From: Pedro Alves @ 2012-02-09 19:55 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 01/23/2012 01:58 PM, Yao Qi wrote:
> In this patch, we create a new chapter for agent, and put "Agent
> Expression" under this chapter. One section "Control Agent" is added to
> document command in patch 3/8, and the rest of them is from original
> "Agent Expression".
Some things, like the new target-side breakpoint conditions, and tracepoints,
use agent expressions as well, without having an agent loaded in the inferior.
Do you think it still makes sense to move the whole section considering that?
I'm just worried about whether moving everything under the same umbrella is
inducing confusion, since it seems that what you're calling an "agent", is
really an in-process agent. I really don't know, and I'm just raising it,
in case it wasn't considered.
--
Pedro Alves
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [patch 5/8] Doc for agent
2012-02-09 19:55 ` Pedro Alves
@ 2012-02-10 13:30 ` Yao Qi
2012-02-10 15:01 ` Pedro Alves
0 siblings, 1 reply; 46+ messages in thread
From: Yao Qi @ 2012-02-10 13:30 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 02/10/2012 03:54 AM, Pedro Alves wrote:
> Some things, like the new target-side breakpoint conditions, and tracepoints,
> use agent expressions as well, without having an agent loaded in the inferior.
> Do you think it still makes sense to move the whole section considering that?
No, I don't think it makes sense anymore, because agent expression is being
used in non-agent cases. My patch should be updated as well. We
can add a new chapter for "Agent", which is about how to control
agent by commands, and leave original appendix GDB Agent Expression
Mechanism there, with some minor changes, where needed. WDYT?
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [patch 5/8] Doc for agent
2012-02-10 13:30 ` Yao Qi
@ 2012-02-10 15:01 ` Pedro Alves
2012-02-10 16:18 ` Yao Qi
0 siblings, 1 reply; 46+ messages in thread
From: Pedro Alves @ 2012-02-10 15:01 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 02/10/2012 01:29 PM, Yao Qi wrote:
> On 02/10/2012 03:54 AM, Pedro Alves wrote:
>> Some things, like the new target-side breakpoint conditions, and tracepoints,
>> use agent expressions as well, without having an agent loaded in the inferior.
>> Do you think it still makes sense to move the whole section considering that?
>
> No, I don't think it makes sense anymore, because agent expression is being
> used in non-agent cases. My patch should be updated as well. We
> can add a new chapter for "Agent", which is about how to control
> agent by commands, and leave original appendix GDB Agent Expression
> Mechanism there, with some minor changes, where needed. WDYT?
Sounds good.
Re. "control agent by commands". So you're planning on making the agent speak
some other command set other than RSP? I'd think that making it talk exactly
RSP would be the best, since #1, you need some kind of command set anyway;
#2, the agent should be able to support debugging (stepping, breakpoints,
etc.) without gdbserver involved, and for that you'd want to support
the RSP anyway so that GDB can connect. Unless you're coming up with some
new rpc/marshaling protocol that's clearly superior to RSP?
--
Pedro Alves
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [patch 5/8] Doc for agent
2012-02-10 15:01 ` Pedro Alves
@ 2012-02-10 16:18 ` Yao Qi
2012-02-10 16:28 ` Pedro Alves
0 siblings, 1 reply; 46+ messages in thread
From: Yao Qi @ 2012-02-10 16:18 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 02/10/2012 11:01 PM, Pedro Alves wrote:
> Re. "control agent by commands". So you're planning on making the agent speak
> some other command set other than RSP? I'd think that making it talk exactly
> RSP would be the best, since #1, you need some kind of command set anyway;
> #2, the agent should be able to support debugging (stepping, breakpoints,
> etc.) without gdbserver involved, and for that you'd want to support
> the RSP anyway so that GDB can connect. Unless you're coming up with some
> new rpc/marshaling protocol that's clearly superior to RSP?
[I planed to discuss on the choice on protocols when this series go
in, because this series of patches just set up a communication
channel, but protocol is not involved this patch series.]
The protocol between agent and GDB/GDBserver is different from RSP.
This protocol is composed by a set of commands. In each command,
there is an opcode (in ascii) and oprand (in binary). When sending
a command, we serialize parameters into command buffer, and in agent
side, we de-serialize parameters out of command buffer. The process
of serialize/de-serialize is quite specific to each command. Taking
command `installing tracepoint' for example, the parameter is an
instance of tracepoint. We copy the instance of tracepoint, along
with objects it references to, to command buffer, and in agent, we
build tracepoint instance from command buffer. It is similar to RPC
call.
The reasons for this kind of design are,
#1. agent, inferior and debugger (GDB or GDBserver) are running
on the same machine, so protocol doesn't to have to handle machine
difference, such as endianess, word size, etc. Binary copy should
work fine.
#2. avoid to transform data twice. When data is ready, say
tracepoint, it is efficient to copy data directly, rather than
transforming to some format, and agent will transform it back later.
#3. be efficient. binary presentation is quite compact, and
memcpy-like operation is efficient as well.
#4. as close to raw data as possible. agent is running in the
same process with inferior, and the same machine with debugger. The
process of protocol is like copying parameter from one process to
another. We don't have to transform the format of raw data.
It is a piece of new work, I am open to comments.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [patch 5/8] Doc for agent
2012-02-10 16:18 ` Yao Qi
@ 2012-02-10 16:28 ` Pedro Alves
2012-02-23 7:51 ` Yao Qi
0 siblings, 1 reply; 46+ messages in thread
From: Pedro Alves @ 2012-02-10 16:28 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 02/10/2012 04:18 PM, Yao Qi wrote:
> The reasons for this kind of design are,
>
> #1. agent, inferior and debugger (GDB or GDBserver) are running
> on the same machine, so protocol doesn't to have to handle machine
> difference, such as endianess, word size, etc. Binary copy should
> work fine.
Nope, that's not true at all. Several architectures can run in
different modes (x86 32-bit/64-bit; arm/thumbx big/little/mixed; mips; ppc, etc.).
Several architectures have more than one ABI. We can have e.g., a 64-bit
gdbserver controlling a 32-bit process on x86. You can have a gdbserver controlling
an inferior that's running a different endianness on some machines.
The current IPA has the limitation that fast tracepoints only work if the
inferior has the same arch/abi as gdbserver. If we're adding a new IPC,
and an IPA v2, let's not repeat the mistake.
> #2. avoid to transform data twice. When data is ready, say
> tracepoint, it is efficient to copy data directly, rather than
> transforming to some format, and agent will transform it back later.
Traceframe data is a different matter. We can open a shared
memory channel for those, for example. Note traceframes have
a fixed and defined format (so we have the tfile target).
> #3. be efficient. binary presentation is quite compact, and
> memcpy-like operation is efficient as well.
It also doesn't work. See above.
> #4. as close to raw data as possible. agent is running in the
> same process with inferior, and the same machine with debugger. The
> process of protocol is like copying parameter from one process to
> another. We don't have to transform the format of raw data.
The command channel is not the fast path. You don't need, and shouldn't
be micro-optimizing it.
> It is a piece of new work, I am open to comments.
--
Pedro Alves
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [patch 5/8] Doc for agent
2012-02-10 16:28 ` Pedro Alves
@ 2012-02-23 7:51 ` Yao Qi
2012-02-23 19:50 ` Pedro Alves
0 siblings, 1 reply; 46+ messages in thread
From: Yao Qi @ 2012-02-23 7:51 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 02/11/2012 12:28 AM, Pedro Alves wrote:
>> > The reasons for this kind of design are,
>> >
>> > #1. agent, inferior and debugger (GDB or GDBserver) are running
>> > on the same machine, so protocol doesn't to have to handle machine
>> > difference, such as endianess, word size, etc. Binary copy should
>> > work fine.
> Nope, that's not true at all. Several architectures can run in
> different modes (x86 32-bit/64-bit; arm/thumbx big/little/mixed; mips; ppc, etc.).
> Several architectures have more than one ABI. We can have e.g., a 64-bit
> gdbserver controlling a 32-bit process on x86. You can have a gdbserver controlling
> an inferior that's running a different endianness on some machines.
>
I agree that the process of GDB/GDBserver and process of inferior have
differences on ABI, word-size, except endianess. I can't find an
example that two processes on the same machine have different endianess.
I google'ed a while, and find some processors such as ARM, PowerPC, and
Itanium have such capability to change endian mode on runtime, but seems
not widely used. Do we need to consider this case here?
> The current IPA has the limitation that fast tracepoints only work if the
> inferior has the same arch/abi as gdbserver. If we're adding a new IPC,
> and an IPA v2, let's not repeat the mistake.
>
Pedro, thanks for your input. I'll post the agent protocol draft later.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [patch 5/8] Doc for agent
2012-02-23 7:51 ` Yao Qi
@ 2012-02-23 19:50 ` Pedro Alves
0 siblings, 0 replies; 46+ messages in thread
From: Pedro Alves @ 2012-02-23 19:50 UTC (permalink / raw)
To: Yao Qi; +Cc: Pedro Alves, gdb-patches
On 02/23/2012 06:55 AM, Yao Qi wrote:
> On 02/11/2012 12:28 AM, Pedro Alves wrote:
>>>> The reasons for this kind of design are,
>>>>
>>>> #1. agent, inferior and debugger (GDB or GDBserver) are running
>>>> on the same machine, so protocol doesn't to have to handle machine
>>>> difference, such as endianess, word size, etc. Binary copy should
>>>> work fine.
>> Nope, that's not true at all. Several architectures can run in
>> different modes (x86 32-bit/64-bit; arm/thumbx big/little/mixed; mips; ppc, etc.).
>> Several architectures have more than one ABI. We can have e.g., a 64-bit
>> gdbserver controlling a 32-bit process on x86. You can have a gdbserver controlling
>> an inferior that's running a different endianness on some machines.
>>
>
> I agree that the process of GDB/GDBserver and process of inferior have
> differences on ABI, word-size, except endianess. I can't find an
> example that two processes on the same machine have different endianess.
> I google'ed a while, and find some processors such as ARM, PowerPC, and
> Itanium have such capability to change endian mode on runtime, but seems
> not widely used. Do we need to consider this case here?
Maybe not used much presently. But I don't see how that makes a
difference. It's best for GDBserver to not assume much from the inferior.
>> The current IPA has the limitation that fast tracepoints only work if the
>> inferior has the same arch/abi as gdbserver. If we're adding a new IPC,
>> and an IPA v2, let's not repeat the mistake.
>>
>
> Pedro, thanks for your input. I'll post the agent protocol draft later.
--
Pedro Alves
^ permalink raw reply [flat|nested] 46+ messages in thread
* [patch 6/8] Agent's capability
2012-01-23 13:37 [patch 0/8] GDB/GDBserver talks with agents Yao Qi
` (4 preceding siblings ...)
2012-01-23 14:03 ` [patch 5/8] Doc for agent Yao Qi
@ 2012-01-23 14:07 ` Yao Qi
2012-01-24 3:49 ` Yao Qi
2012-02-09 20:09 ` Pedro Alves
2012-01-23 14:29 ` [patch 7/8] Agent capability for static tracepoint Yao Qi
` (3 subsequent siblings)
9 siblings, 2 replies; 46+ messages in thread
From: Yao Qi @ 2012-01-23 14:07 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 558 bytes --]
GDB/GDBserver could be able to talk with different agents, if they
follow the same protocol. However, different agents may have different
capabilities, say, one agent can install fast tracepoint, while the
other agent can't. Agent capability is to present what agent can do.
In agent side, we use an interger bit map, and each bit represents a
corresponding capability.
This piece of work is different from this, which is about target capability,
http://sourceware.org/gdb/current/onlinedocs/gdb/Varying-Target-Capabilities.html
--
Yao (é½å°§)
[-- Attachment #2: 0006-agent-capability.patch --]
[-- Type: text/x-patch, Size: 2776 bytes --]
2012-01-23 Yao Qi <yao@codesourcery.com>
* common/agent.c (struct ipa_sym_addresses) <addr_capability>: New.
(agent_check_capability): New.
(symbol_list): New array element.
* common/agent.h (enum agent_capa): New.
---
gdb/common/agent.c | 31 +++++++++++++++++++++++++++++++
gdb/common/agent.h | 15 +++++++++++++++
2 files changed, 46 insertions(+), 0 deletions(-)
diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index 5c66071..f4ff08c 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -44,6 +44,7 @@ struct ipa_sym_addresses
{
CORE_ADDR addr_helper_thread_id;
CORE_ADDR addr_cmd_buf;
+ CORE_ADDR addr_capability;
};
/* Cache of the helper thread id. */
@@ -57,6 +58,7 @@ static struct
} symbol_list[] = {
IPA_SYM(helper_thread_id),
IPA_SYM(cmd_buf),
+ IPA_SYM(capability),
};
static struct ipa_sym_addresses ipa_sym_addrs;
@@ -114,6 +116,9 @@ agent_get_helper_thread_id (void)
return helper_thread_id;
}
+/* Each bit of it stands for a capability of agent. */
+static unsigned int agent_capability = 0;
+
/* Connects to synchronization socket. PID is the pid of inferior, which is
used to set up connection socket. */
@@ -260,3 +265,29 @@ agent_run_command (int pid, const char *cmd, int len)
}
}
}
+
+/* Return true if agent has capability AGENT_CAP, otherwise return false. */
+
+int
+agent_check_capability (enum agent_capa agent_capa)
+{
+ if (agent_capability == 0)
+ {
+#ifdef GDBSERVER
+ if (read_inferior_memory (ipa_sym_addrs.addr_capability,
+ (unsigned char *) &agent_capability,
+ sizeof agent_capability))
+#else
+ enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch);
+ gdb_byte buf[4];
+
+ if (target_read_memory (ipa_sym_addrs.addr_capability,
+ buf, sizeof buf) == 0)
+ agent_capability = extract_unsigned_integer (buf, sizeof buf,
+ byte_order);
+ else
+#endif
+ warning ("Error reading helper thread's id in lib");
+ }
+ return agent_capability & agent_capa;
+}
diff --git a/gdb/common/agent.h b/gdb/common/agent.h
index 079b65e..cf3fab1 100644
--- a/gdb/common/agent.h
+++ b/gdb/common/agent.h
@@ -36,3 +36,18 @@ void agent_look_up_symbols (void);
extern int debug_agent ;
extern int use_agent;
+
+/* Capability of agent. Different agents may have different capabilities,
+ such as installing fast tracepoint or evaluating breakpoint conditions.
+ Capabilities are represented by bit-maps, and each capability occupy one
+ bit. */
+
+enum agent_capa
+{
+ /* Capability to install fast tracepoint. */
+ AGENT_CAPA_FAST_TRACE = 0x1,
+ /* Capability to install static tracepoint. */
+ AGENT_CAPA_STATIC_TRACE = (0x1 << 1),
+};
+
+int agent_check_capability (enum agent_capa);
--
1.7.0.4
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 6/8] Agent's capability
2012-01-23 14:07 ` [patch 6/8] Agent's capability Yao Qi
@ 2012-01-24 3:49 ` Yao Qi
2012-02-09 20:09 ` Pedro Alves
1 sibling, 0 replies; 46+ messages in thread
From: Yao Qi @ 2012-01-24 3:49 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 865 bytes --]
On 01/23/2012 10:03 PM, Yao Qi wrote:
> + warning ("Error reading helper thread's id in lib");
This warning message is copied form somewhere else. Fix it to
warning ("Error reading capability of agent");
> diff --git a/gdb/common/agent.h b/gdb/common/agent.h
> index 079b65e..cf3fab1 100644
> --- a/gdb/common/agent.h
> +++ b/gdb/common/agent.h
> @@ -36,3 +36,18 @@ void agent_look_up_symbols (void);
> extern int debug_agent ;
>
> extern int use_agent;
> +
> +/* Capability of agent. Different agents may have different capabilities,
> + such as installing fast tracepoint or evaluating breakpoint conditions.
> + Capabilities are represented by bit-maps, and each capability occupy one
^^^^
s/occupy/occupies/
> + bit. */
Here is the updated version.
--
Yao (é½å°§)
[-- Attachment #2: 0006-agent-capability.patch --]
[-- Type: text/x-patch, Size: 2771 bytes --]
2012-01-23 Yao Qi <yao@codesourcery.com>
* common/agent.c (struct ipa_sym_addresses) <addr_capability>: New.
(agent_check_capability): New.
(symbol_list): New array element.
* common/agent.h (enum agent_capa): New.
---
gdb/common/agent.c | 31 +++++++++++++++++++++++++++++++
gdb/common/agent.h | 15 +++++++++++++++
2 files changed, 46 insertions(+), 0 deletions(-)
diff --git a/gdb/common/agent.c b/gdb/common/agent.c
index 5c66071..f014a98 100644
--- a/gdb/common/agent.c
+++ b/gdb/common/agent.c
@@ -44,6 +44,7 @@ struct ipa_sym_addresses
{
CORE_ADDR addr_helper_thread_id;
CORE_ADDR addr_cmd_buf;
+ CORE_ADDR addr_capability;
};
/* Cache of the helper thread id. */
@@ -57,6 +58,7 @@ static struct
} symbol_list[] = {
IPA_SYM(helper_thread_id),
IPA_SYM(cmd_buf),
+ IPA_SYM(capability),
};
static struct ipa_sym_addresses ipa_sym_addrs;
@@ -114,6 +116,9 @@ agent_get_helper_thread_id (void)
return helper_thread_id;
}
+/* Each bit of it stands for a capability of agent. */
+static unsigned int agent_capability = 0;
+
/* Connects to synchronization socket. PID is the pid of inferior, which is
used to set up connection socket. */
@@ -260,3 +265,29 @@ agent_run_command (int pid, const char *cmd, int len)
}
}
}
+
+/* Return true if agent has capability AGENT_CAP, otherwise return false. */
+
+int
+agent_check_capability (enum agent_capa agent_capa)
+{
+ if (agent_capability == 0)
+ {
+#ifdef GDBSERVER
+ if (read_inferior_memory (ipa_sym_addrs.addr_capability,
+ (unsigned char *) &agent_capability,
+ sizeof agent_capability))
+#else
+ enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch);
+ gdb_byte buf[4];
+
+ if (target_read_memory (ipa_sym_addrs.addr_capability,
+ buf, sizeof buf) == 0)
+ agent_capability = extract_unsigned_integer (buf, sizeof buf,
+ byte_order);
+ else
+#endif
+ warning ("Error reading capability of agent");
+ }
+ return agent_capability & agent_capa;
+}
diff --git a/gdb/common/agent.h b/gdb/common/agent.h
index 079b65e..a858472 100644
--- a/gdb/common/agent.h
+++ b/gdb/common/agent.h
@@ -36,3 +36,18 @@ void agent_look_up_symbols (void);
extern int debug_agent ;
extern int use_agent;
+
+/* Capability of agent. Different agents may have different capabilities,
+ such as installing fast tracepoint or evaluating breakpoint conditions.
+ Capabilities are represented by bit-maps, and each capability occupies one
+ bit. */
+
+enum agent_capa
+{
+ /* Capability to install fast tracepoint. */
+ AGENT_CAPA_FAST_TRACE = 0x1,
+ /* Capability to install static tracepoint. */
+ AGENT_CAPA_STATIC_TRACE = (0x1 << 1),
+};
+
+int agent_check_capability (enum agent_capa);
--
1.7.0.4
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 6/8] Agent's capability
2012-01-23 14:07 ` [patch 6/8] Agent's capability Yao Qi
2012-01-24 3:49 ` Yao Qi
@ 2012-02-09 20:09 ` Pedro Alves
2012-02-10 12:25 ` Yao Qi
1 sibling, 1 reply; 46+ messages in thread
From: Pedro Alves @ 2012-02-09 20:09 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 01/23/2012 02:03 PM, Yao Qi wrote:
> GDB/GDBserver could be able to talk with different agents, if they
> follow the same protocol. However, different agents may have different
> capabilities, say, one agent can install fast tracepoint, while the
> other agent can't. Agent capability is to present what agent can do.
>
> In agent side, we use an interger bit map, and each bit represents a
> corresponding capability.
>
> This piece of work is different from this, which is about target capability,
>
> http://sourceware.org/gdb/current/onlinedocs/gdb/Varying-Target-Capabilities.html
That page is obsolete. We've been addressing those kinds of things with
qSupported features. I won't stand in the way of this change, but I do
wonder whether we shouldn't instead ask the agent about its features with
a similar (or exactly the same) mechanism.
>
> -- Yao (é½å°§)
>
>
> 0006-agent-capability.patch
>
>
> 2012-01-23 Yao Qi <yao@codesourcery.com>
>
> * common/agent.c (struct ipa_sym_addresses) <addr_capability>: New.
> (agent_check_capability): New.
> (symbol_list): New array element.
> * common/agent.h (enum agent_capa): New.
> ---
> gdb/common/agent.c | 31 +++++++++++++++++++++++++++++++
> gdb/common/agent.h | 15 +++++++++++++++
> 2 files changed, 46 insertions(+), 0 deletions(-)
>
> diff --git a/gdb/common/agent.c b/gdb/common/agent.c
> index 5c66071..f4ff08c 100644
> --- a/gdb/common/agent.c
> +++ b/gdb/common/agent.c
> @@ -44,6 +44,7 @@ struct ipa_sym_addresses
> {
> CORE_ADDR addr_helper_thread_id;
> CORE_ADDR addr_cmd_buf;
> + CORE_ADDR addr_capability;
> };
>
> /* Cache of the helper thread id. */
> @@ -57,6 +58,7 @@ static struct
> } symbol_list[] = {
> IPA_SYM(helper_thread_id),
> IPA_SYM(cmd_buf),
> + IPA_SYM(capability),
> };
>
> static struct ipa_sym_addresses ipa_sym_addrs;
> @@ -114,6 +116,9 @@ agent_get_helper_thread_id (void)
> return helper_thread_id;
> }
>
> +/* Each bit of it stands for a capability of agent. */
> +static unsigned int agent_capability = 0;
Something should be clearing these fields whenever you launch a
new program, or reconnect.
> +
> /* Connects to synchronization socket. PID is the pid of inferior, which is
> used to set up connection socket. */
>
> @@ -260,3 +265,29 @@ agent_run_command (int pid, const char *cmd, int len)
> }
> }
> }
> +
> +/* Return true if agent has capability AGENT_CAP, otherwise return false. */
> +
> +int
> +agent_check_capability (enum agent_capa agent_capa)
> +{
> + if (agent_capability == 0)
Spurious space.
> + {
> +#ifdef GDBSERVER
> + if (read_inferior_memory (ipa_sym_addrs.addr_capability,
> + (unsigned char *) &agent_capability,
> + sizeof agent_capability))
> +#else
> + enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch);
> + gdb_byte buf[4];
> +
> + if (target_read_memory (ipa_sym_addrs.addr_capability,
> + buf, sizeof buf) == 0)
> + agent_capability = extract_unsigned_integer (buf, sizeof buf,
> + byte_order);
> + else
> +#endif
#endif seems misplaced.
> + warning ("Error reading helper thread's id in lib");
Wrong warning?
> + }
> + return agent_capability & agent_capa;
> +}
> diff --git a/gdb/common/agent.h b/gdb/common/agent.h
> index 079b65e..cf3fab1 100644
> --- a/gdb/common/agent.h
> +++ b/gdb/common/agent.h
> @@ -36,3 +36,18 @@ void agent_look_up_symbols (void);
> extern int debug_agent ;
>
> extern int use_agent;
> +
> +/* Capability of agent. Different agents may have different capabilities,
> + such as installing fast tracepoint or evaluating breakpoint conditions.
> + Capabilities are represented by bit-maps, and each capability occupy one
> + bit. */
> +
> +enum agent_capa
> +{
> + /* Capability to install fast tracepoint. */
> + AGENT_CAPA_FAST_TRACE = 0x1,
> + /* Capability to install static tracepoint. */
> + AGENT_CAPA_STATIC_TRACE = (0x1 << 1),
> +};
> +
> +int agent_check_capability (enum agent_capa);
--
Pedro Alves
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 6/8] Agent's capability
2012-02-09 20:09 ` Pedro Alves
@ 2012-02-10 12:25 ` Yao Qi
2012-02-10 12:37 ` Pedro Alves
0 siblings, 1 reply; 46+ messages in thread
From: Yao Qi @ 2012-02-10 12:25 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 02/10/2012 04:09 AM, Pedro Alves wrote:
> That page is obsolete. We've been addressing those kinds of things with
> qSupported features. I won't stand in the way of this change, but I do
> wonder whether we shouldn't instead ask the agent about its features with
> a similar (or exactly the same) mechanism.
qSupported is used for GDB to get the list of features remote stub
supports. However, agent can talk with GDB or GDBserver directly, so I
hope GDB and GDBserver can use the same interface to get agent's
capability. qSupported doesn't help here.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [patch 6/8] Agent's capability
2012-02-10 12:25 ` Yao Qi
@ 2012-02-10 12:37 ` Pedro Alves
2012-02-10 13:07 ` Yao Qi
0 siblings, 1 reply; 46+ messages in thread
From: Pedro Alves @ 2012-02-10 12:37 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 02/10/2012 12:25 PM, Yao Qi wrote:
> On 02/10/2012 04:09 AM, Pedro Alves wrote:
>> That page is obsolete. We've been addressing those kinds of things with
>> qSupported features. I won't stand in the way of this change, but I do
>> wonder whether we shouldn't instead ask the agent about its features with
>> a similar (or exactly the same) mechanism.
>
> qSupported is used for GDB to get the list of features remote stub
> supports. However, agent can talk with GDB or GDBserver directly, so I
> hope GDB and GDBserver can use the same interface to get agent's
> capability. qSupported doesn't help here.
??? We have a way to sent commands to the agent, hence,
"I do wonder whether we shouldn't instead ask the agent about its
features with a similar (or exactly the same) mechanism.". Heck,
the agent itself could speak RSP, so we could just have gdb or gdbserver
forward it the tracepoint, breakpoint, etc packets.
Bit flags only allow on/off, and we'll run out of bits at some point. A
qSupported-like mechanism wouldn't have those issues. Just saying.
--
Pedro Alves
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [patch 6/8] Agent's capability
2012-02-10 12:37 ` Pedro Alves
@ 2012-02-10 13:07 ` Yao Qi
0 siblings, 0 replies; 46+ messages in thread
From: Yao Qi @ 2012-02-10 13:07 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 02/10/2012 08:37 PM, Pedro Alves wrote:
> On 02/10/2012 12:25 PM, Yao Qi wrote:
>> On 02/10/2012 04:09 AM, Pedro Alves wrote:
>>> That page is obsolete. We've been addressing those kinds of things with
>>> qSupported features. I won't stand in the way of this change, but I do
>>> wonder whether we shouldn't instead ask the agent about its features with
>>> a similar (or exactly the same) mechanism.
>>
>> qSupported is used for GDB to get the list of features remote stub
>> supports. However, agent can talk with GDB or GDBserver directly, so I
>> hope GDB and GDBserver can use the same interface to get agent's
>> capability. qSupported doesn't help here.
>
> ??? We have a way to sent commands to the agent, hence,
> "I do wonder whether we shouldn't instead ask the agent about its
> features with a similar (or exactly the same) mechanism.". Heck,
> the agent itself could speak RSP, so we could just have gdb or gdbserver
> forward it the tracepoint, breakpoint, etc packets.
> Bit flags only allow on/off, and we'll run out of bits at some point. A
> qSupported-like mechanism wouldn't have those issues. Just saying.
>
OK, I get your point. It is reasonable to me.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 46+ messages in thread
* [patch 7/8] Agent capability for static tracepoint
2012-01-23 13:37 [patch 0/8] GDB/GDBserver talks with agents Yao Qi
` (5 preceding siblings ...)
2012-01-23 14:07 ` [patch 6/8] Agent's capability Yao Qi
@ 2012-01-23 14:29 ` Yao Qi
2012-02-09 20:13 ` Pedro Alves
2012-01-23 16:03 ` [patch 8/8] Control agent in testsuite Yao Qi
` (2 subsequent siblings)
9 siblings, 1 reply; 46+ messages in thread
From: Yao Qi @ 2012-01-23 14:29 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 262 bytes --]
Current libinproctrace.so agent is able to do operations on static
tracepoint, which can be treated as one capability. This patch is to
teach gdbserver to check agent's capability when performing operations
related to static tracepoint.
--
Yao (é½å°§)
[-- Attachment #2: 0007-gdb-agent-for-static-tracepoint.patch --]
[-- Type: text/x-patch, Size: 3419 bytes --]
2012-01-23 Yao Qi <yao@codesourcery.com>
* tracepoint.c (gdb_agent_capability): New global.
(clear_installed_tracepoints): Check whether agent has capability
for static tracepoint.
(install_tracepoint): Likewise.
(cmd_qtstart): Likewise.
(handle_tracepoint_query): Likewise.
---
gdb/gdbserver/tracepoint.c | 68 +++++++++++++++++++++++++++++--------------
1 files changed, 46 insertions(+), 22 deletions(-)
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 7f462bc..9527d74 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -2314,7 +2314,11 @@ clear_installed_tracepoints (void)
;
else
{
- unprobe_marker_at (tpoint->address);
+ if (agent_check_capability (AGENT_CAPA_STATIC_TRACE) == 0)
+ warning ("Agent does not have capability"
+ "for static tracepoint.");
+ else
+ unprobe_marker_at (tpoint->address);
prev_stpoint = tpoint;
}
break;
@@ -2989,8 +2993,8 @@ install_tracepoint (struct tracepoint *tpoint, char *own_buf)
}
else
{
- if (tp)
- tpoint->handle = (void *) -1;
+ if (agent_check_capability (AGENT_CAPA_STATIC_TRACE) == 0)
+ warning ("Agent does not have capability for static tracepoint.");
else
{
if (probe_marker_at (tpoint->address, own_buf) == 0)
@@ -3094,13 +3098,19 @@ cmd_qtstart (char *packet)
}
else
{
- if (probe_marker_at (tpoint->address, packet) == 0)
+ if (agent_check_capability (AGENT_CAPA_STATIC_TRACE) == 0)
+ warning ("Agent does not have capability"
+ "for static tracepoint.");
+ else
{
- tpoint->handle = (void *) -1;
-
- /* So that we can handle multiple static tracepoints
- at the same address easily. */
- prev_stpoint = tpoint;
+ if (probe_marker_at (tpoint->address, packet) == 0)
+ {
+ tpoint->handle = (void *) -1;
+
+ /* So that we can handle multiple static tracepoints
+ at the same address easily. */
+ prev_stpoint = tpoint;
+ }
}
}
}
@@ -3968,20 +3978,32 @@ handle_tracepoint_query (char *packet)
cmd_qtbuffer (packet);
return 1;
}
- else if (strcmp ("qTfSTM", packet) == 0)
- {
- cmd_qtfstm (packet);
- return 1;
- }
- else if (strcmp ("qTsSTM", packet) == 0)
+ else if (strcmp ("qTfSTM", packet) == 0 || strcmp ("qTsSTM", packet) == 0
+ || strncmp ("qTSTMat:", packet, strlen ("qTSTMat:")) == 0)
{
- cmd_qtsstm (packet);
- return 1;
- }
- else if (strncmp ("qTSTMat:", packet, strlen ("qTSTMat:")) == 0)
- {
- cmd_qtstmat (packet);
- return 1;
+ if (agent_check_capability (AGENT_CAPA_STATIC_TRACE) == 0)
+ {
+ warning ("Agent does not have capability for static tracepoint.");
+ return 0;
+ }
+ else
+ {
+ if (strcmp ("qTfSTM", packet) == 0)
+ {
+ cmd_qtfstm (packet);
+ return 1;
+ }
+ else if (strcmp ("qTsSTM", packet) == 0)
+ {
+ cmd_qtsstm (packet);
+ return 1;
+ }
+ else if (strncmp ("qTSTMat:", packet, strlen ("qTSTMat:")) == 0)
+ {
+ cmd_qtstmat (packet);
+ return 1;
+ }
+ }
}
else if (strcmp ("qTMinFTPILen", packet) == 0)
{
@@ -7986,6 +8008,8 @@ gdb_ust_thread (void *arg)
#include <signal.h>
+IP_AGENT_EXPORT int gdb_agent_capability = AGENT_CAPA_STATIC_TRACE;
+
static void
gdb_ust_init (void)
{
--
1.7.0.4
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 7/8] Agent capability for static tracepoint
2012-01-23 14:29 ` [patch 7/8] Agent capability for static tracepoint Yao Qi
@ 2012-02-09 20:13 ` Pedro Alves
2012-02-10 14:29 ` Yao Qi
0 siblings, 1 reply; 46+ messages in thread
From: Pedro Alves @ 2012-02-09 20:13 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 01/23/2012 02:07 PM, Yao Qi wrote:
> Current libinproctrace.so agent is able to do operations on static
> tracepoint, which can be treated as one capability. This patch is to
> teach gdbserver to check agent's capability when performing operations
> related to static tracepoint.
>
Hmm, not sure. Why aren't these being hooked at the same places
where we already check/call maybe_write_ipa_ust_not_loaded and
in_process_agent_loaded_ust?
> -- Yao (é½å°§)
>
>
> 0007-gdb-agent-for-static-tracepoint.patch
>
>
> 2012-01-23 Yao Qi <yao@codesourcery.com>
>
> * tracepoint.c (gdb_agent_capability): New global.
> (clear_installed_tracepoints): Check whether agent has capability
> for static tracepoint.
> (install_tracepoint): Likewise.
> (cmd_qtstart): Likewise.
> (handle_tracepoint_query): Likewise.
> ---
> gdb/gdbserver/tracepoint.c | 68 +++++++++++++++++++++++++++++--------------
> 1 files changed, 46 insertions(+), 22 deletions(-)
>
> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
> index 7f462bc..9527d74 100644
> --- a/gdb/gdbserver/tracepoint.c
> +++ b/gdb/gdbserver/tracepoint.c
> @@ -2314,7 +2314,11 @@ clear_installed_tracepoints (void)
> ;
> else
> {
> - unprobe_marker_at (tpoint->address);
> + if (agent_check_capability (AGENT_CAPA_STATIC_TRACE) == 0)
> + warning ("Agent does not have capability"
> + "for static tracepoint.");
> + else
> + unprobe_marker_at (tpoint->address);
> prev_stpoint = tpoint;
> }
> break;
> @@ -2989,8 +2993,8 @@ install_tracepoint (struct tracepoint *tpoint, char *own_buf)
> }
> else
> {
> - if (tp)
> - tpoint->handle = (void *) -1;
> + if (agent_check_capability (AGENT_CAPA_STATIC_TRACE) == 0)
> + warning ("Agent does not have capability for static tracepoint.");
> else
> {
> if (probe_marker_at (tpoint->address, own_buf) == 0)
> @@ -3094,13 +3098,19 @@ cmd_qtstart (char *packet)
> }
> else
> {
> - if (probe_marker_at (tpoint->address, packet) == 0)
> + if (agent_check_capability (AGENT_CAPA_STATIC_TRACE) == 0)
> + warning ("Agent does not have capability"
> + "for static tracepoint.");
> + else
> {
> - tpoint->handle = (void *) -1;
> -
> - /* So that we can handle multiple static tracepoints
> - at the same address easily. */
> - prev_stpoint = tpoint;
> + if (probe_marker_at (tpoint->address, packet) == 0)
> + {
> + tpoint->handle = (void *) -1;
> +
> + /* So that we can handle multiple static tracepoints
> + at the same address easily. */
> + prev_stpoint = tpoint;
> + }
> }
> }
> }
> @@ -3968,20 +3978,32 @@ handle_tracepoint_query (char *packet)
> cmd_qtbuffer (packet);
> return 1;
> }
> - else if (strcmp ("qTfSTM", packet) == 0)
> - {
> - cmd_qtfstm (packet);
> - return 1;
> - }
> - else if (strcmp ("qTsSTM", packet) == 0)
> + else if (strcmp ("qTfSTM", packet) == 0 || strcmp ("qTsSTM", packet) == 0
> + || strncmp ("qTSTMat:", packet, strlen ("qTSTMat:")) == 0)
> {
> - cmd_qtsstm (packet);
> - return 1;
> - }
> - else if (strncmp ("qTSTMat:", packet, strlen ("qTSTMat:")) == 0)
> - {
> - cmd_qtstmat (packet);
> - return 1;
> + if (agent_check_capability (AGENT_CAPA_STATIC_TRACE) == 0)
> + {
> + warning ("Agent does not have capability for static tracepoint.");
> + return 0;
> + }
> + else
> + {
> + if (strcmp ("qTfSTM", packet) == 0)
> + {
> + cmd_qtfstm (packet);
> + return 1;
> + }
> + else if (strcmp ("qTsSTM", packet) == 0)
> + {
> + cmd_qtsstm (packet);
> + return 1;
> + }
> + else if (strncmp ("qTSTMat:", packet, strlen ("qTSTMat:")) == 0)
> + {
> + cmd_qtstmat (packet);
> + return 1;
> + }
> + }
> }
> else if (strcmp ("qTMinFTPILen", packet) == 0)
> {
> @@ -7986,6 +8008,8 @@ gdb_ust_thread (void *arg)
>
> #include <signal.h>
>
> +IP_AGENT_EXPORT int gdb_agent_capability = AGENT_CAPA_STATIC_TRACE;
> +
> static void
> gdb_ust_init (void)
> {
> -- 1.7.0.4
--
Pedro Alves
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 7/8] Agent capability for static tracepoint
2012-02-09 20:13 ` Pedro Alves
@ 2012-02-10 14:29 ` Yao Qi
2012-02-10 14:56 ` Pedro Alves
0 siblings, 1 reply; 46+ messages in thread
From: Yao Qi @ 2012-02-10 14:29 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 02/10/2012 04:13 AM, Pedro Alves wrote:
> On 01/23/2012 02:07 PM, Yao Qi wrote:
>> > Current libinproctrace.so agent is able to do operations on static
>> > tracepoint, which can be treated as one capability. This patch is to
>> > teach gdbserver to check agent's capability when performing operations
>> > related to static tracepoint.
>> >
> Hmm, not sure. Why aren't these being hooked at the same places
> where we already check/call maybe_write_ipa_ust_not_loaded and
> in_process_agent_loaded_ust?
>
maybe_write_ipa_ust_not_loaded and in_process_agent_loaded_ust returns
agent is loaded or not. Considering GDB may/will support multiple
different agents, which have different capability, so "agent is loaded"
doesn't mean "agent has a certain capability". Is it reasonable?
I'd like replace global variable `ust_loaded' with capability mechanism
in agent.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [patch 7/8] Agent capability for static tracepoint
2012-02-10 14:29 ` Yao Qi
@ 2012-02-10 14:56 ` Pedro Alves
0 siblings, 0 replies; 46+ messages in thread
From: Pedro Alves @ 2012-02-10 14:56 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 02/10/2012 02:29 PM, Yao Qi wrote:
> On 02/10/2012 04:13 AM, Pedro Alves wrote:
>> On 01/23/2012 02:07 PM, Yao Qi wrote:
>>>> Current libinproctrace.so agent is able to do operations on static
>>>> tracepoint, which can be treated as one capability. This patch is to
>>>> teach gdbserver to check agent's capability when performing operations
>>>> related to static tracepoint.
>>>>
>> Hmm, not sure. Why aren't these being hooked at the same places
>> where we already check/call maybe_write_ipa_ust_not_loaded and
>> in_process_agent_loaded_ust?
>>
>
> maybe_write_ipa_ust_not_loaded and in_process_agent_loaded_ust returns
> agent is loaded or not. Considering GDB may/will support multiple
> different agents, which have different capability, so "agent is loaded"
> doesn't mean "agent has a certain capability". Is it reasonable?
> I'd like replace global variable `ust_loaded' with capability mechanism
> in agent.
in_process_agent_loaded_ust (note the ust) is only true when both the
IPA is loaded, _and_ there's ust support. So you should be able to
replace all those with "agent can do static tracepoints", as that's
what the checks are really doing. So again, why aren't your
checks done at exactly the same places the present checks are done?
--
Pedro Alves
^ permalink raw reply [flat|nested] 46+ messages in thread
* [patch 8/8] Control agent in testsuite
2012-01-23 13:37 [patch 0/8] GDB/GDBserver talks with agents Yao Qi
` (6 preceding siblings ...)
2012-01-23 14:29 ` [patch 7/8] Agent capability for static tracepoint Yao Qi
@ 2012-01-23 16:03 ` Yao Qi
2012-02-09 20:16 ` Pedro Alves
2012-02-05 4:32 ` [ping] [patch 0/8] GDB/GDBserver talks with agents Yao Qi
2012-02-09 19:02 ` Pedro Alves
9 siblings, 1 reply; 46+ messages in thread
From: Yao Qi @ 2012-01-23 16:03 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 430 bytes --]
After using agent, we need to run testsuite in two cases, agent is on
and off. This patch is to add such logic in testsuite, so that we can
control this through board file. If we add the following line in board
file,
set_board_info use_agent "on"
the agent is always turned on for remote testing. We need also to turn
agent on in native debugging, but I'll postpone it when gdb starts to
use agent.
--
Yao (é½å°§)
[-- Attachment #2: 0008-set-agent-on-in-board-file.patch --]
[-- Type: text/x-patch, Size: 1031 bytes --]
2012-01-22 Yao Qi <yao@codesourcery.com>
* lib/gdbserver-support.exp (gdbserver_run): Set agent on or off if use_agent
exits in board file.
---
gdb/testsuite/lib/gdbserver-support.exp | 12 +++++++++++-
1 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
index 11d9107..e72fc36 100644
--- a/gdb/testsuite/lib/gdbserver-support.exp
+++ b/gdb/testsuite/lib/gdbserver-support.exp
@@ -337,7 +337,17 @@ proc gdbserver_run { child_args } {
set gdbserver_protocol [lindex $res 0]
set gdbserver_gdbport [lindex $res 1]
- return [gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport]
+ set ret [gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport]
+
+ if [target_info exists use_agent] {
+ send_gdb "set agent [target_info use_agent]\n"
+ gdb_expect 120 {
+ -re "^set agent \(on|off\)\r\n$gdb_prompt $" {
+ }
+ }
+ }
+
+ return $ret
}
# Reconnect to the previous gdbserver session.
--
1.7.0.4
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 8/8] Control agent in testsuite
2012-01-23 16:03 ` [patch 8/8] Control agent in testsuite Yao Qi
@ 2012-02-09 20:16 ` Pedro Alves
0 siblings, 0 replies; 46+ messages in thread
From: Pedro Alves @ 2012-02-09 20:16 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 01/23/2012 02:29 PM, Yao Qi wrote:
> After using agent, we need to run testsuite in two cases, agent is on
> and off. This patch is to add such logic in testsuite, so that we can
> control this through board file. If we add the following line in board
> file,
>
> set_board_info use_agent "on"
>
> the agent is always turned on for remote testing. We need also to turn
> agent on in native debugging, but I'll postpone it when gdb starts to
> use agent.
This is a bit ugly, and I'm not seeing much point as is. You can just
put
set GDBFLAGS "-ex \"set agent on\""
in your board file?
--
Pedro Alves
^ permalink raw reply [flat|nested] 46+ messages in thread
* [ping] [patch 0/8] GDB/GDBserver talks with agents
2012-01-23 13:37 [patch 0/8] GDB/GDBserver talks with agents Yao Qi
` (7 preceding siblings ...)
2012-01-23 16:03 ` [patch 8/8] Control agent in testsuite Yao Qi
@ 2012-02-05 4:32 ` Yao Qi
2012-02-09 19:02 ` Pedro Alves
9 siblings, 0 replies; 46+ messages in thread
From: Yao Qi @ 2012-02-05 4:32 UTC (permalink / raw)
To: gdb-patches
On 01/23/2012 08:43 PM, Yao Qi wrote:
> When I am working on the new agent library [1], I found that only
> gdbserver is able to interact with agent on static tracepoints only.
> There are two limitations here, 1) only gdbserver is able to talk with
> agent, 2) only static tracepoint operations can be performed by agent.
> In order to make the interface of gdbserver and agent more clear, and
> new agent library as flexible as possible, these two limitations should
> be overcame/removed.
>
> In this patch set, we generalize the original code in
> gdbserver/tracepoint.c to talk with agent for static tracepoint, move
> "common" part into gdb/common/agent.c, so that both gdb and gdbserver
> can call them to interact with agent. Secondly, the communication
> method (sync socket and command buffer) between gdbserver and
> libinproctrace.so is kept, but is generalized to some extent, so that,
> gdb and gdbserver is able to communicate with agent for other purposes,
> such as installing fast tracepoint or evaluating breakpoint conditions.
> The operations for static tracepoint are regarded as one sort of
> commands on this communication channel.
>
> This patch set doesn't change any existing functions of gdb and
> gdbserver, but it makes gdb and gdbserver easier to communicate with
> other agents.
>
Ping this series of patches.
http://sourceware.org/ml/gdb-patches/2012-01/msg00762.html
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [patch 0/8] GDB/GDBserver talks with agents
2012-01-23 13:37 [patch 0/8] GDB/GDBserver talks with agents Yao Qi
` (8 preceding siblings ...)
2012-02-05 4:32 ` [ping] [patch 0/8] GDB/GDBserver talks with agents Yao Qi
@ 2012-02-09 19:02 ` Pedro Alves
9 siblings, 0 replies; 46+ messages in thread
From: Pedro Alves @ 2012-02-09 19:02 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 01/23/2012 12:43 PM, Yao Qi wrote:
> When I am working on the new agent library [1], I found that only
> gdbserver is able to interact with agent on static tracepoints only.
> There are two limitations here, 1) only gdbserver is able to talk with
> agent, 2) only static tracepoint operations can be performed by agent.
This was a result of some consideration. I thought of two main
options when I was first designing the IPA:
- have gdbserver peek/poke at the IPA memory directly. Requires
some way for gdbserver to know where to poke etc., but, this was it's
possible for the inferior to be completely (or virtually completely)
unaffected by the IPA.
- spawn a thread in the IPA, and use it as a communication gate
with gdbserver. The upside of this solution you can come up with
some protocol for the communication, which is simpler than caring about
ABIs and layouts of objects. The downsides are that spawning a
thread in the inferior makes non-threaded programs suddenly become
threaded; and, you need to come up with a way to run the IPA
communication thread a while behind gdb/gdbserver's back, so it can
process the commands.
The IPA only gained static tracepoints support later.
I had to pick one approach and move on, so I picked the first. Later on,
the only way to communicate with ust was by calling some of its functions,
which implies the second approach...
--
Pedro Alves
^ permalink raw reply [flat|nested] 46+ messages in thread