This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 6/9] agent capability of static tracepoint
On 02/17/2012 02:54 AM, Yao Qi wrote:
> This patch is to teach both GDB and GDBserver to check agent's capability on
> static tracepoint, before performing any operations.
>
> gdb:
> 2012-02-15 Yao Qi <yao@codesourcery.com>
>
> * tracepoint.c (info_static_tracepoint_markers_command): Call
> agent_capability_check.
>
> gdb/gdbserver:
> 2012-02-15 Yao Qi <yao@codesourcery.com>
>
> * tracepoint.c (gdb_agent_capability): New global.
> (in_process_agent_loaded_ust): Renamed to
> `in_process_agent_supports_ust'.
> Update callers.
> (in_process_agent_supports_ust): Call agent_capability_check.
> (clear_installed_tracepoints): Assert that agent supports
> agent.
> (install_tracepoint): Call in_process_agent_supports_ust.
> ---
> gdb/gdbserver/tracepoint.c | 25 ++++++++++++++++++-------
> gdb/tracepoint.c | 5 +++++
> 2 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
> index 091af5a..0e1f9ed 100644
> --- a/gdb/gdbserver/tracepoint.c
> +++ b/gdb/gdbserver/tracepoint.c
> @@ -239,10 +239,11 @@ in_process_agent_loaded (void)
> static int read_inferior_integer (CORE_ADDR symaddr, int *val);
>
> /* Returns true if both the in-process agent library and the static
> - tracepoints libraries are loaded in the inferior. */
> + tracepoints libraries are loaded in the inferior, and agent has
> + capability on static tracepoints. */
>
> static int
> -in_process_agent_loaded_ust (void)
> +in_process_agent_supports_ust (void)
> {
> int loaded = 0;
>
> @@ -258,7 +259,10 @@ in_process_agent_loaded_ust (void)
> return 0;
> }
>
> - return loaded;
> + if (loaded)
> + return agent_capability_check (AGENT_CAPA_STATIC_TRACE);
Hmm, this looks backwards. We're reading the existence of a global in
the agent called "ust_loaded", indicating whether it has loaded
ust, and after, we check for the static trace capability. If
"ust_loaded" exists in the agent, then it sure understands static
tracepoints. The right check is:
1. does the agent understand static tracepoints?
2. yes? good. and, is ust loaded perchance?
If the agent doesn't understand AGENT_CAPA_STATIC_TRACE,
then you'd fail right on the ust_loaded read, or some other
mechanism to check whether ust is in fact loaded in the inferior.
> + else
> + return 0;
> }
>
> static void
> @@ -310,7 +314,7 @@ maybe_write_ipa_ust_not_loaded (char *buffer)
> write_e_ipa_not_loaded (buffer);
> return 1;
> }
> - else if (!in_process_agent_loaded_ust ())
> + else if (!in_process_agent_supports_ust ())
> {
> write_e_ust_not_loaded (buffer);
> return 1;
> @@ -2315,6 +2319,10 @@ clear_installed_tracepoints (void)
> ;
> else
> {
> + /* Static tracepoints have been inserted, so agent should have
> + been loaded and working. */
> + gdb_assert (in_process_agent_supports_ust ());
This triggers an extra read off the inferior at each installed tracepoints. Is
it worth it?
> +
> unprobe_marker_at (tpoint->address);
> prev_stpoint = tpoint;
> }
> @@ -2965,7 +2973,8 @@ install_tracepoint (struct tracepoint *tpoint, char *own_buf)
> write_e_ipa_not_loaded (own_buf);
> return;
> }
> - if (tpoint->type == static_tracepoint && !in_process_agent_loaded_ust ())
> + if (tpoint->type == static_tracepoint
> + && !in_process_agent_supports_ust ())
> {
> trace_debug ("Requested a static tracepoint, but static "
> "tracepoints are not supported.");
> @@ -2990,8 +2999,8 @@ install_tracepoint (struct tracepoint *tpoint, char *own_buf)
> }
> else
> {
> - if (tp)
> - tpoint->handle = (void *) -1;
Why do we lose this? This was just cloning another static tracepoint, but
in the static tracepoint case, an installed static tracepoint has a handle == -1
(vs NULL).
> + if (!in_process_agent_supports_ust ())
> + warning ("Agent does not have capability for static tracepoint.");
How did we get so far then? There's that "Requested a static tracepoint, but static..."
check quoted above, above.
> else
This if/else connection appears confused.
> {
> if (probe_marker_at (tpoint->address, own_buf) == 0)
> @@ -7994,6 +8003,8 @@ gdb_agent_helper_thread (void *arg)
> #include <signal.h>
> #include <pthread.h>
>
> +IP_AGENT_EXPORT int gdb_agent_capability = AGENT_CAPA_STATIC_TRACE;
> +
> static void
> gdb_agent_init (void)
> {
> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index c56a02c..c2801f9 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -4893,6 +4893,11 @@ info_static_tracepoint_markers_command (char *arg, int from_tty)
> warning (_("Agent is off. Run `set agent on'."));
> return;
> }
> + if (!agent_capability_check (AGENT_CAPA_STATIC_TRACE))
> + {
> + warning (_("Agent is not capable of operating static tracepoints"));
> + return;
> + }
Same comment as in the other patch. I don't think this is right. Also, does
this work for remote debugging? Who is calling agent_look_up_symbols? gdb
knowing about IPA's internals when remote debugging feels a bit dirty.
>
> old_chain
> = make_cleanup_ui_out_table_begin_end (uiout, 5, -1,
--
Pedro Alves