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