Re: [PATCH 3/9] New commands `enable probe' and `disable probe'.

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Friday, September 26 2014, Jose E. Marchesi wrote:

> This patch adds the above-mentioned commands to the generic probe abstraction
> implemented in probe.[ch].  The effects associated to enabling or disabling a
> probe depend on the type of probe being handled, and is triggered by invoking
> two back-end hooks in `probe_ops'.
>
> In case some particular probe type does not support the notion of enabling
> and/or disabling, the corresponding fields on `probe_ops' can be initialized
> to NULL.  This is the case of SystemTap probes.

Thanks for the patch, Jose.  Comments below, as usual.

> gdb:
>
> 2014-09-26  Jose E. Marchesi  <jose.marchesi@oracle.com>
>
> 	* stap-probe.c (stap_probe_ops): Add NULLs in the static
> 	stap_probe_ops for `enable_probe' and `disable_probe'.
>
> 	* probe.c (enable_probes_command): New function.
> 	(disable_probes_command): Likewise.
> 	(_initialize_probe): Define the cli commands `enable probe' and
> 	`disable probe'.
>
> 	* probe.h (probe_ops): New hooks `enable_probe' and
> 	`disable_probe'.

Same comment about blank lines in the ChangeLog entry applies here.

> gdb/doc:
>
> 2014-09-26  Jose E. Marchesi  <jose.marchesi@oracle.com>
>
> 	* gdb.texinfo (Static Probe Points): Cover the `enable probe' and
> 	`disable probe' commands.
> ---
>  gdb/ChangeLog       |   13 ++++++
>  gdb/doc/ChangeLog   |    5 +++
>  gdb/doc/gdb.texinfo |   30 +++++++++++++
>  gdb/probe.c         |  123 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  gdb/probe.h         |   12 +++++
>  gdb/stap-probe.c    |    2 +
>  6 files changed, 185 insertions(+)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 5ec0d85..2f94e05 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,18 @@
>  2014-09-26  Jose E. Marchesi  <jose.marchesi@oracle.com>
>  
> +	* stap-probe.c (stap_probe_ops): Add NULLs in the static
> +	stap_probe_ops for `enable_probe' and `disable_probe'.
> +
> +	* probe.c (enable_probes_command): New function.
> +	(disable_probes_command): Likewise.
> +	(_initialize_probe): Define the cli commands `enable probe' and
> +	`disable probe'.
> +
> +	* probe.h (probe_ops): New hooks `enable_probe' and
> +	`disable_probe'.
> +
> +2014-09-26  Jose E. Marchesi  <jose.marchesi@oracle.com>
> +
>  	* probe.c (compute_probe_arg): Function moved from the probe
>  	backends.
>  	(compile_probe_arg): Likewise.
> diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
> index 9a4bc09..5667bf1 100644
> --- a/gdb/doc/ChangeLog
> +++ b/gdb/doc/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-09-26  Jose E. Marchesi  <jose.marchesi@oracle.com>
> +
> +	* gdb.texinfo (Static Probe Points): Cover the `enable probe' and
> +	`disable probe' commands.
> +
>  2014-09-22  Jan-Benedict Glaw  <jbglaw@lug-owl.de>
>  
>  	* gdb.texinfo (Set Breaks): Add missing "@end table".
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 026706a..4d6dfbc 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -4976,6 +4976,36 @@ given, all object files are considered.
>  List the available static probes, from all types.
>  @end table
>  
> +@cindex enabling and disabling probes
> +Some probe points can be enabled and/or disabled.  The effects
> +associated to enabling or disabling a probe depend on the type of
> +probe being handled. @code{SystemTap} probes do not support these
> +notions.
> +
> +You can enable (or disable) one or more probes using the following
> +commands, with optional arguments:
> +
> +@table @code
> +@kindex enable probes
> +@item enable probes @r{[}@var{provider} @r{[}@var{name} @r{[}@var{objfile}@r{]}@r{]}@r{]}
> +If given, @var{provider} is a regular expression used to match against
> +provider names when selecting which probes to enable.  If omitted,
> +probes by all probes from all providers are enabled.
> +
> +If given, @var{name} is a regular expression to match against probe
> +names when selecting which probes to enable.  If omitted, probe names
> +are not considered when deciding whether to enable them.
> +
> +If given, @var{objfile} is a regular expression used to select which
> +object files (executable or shared libraries) to examine.  If not
> +given, all object files are considered.
> +
> +@kindex disable probes
> +@item disable probes @r{[}@var{provider} @r{[}@var{name} @r{[}@var{objfile}@r{]}@r{]}@r{]}
> +See the @code{enable probes} command above for a description of the
> +optional arguments accepted by this command.
> +@end table
> +
>  @vindex $_probe_arg@r{, convenience variable}
>  A probe may specify up to twelve arguments.  These are available at the
>  point at which the probe is defined---that is, when the current PC is
> diff --git a/gdb/probe.c b/gdb/probe.c
> index d218f00..1e5c460 100644
> --- a/gdb/probe.c
> +++ b/gdb/probe.c
> @@ -685,6 +685,106 @@ info_probes_command (char *arg, int from_tty)
>    info_probes_for_ops (arg, from_tty, NULL);
>  }
>  
> +/* Implementation of the `enable probes' command.  */
> +
> +static void
> +enable_probes_command (char *arg, int from_tty)
> +{
> +  char *provider, *probe_name = NULL, *objname = NULL;
> +  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
> +  VEC (bound_probe_s) *probes;
> +  struct bound_probe *probe;
> +  int i;
> +
> +  /* Do we have a `provider:probe:objfile' style of linespec?  */
> +  provider = extract_arg (&arg);
> +  if (provider)
> +    {
> +      make_cleanup (xfree, provider);
> +
> +      probe_name = extract_arg (&arg);
> +      if (probe_name)
> +	{
> +	  make_cleanup (xfree, probe_name);
> +
> +	  objname = extract_arg (&arg);
> +	  if (objname)
> +	    make_cleanup (xfree, objname);
> +	}
> +    }

This construct has become useful enough that it can be put in a
function, probably.  Would you mind doing that?

> +  probes = collect_probes (objname, provider, probe_name, NULL /* pops */);

When I code, I try not to put comments inside the parentheses.  You can
put a comment above the function, or just remove it at all (there is
another call to collect_probes that doesn't have a comment).

> +  if (VEC_empty (bound_probe_s, probes))
> +    {
> +      ui_out_message (current_uiout, 0, _("No probes matched.\n"));
> +      return;
> +    }

You should call do_cleanups before returning here (and in other places).

> +  /* Enable the selected probes, provided their backends support the
> +     notion of enabling a probe.  */
> +  for (i = 0; VEC_iterate (bound_probe_s, probes, i, probe); ++i)
> +    {
> +      const struct probe_ops *pops = probe->probe->pops;
> +      if (pops->enable_probe)

Newline between variable declaration and code.  You are also comparing
things implicitly, and we do it explicitly:

  if (pops->enable_probe != NULL)

> +	{
> +	  pops->enable_probe (probe->probe);
> +	  ui_out_message (current_uiout, 0,
> +			  _("Probe %s:%s enabled.\n"),
> +			  probe->probe->provider, probe->probe->name);
> +	}
> +    }

I would appreciate a warning here if pops->enable_probe == NULL, saying
that the probe cannot be enabled/disabled.

> +}
> +
> +/* Implementation of the `disable probes' command.  */
> +
> +static void
> +disable_probes_command (char *arg, int from_tty)
> +{
> +  char *provider, *probe_name = NULL, *objname = NULL;
> +  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
> +  VEC (bound_probe_s) *probes;
> +  struct bound_probe *probe;
> +  int i;
> +
> +  /* Do we have a `provider:probe:objfile' style of linespec?  */
> +  provider = extract_arg (&arg);
> +  if (provider)
> +    {
> +      make_cleanup (xfree, provider);
> +
> +      probe_name = extract_arg (&arg);
> +      if (probe_name)
> +	{
> +	  make_cleanup (xfree, probe_name);
> +
> +	  objname = extract_arg (&arg);
> +	  if (objname)
> +	    make_cleanup (xfree, objname);
> +	}
> +    }
> +
> +  probes = collect_probes (objname, provider, probe_name, NULL /* pops */);
> +  if (VEC_empty (bound_probe_s, probes))
> +    {
> +      ui_out_message (current_uiout, 0, _("No probes matched.\n"));
> +      return;
> +    }
> +  
> +  /* Disable the selected probes, provided their backends support the
> +     notion of enabling a probe.  */
> +  for (i = 0; VEC_iterate (bound_probe_s, probes, i, probe); ++i)
> +    {
> +      const struct probe_ops *pops = probe->probe->pops;
> +      if (pops->disable_probe)
> +	{
> +	  pops->disable_probe (probe->probe);
> +	  ui_out_message (current_uiout, 0,
> +			  _("Probe %s:%s disabled.\n"),
> +			  probe->probe->provider, probe->probe->name);
> +	}
> +    }  
> +}

Same comments for enable_probes_command apply here as well.

> +
>  /* See comments in probe.h.  */
>  
>  CORE_ADDR
> @@ -946,4 +1046,27 @@ _initialize_probe (void)
>  	   _("\
>  Show information about all type of probes."),
>  	   info_probes_cmdlist_get ());
> +
> +  add_cmd ("probes", no_class, enable_probes_command, _("\
> +Enable probes.\n\
> +Usage: enable probes [PROVIDER [NAME [OBJECT]]]\n\
> +Each argument is a regular expression, used to select probes.\n\
> +PROVIDER matches probe provider names.\n\
> +NAME matches the probe names.\n\
> +OBJECT matches the executable or shared library name.\n\
> +If you do not specify any argument then the command will disable\n\
> +all defined probes."),
> +	   &enablelist);
> +
> +  add_cmd ("probes", no_class, disable_probes_command, _("\
> +Disable probes.\n\
> +Usage: disable probes [PROVIDER [NAME [OBJECT]]]\n\
> +Each argument is a regular expression, used to select probes.\n\
> +PROVIDER matches probe provider names.\n\
> +NAME matches the probe names.\n\
> +OBJECT matches the executable or shared library name.\n\
> +If you do not specify any argument then the command will disable\n\
> +all defined probes."),
> +	   &disablelist);

Those commands work on probes, which are special kinds of breakpoints,
so I would specify class_breakpoint instead of no_class.

> +
>  }
> diff --git a/gdb/probe.h b/gdb/probe.h
> index b4ff0a6..3f27d0d 100644
> --- a/gdb/probe.h
> +++ b/gdb/probe.h
> @@ -132,6 +132,18 @@ struct probe_ops
>  
>      void (*gen_info_probes_table_values) (struct probe *probe,
>  					  VEC (const_char_ptr) **values);
> +
> +    /* Enable a probe.  The semantics of "enabling" a probe depend on
> +       the specific backend and the field can be NULL in case enabling
> +       probes is not supported.  */
> +
> +    void (*enable_probe) (struct probe *probe);
> +
> +    /* Disable a probe.  The semantics of "disabling" a probe depend
> +       on the specific backend and the field can be NULL in case
> +       disabling probes is not supported.  */
> +
> +    void (*disable_probe) (struct probe *probe);
>    };
>  
>  /* Definition of a vector of probe_ops.  */
> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
> index 17a73fc..c5d0dbb 100644
> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -1675,6 +1675,8 @@ static const struct probe_ops stap_probe_ops =
>    stap_probe_destroy,
>    stap_gen_info_probes_table_header,
>    stap_gen_info_probes_table_values,
> +  NULL,  /* enable_probe  */
> +  NULL   /* disable_probe  */
>  };
>  
>  /* Implementation of the `info probes stap' command.  */
> -- 
> 1.7.10.4

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/