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

[Pedro Alves <palves@redhat.com>]

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