Re: [PATCH 6/9] agent capability of static tracepoint

[Yao Qi <yao@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 3120 bytes --]

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 (齐尧)

[-- Attachment #2: 0006-agent-capability-of-static-tracepoint.patch --]
[-- Type: text/x-patch, Size: 2662 bytes --]


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