Re: [PATCH 2/9] add target_ops fields use_agent and can_use_agent

Pedro Alves
2012-02-23T21:17:49Z

On 02/17/2012 02:54 AM, Yao Qi wrote:
> This patch allows GDB to talk with GDBserver with agent running along
> with inferior.
>
> 2012-02-15 Yao Qi <yao@codesourcery.com>
>
> * target.h (struct target_ops) <to_use_agent>: New field.
> (struct target_ops) <to_can_use_agent>: New field.
> (target_use_agent, target_can_use_agent): New macro.
> * target.c (update_current_target): Update.
> * remote.c: New enum `PACKET_QAgent'.
> (remote_protocol_features): Add a new element.
> (remote_use_agent, remote_can_use_agent): New.
> (init_remote_ops): Initialize field `can_use_agent' with
> remote_can_use_agent. Intiailize field `use_agent' with
> remote_use_agent.
> * common/agent.c (use_agent): New global.
> * common/agent.h: Declare it.
> * tracepoint.c (info_static_tracepoint_markers_command): Call
> target_can_use_agent.
>
> gdb/gdbserver:
>
> 2012-02-15 Yao Qi <yao@codesourcery.com>
>
> * linux-low.c (linux_supports_agent): New.
> (linux_target_ops): Initialize field `supports_agent' with
> linux_supports_agent.
> * target.h (struct target_ops) <supports_agent>: New.
> (target_supports_agent): New macro.
> * server.c (handle_general_set): Handle packet 'QAgent'.
> (handle_query): Send `QAgent+'.
> * Makefile.in (server.o): Depends on agent.h
>
> gdb/doc:
>
> 2012-02-15 Yao Qi <yao@codesourcery.com>
>
> * gdb.texinfo (General Query Packets): Add packet `QAgent'.
> ---
> gdb/common/agent.c | 3 +++
> gdb/common/agent.h | 1 +
> gdb/doc/gdb.texinfo | 13 +++++++++++++
> gdb/gdbserver/Makefile.in | 2 +-
> gdb/gdbserver/linux-low.c | 7 +++++++
> gdb/gdbserver/server.c | 28 ++++++++++++++++++++++++++++
> gdb/gdbserver/target.h | 7 +++++++
> gdb/remote.c | 36 ++++++++++++++++++++++++++++++++++++
> gdb/target.c | 8 ++++++++
> gdb/target.h | 13 +++++++++++++
> gdb/tracepoint.c | 12 ++++++++++++
> 11 files changed, 129 insertions(+), 1 deletions(-)
>
> diff --git a/gdb/common/agent.c b/gdb/common/agent.c
> index 657bba6..5a9ac16 100644
> --- a/gdb/common/agent.c
> +++ b/gdb/common/agent.c
> @@ -41,6 +41,9 @@ int debug_agent = 0;
> fprintf_unfiltered (gdb_stdlog, fmt, ##args);
> #endif
>
> +/* Global flag to determine using agent or not. */
> +int use_agent = 0;
> +
> /* Addresses of in-process agent's symbols both GDB and GDBserver cares
> about. */
>
> diff --git a/gdb/common/agent.h b/gdb/common/agent.h
> index 31c46d9..2965215 100644
> --- a/gdb/common/agent.h
> +++ b/gdb/common/agent.h
> @@ -35,3 +35,4 @@ int agent_look_up_symbols (void);
>
> extern int debug_agent;
>
> +extern int use_agent;
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 02beed7..34bf77e 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -34512,6 +34512,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
> @@ -35091,6 +35096,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{-}
> @@ -35224,6 +35234,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 understands the @samp{QAgent} packet.
> +
> @item QAllow
> The remote stub understands the @samp{QAllow} packet.
>
> diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
> index 17fd043..e840db2 100644
> --- a/gdb/gdbserver/Makefile.in
> +++ b/gdb/gdbserver/Makefile.in
> @@ -395,7 +395,7 @@ mem-break.o: mem-break.c $(server_h)
> proc-service.o: proc-service.c $(server_h) $(gdb_proc_service_h)
> regcache.o: regcache.c $(server_h) $(regdef_h)
> remote-utils.o: remote-utils.c terminal.h $(server_h)
> -server.o: server.c $(server_h)
> +server.o: server.c $(server_h) $(agent_h)
> target.o: target.c $(server_h)
> thread-db.o: thread-db.c $(server_h) $(linux_low_h) $(gdb_proc_service_h) \
> $(gdb_thread_db_h)
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index ab34d84..c9b8a3b 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -4716,6 +4716,12 @@ linux_supports_disable_randomization (void)
> #endif
> }
>
> +static int
> +linux_supports_agent (void)
> +{
> + return 1;
> +}
> +
> /* Enumerate spufs IDs for process PID. */
> static int
> spu_enumerate_spu_ids (long pid, unsigned char *buf, CORE_ADDR offset, int len)
> @@ -5438,6 +5444,7 @@ static struct target_ops linux_target_ops = {
> linux_supports_disable_randomization,
> linux_get_min_fast_tracepoint_insn_len,
> linux_qxfer_libraries_svr4,
> + linux_supports_agent,
> };
>
> static void
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index c1788a9..28aef72 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -18,6 +18,7 @@
> along with this program. If not, see <http://www.gnu.org/licenses/>. */
>
> #include "server.h"
> +#include "agent.h"
>
> #if HAVE_UNISTD_H
> #include <unistd.h>
> @@ -529,6 +530,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. */
> + sprintf (own_buf, "E.Unknown QAgent value");
> + return;
> + }
> +
> + /* Update the flag. */
> + use_agent = req;
> + if (remote_debug)
> + fprintf (stderr, "[%s agent]\n", req ? "Enable" : "Disable");
> + write_ok (own_buf);
> + return;
> + }
> +
> /* Otherwise we didn't know what packet it was. Say we didn't
> understand it. */
> own_buf[0] = 0;
> @@ -1621,6 +1646,9 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
> strcat (own_buf, ";tracenz+");
> }
>
> + if (target_supports_agent ())
> + strcat (own_buf, ";QAgent+");
> +
> return;
> }
>
> diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
> index 03dff0f..256cfd9 100644
> --- a/gdb/gdbserver/target.h
> +++ b/gdb/gdbserver/target.h
> @@ -394,6 +394,9 @@ struct target_ops
> int (*qxfer_libraries_svr4) (const char *annex, unsigned char *readbuf,
> unsigned const char *writebuf,
> CORE_ADDR offset, int len);
> +
> + /* Return true if target supports debugging agent. */
> + int (*supports_agent) (void);
> };
>
> extern struct target_ops *the_target;
> @@ -514,6 +517,10 @@ void set_target_ops (struct target_ops *);
> (the_target->supports_disable_randomization ? \
> (*the_target->supports_disable_randomization) () : 0)
>
> +#define target_supports_agent() \
> + (the_target->supports_agent ? \
> + (*the_target->supports_agent) () : 0)
> +
> /* Start non-stop mode, returns 0 on success, -1 on failure. */
>
> int start_non_stop (int nonstop);
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 3187ac0..2977e31 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -65,6 +65,7 @@
> #include "tracepoint.h"
> #include "ax.h"
> #include "ax-gdb.h"
> +#include "agent.h"
>
> /* Temp hacks for tracepoint encoding migration. */
> static char *target_buf;
> @@ -1272,6 +1273,7 @@ enum {
> PACKET_QAllow,
> PACKET_qXfer_fdpic,
> PACKET_QDisableRandomization,
> + PACKET_QAgent,
> PACKET_MAX
> };
>
> @@ -3832,6 +3834,7 @@ static struct protocol_feature remote_protocol_features[] = {
> PACKET_qXfer_fdpic },
> { "QDisableRandomization", PACKET_DISABLE, remote_supported_packet,
> PACKET_QDisableRandomization },
> + { "QAgent", PACKET_DISABLE, remote_supported_packet, PACKET_QAgent},
> { "tracenz", PACKET_DISABLE,
> remote_string_tracing_feature, -1 },
> };
> @@ -10669,6 +10672,34 @@ remote_set_trace_notes (char *user, char *notes, char *stop_notes)
> return 1;
> }
>
> +static int
> +remote_use_agent (int use)
> +{
> + if (remote_protocol_packets[PACKET_QAgent].support != PACKET_DISABLE)
> + {
> + struct remote_state *rs = get_remote_state ();
> +
> + /* If the stub supports QAgent. */
> + 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 1;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int
> +remote_can_use_agent (void)
> +{
> + return (remote_protocol_packets[PACKET_QAgent].support != PACKET_DISABLE);
> +}
> +
> static void
> init_remote_ops (void)
> {
> @@ -10778,6 +10809,8 @@ 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;
> + remote_ops.to_can_use_agent = remote_can_use_agent;
> }
>
> /* Set up the extended remote vector by making a copy of the standard
> @@ -11286,6 +11319,9 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
> add_packet_config_cmd (&remote_protocol_packets[PACKET_QDisableRandomization],
> "QDisableRandomization", "disable-randomization", 0);
>
> + add_packet_config_cmd (&remote_protocol_packets[PACKET_QAgent],
> + "QAgent", "agent", 0);
> +
> /* Keep the old ``set remote Z-packet ...'' working. Each individual
> Z sub-packet has its own set and show commands, but users may
> have sets to this variable in their .gdbinit files (or in their
> diff --git a/gdb/target.c b/gdb/target.c
> index ad304bc..a7c11c3 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -698,6 +698,8 @@ 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_can_use_agent, t);
> INHERIT (to_magic, t);
> /* Do not inherit to_memory_map. */
> /* Do not inherit to_flash_erase. */
> @@ -925,6 +927,12 @@ 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_can_use_agent,
> + (int (*) (void))
> + return_zero);
> de_fault (to_execution_direction, default_execution_direction);
>
> #undef de_fault
> diff --git a/gdb/target.h b/gdb/target.h
> index d4605ae..3bc9428 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -832,6 +832,13 @@ 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 1
> + successful, 0 otherwise. */
> + int (*to_use_agent) (int use);
> +
> + /* Is the target able to use agent in current state? */
> + int (*to_can_use_agent) (void);
> +
> int to_magic;
> /* Need sub-structure for target machine related rather than comm related?
> */
> @@ -1663,6 +1670,12 @@ extern char *target_fileio_read_stralloc (const char *filename);
> #define target_traceframe_info() \
> (*current_target.to_traceframe_info) ()
>
> +#define target_use_agent(use) \
> + (*current_target.to_use_agent) (use)
> +
> +#define target_can_use_agent() \
> + (*current_target.to_can_use_agent) ()
> +
> /* Command logging facility. */
>
> #define target_log_command(p) \
> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index 37e1f52..c56a02c 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -52,6 +52,7 @@
> #include "memrange.h"
> #include "exceptions.h"
> #include "cli/cli-utils.h"
> +#include "agent.h"
>
> /* readline include files */
> #include "readline/readline.h"
> @@ -4882,6 +4883,17 @@ info_static_tracepoint_markers_command (char *arg, int from_tty)
> struct ui_out *uiout = current_uiout;
> int i;
>
> + if (target_can_use_agent () == 0)

Hmm, I don't think doing this change now is right. If you connect GDB to a GDBserver
that hasn't been patched, this will fail because that GDBserver doesn't report
the QAgent feature, so it is an incompatible change of the protocol to require QAgent
for static tracepoint listing.
The right thing to do seems to be to just go ahead and try the target method call,
as before. We could at a minimum check whether the "statictracepoints" qSupported
feature Was reported by the target, though that doesn't tell you whether the current
inferior has the IPA or UST loaded at all, only that the frontend stub understands
static tracepoints. What was the motivation for these checks here?

> + {
> + warning (_("Can't list markers when agent doesn't work"));
> + return;
> + }
> + if (use_agent == 0)
> + {
> + warning (_("Agent is off. Run `set agent on'."));
> + return;
> + }
> +
> old_chain
> = make_cleanup_ui_out_table_begin_end (uiout, 5, -1,
> "StaticTracepointMarkersTable");

--
Pedro Alves