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/24/2012 05:51 AM, Pedro Alves wrote:
> 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.
>
This logic makes sense to me.
>> > @@ -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?
>
Hmm, I am OK to remove it, to avoid reading from inferior.
>> > @@ -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).
>
>
Sorry, it is a mistake when I split patches. It should be in my
next patch set, which refactor code here a little.
>> > + 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.
>
This part is redundant. Removed.
>> > 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.
>
This chunk is removed, explained in my reply to patch 2/9.
--
Yao (éå)
gdb/gdbserver:
2012-02-24 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.
---
gdb/gdbserver/tracepoint.c | 29 ++++++++++++++++++++---------
1 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 3b6f2f4..a48edaa 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;
@@ -252,13 +253,20 @@ in_process_agent_loaded_ust (void)
return 0;
}
- if (read_inferior_integer (ipa_sym_addrs.addr_ust_loaded, &loaded))
+ if (agent_capability_check (AGENT_CAPA_STATIC_TRACE))
{
- warning ("Error reading ust_loaded in lib");
- return 0;
- }
+ /* Agent understands static tracepoint, then check whether UST is in
+ fact loaded in the inferior. */
+ if (read_inferior_integer (ipa_sym_addrs.addr_ust_loaded, &loaded))
+ {
+ warning ("Error reading ust_loaded in lib");
+ return 0;
+ }
- return loaded;
+ return loaded;
+ }
+ else
+ return 0;
}
static void
@@ -310,7 +318,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;
@@ -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.");
@@ -7988,6 +7997,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)
{
--
1.7.0.4