Re: [PATCH V3 1/9] Adapt `info probes' to support printing probes of different types.

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Tuesday, October 28 2014, Jose E. Marchesi wrote:

> A "probe type" (backend for the probe abstraction implemented in
> probe.[ch]) can extend the information printed by `info probes' by
> defining additional columns.  This means that when `info probes' is
> used to print all the probes regardless of their types, some of the
> columns will be "not applicable" to some of the probes (like, say, the
> Semaphore column only makes sense for SystemTap probes).  This patch
> makes `info probes' fill these slots with "n/a" marks (currently it
> breaks the table) and not include headers for which no actual probe
> has been found in the list of defined probes.
>
> This patch also adds support for a new generic column "Type", that
> displays the type of each probe.  SystemTap probes identify themselves
> as "stap" probes.

Thanks a lot for persevering!  I think we are almost there :-).

> gdb/ChangeLog:
>
>   2014-10-28  Jose E. Marchesi  <jose.marchesi@oracle.com>
>
> 	* stap-probe.c (stap_probe_ops): Add `stap_type_name'.
> 	(stap_type_name): New function.
> 	* probe.h (probe_ops): Added a new probe operation `type_name'.
> 	* probe.c (print_ui_out_not_applicables): New function.
> 	(exists_probe_with_pops): Likewise.
> 	(info_probes_for_ops): Do not include column headers for probe
> 	types for which no probe has been actually found on any object.
> 	Also invoke `print_ui_out_not_applicables' in order to match the
> 	column rows with the header when probes of several types are
> 	listed.
> 	Print the "Type" column.

My personal preference (and I saw Pedro commenting about that a while
ago, so it's not just me) is that we sort the ChangeLog entries
according to the diff.  So in this case, it would be

  * probe.c ...
  * probe.h ...
  * stap-probe.c ...

It makes it easier to follow the flow when you read it.  But it is just
my preference; the ChangeLog is OK the way it is already.

> ---
>  gdb/ChangeLog    |   14 ++++++++++
>  gdb/probe.c      |   76 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  gdb/probe.h      |    8 +++++-
>  gdb/stap-probe.c |   12 ++++++++-
>  4 files changed, 98 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/probe.c b/gdb/probe.c
> index 3b8882e..3151ada 100644
> --- a/gdb/probe.c
> +++ b/gdb/probe.c
> @@ -410,6 +410,31 @@ gen_ui_out_table_header_info (VEC (bound_probe_s) *probes,
>    do_cleanups (c);
>  }
>  
> +/* Helper function to print not-applicable strings for all the extra
> +   columns defined in a probe_ops.  */
> +
> +static void
> +print_ui_out_not_applicables (const struct probe_ops *pops)
> +{
> +  struct cleanup *c;
> +  VEC (info_probe_column_s) *headings = NULL;
> +  info_probe_column_s *column;
> +  int ix;
> +  
> +  if (pops->gen_info_probes_table_header == NULL)
> +    return;
> +
> +  c = make_cleanup (VEC_cleanup (info_probe_column_s), &headings);
> +  pops->gen_info_probes_table_header (&headings);
> +  
> +  for (ix = 0;
> +       VEC_iterate (info_probe_column_s, headings, ix, column);
> +       ++ix)
> +    ui_out_field_string (current_uiout, column->field_name, _("n/a"));
> +  
> +  do_cleanups (c);
> +}
> +
>  /* Helper function to print extra information about a probe and an objfile
>     represented by PROBE.  */
>  
> @@ -482,6 +507,23 @@ get_number_extra_fields (const struct probe_ops *pops)
>    return n;
>  }
>  
> +/* Helper function that returns 1 if there is a probe in PROBES
> +   featuring the given POPS.  It returns 0 otherwise.  */
> +
> +static int
> +exists_probe_with_pops (VEC (bound_probe_s) *probes,
> +			const struct probe_ops *pops)
> +{
> +  struct bound_probe *probe;
> +  int ix;
> +
> +  for (ix = 0; VEC_iterate (bound_probe_s, probes, ix, probe); ++ix)
> +    if (probe->probe->pops == pops)
> +      return 1;
> +
> +  return 0;
> +}
> +
>  /* See comment in probe.h.  */
>  
>  void
> @@ -497,6 +539,7 @@ info_probes_for_ops (const char *arg, int from_tty,
>    size_t size_name = strlen ("Name");
>    size_t size_objname = strlen ("Object");
>    size_t size_provider = strlen ("Provider");
> +  size_t size_type = strlen ("Type");
>    struct bound_probe *probe;
>    struct gdbarch *gdbarch = get_current_arch ();
>  
> @@ -517,6 +560,9 @@ info_probes_for_ops (const char *arg, int from_tty,
>  	}
>      }
>  
> +  probes = collect_probes (objname, provider, probe_name, pops);
> +  make_cleanup (VEC_cleanup (probe_p), &probes);
> +  

Extraneous whitespaces.

>    if (pops == NULL)
>      {
>        const struct probe_ops *po;
> @@ -529,18 +575,18 @@ info_probes_for_ops (const char *arg, int from_tty,
>  
>  	 To do that, we iterate over all probe_ops, querying each one about
>  	 its extra fields, and incrementing `ui_out_extra_fields' to reflect
> -	 that number.  */
> +	 that number.  But note that we ignore the probe_ops for which no probes
> +         are defined with the given search criteria.  */
>  
>        for (ix = 0; VEC_iterate (probe_ops_cp, all_probe_ops, ix, po); ++ix)
> -	ui_out_extra_fields += get_number_extra_fields (po);
> +	if (exists_probe_with_pops (probes, po))
> +	  ui_out_extra_fields += get_number_extra_fields (po);
>      }
>    else
>      ui_out_extra_fields = get_number_extra_fields (pops);
>  
> -  probes = collect_probes (objname, provider, probe_name, pops);
> -  make_cleanup (VEC_cleanup (probe_p), &probes);
>    make_cleanup_ui_out_table_begin_end (current_uiout,
> -				       4 + ui_out_extra_fields,
> +				       5 + ui_out_extra_fields,

I wouldn't oppose a #define naming this number "5" there...  In fact, I
wanted to fix it long ago, but I forgot.

>  				       VEC_length (bound_probe_s, probes),
>  				       "StaticProbes");
>  
> @@ -552,15 +598,19 @@ info_probes_for_ops (const char *arg, int from_tty,
>    /* What's the size of an address in our architecture?  */
>    size_addr = gdbarch_addr_bit (gdbarch) == 64 ? 18 : 10;
>  
> -  /* Determining the maximum size of each field (`provider', `name' and
> -     `objname').  */
> +  /* Determining the maximum size of each field (`type', `provider',
> +     `name' and `objname').  */
>    for (i = 0; VEC_iterate (bound_probe_s, probes, i, probe); ++i)
>      {
> +      const char *probe_type = probe->probe->pops->type_name (probe->probe);
> +      

Extraneous whitespaces.

Also, any particular reason why you did not create a field "type" inside
"struct probe", instead of making a function that returns the string?  I
consider the field would be simpler (i.e., not pollute the probe_ops
structure), and it could be set when creating the probes.

> +      size_type = max (strlen (probe_type), size_type);
>        size_name = max (strlen (probe->probe->name), size_name);
>        size_provider = max (strlen (probe->probe->provider), size_provider);
>        size_objname = max (strlen (objfile_name (probe->objfile)), size_objname);
>      }
>  
> +  ui_out_table_header (current_uiout, size_type, ui_left, "type", _("Type"));
>    ui_out_table_header (current_uiout, size_provider, ui_left, "provider",
>  		       _("Provider"));
>    ui_out_table_header (current_uiout, size_name, ui_left, "name", _("Name"));
> @@ -571,10 +621,12 @@ info_probes_for_ops (const char *arg, int from_tty,
>        const struct probe_ops *po;
>        int ix;
>  
> -      /* We have to generate the table header for each new probe type that we
> -	 will print.  */
> +      /* We have to generate the table header for each new probe type
> +	 that we will print.  Note that this excludes probe types not
> +	 having any defined probe with the search criteria.  */
>        for (ix = 0; VEC_iterate (probe_ops_cp, all_probe_ops, ix, po); ++ix)
> -	gen_ui_out_table_header_info (probes, po);
> +	if (exists_probe_with_pops (probes, po))
> +	  gen_ui_out_table_header_info (probes, po);
>      }
>    else
>      gen_ui_out_table_header_info (probes, pops);
> @@ -586,9 +638,11 @@ info_probes_for_ops (const char *arg, int from_tty,
>    for (i = 0; VEC_iterate (bound_probe_s, probes, i, probe); ++i)
>      {
>        struct cleanup *inner;
> +      const char *probe_type = probe->probe->pops->type_name (probe->probe);
>  
>        inner = make_cleanup_ui_out_tuple_begin_end (current_uiout, "probe");
>  
> +      ui_out_field_string (current_uiout, "type",probe_type);
>        ui_out_field_string (current_uiout, "provider", probe->probe->provider);
>        ui_out_field_string (current_uiout, "name", probe->probe->name);
>        ui_out_field_core_addr (current_uiout, "addr",
> @@ -604,6 +658,8 @@ info_probes_for_ops (const char *arg, int from_tty,
>  	       ++ix)
>  	    if (probe->probe->pops == po)
>  	      print_ui_out_info (probe->probe);
> +	    else if (exists_probe_with_pops (probes, po))
> +	      print_ui_out_not_applicables (po);
>  	}
>        else
>  	print_ui_out_info (probe->probe);
> diff --git a/gdb/probe.h b/gdb/probe.h
> index a128151..66c8c53 100644
> --- a/gdb/probe.h
> +++ b/gdb/probe.h
> @@ -50,7 +50,7 @@ DEF_VEC_O (info_probe_column_s);
>  /* Operations associated with a probe.  */
>  
>  struct probe_ops
> -  {
> +{

This hunk is reindenting code, so it shouldn't be in this patch.

>      /* Method responsible for verifying if LINESPECP is a valid linespec for
>         a probe breakpoint.  It should return 1 if it is, or zero if it is not.
>         It also should update LINESPECP in order to discard the breakpoint
> @@ -113,6 +113,12 @@ struct probe_ops
>  
>      void (*destroy) (struct probe *probe);
>  
> +    /* Return a pointer to a name identifying the probe type.  This is
> +       the string that will be displayed in the "Type" column of the
> +       `info probes' command.  */
> +
> +    const char *(*type_name) (struct probe *probe);
> +  
>      /* Function responsible for providing the extra fields that will be
>         printed in the `info probes' command.  It should fill HEADS
>         with whatever extra fields it needs.  If the backend doesn't need
> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
> index 3902997..061f6d3 100644
> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -1703,6 +1703,15 @@ stap_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
>      }
>  }
>  
> +/* Implementation of the type_name method.  */
> +
> +static const char *
> +stap_type_name (struct probe *probe)
> +{
> +  gdb_assert (probe->pops == &stap_probe_ops);
> +  return "stap";
> +}
> +
>  static int
>  stap_probe_is_linespec (const char **linespecp)
>  {
> @@ -1743,7 +1752,7 @@ stap_gen_info_probes_table_values (struct probe *probe_generic,
>  /* SystemTap probe_ops.  */
>  
>  static const struct probe_ops stap_probe_ops =
> -{
> +  {
>    stap_probe_is_linespec,
>    stap_get_probes,
>    stap_get_probe_address,
> @@ -1754,6 +1763,7 @@ static const struct probe_ops stap_probe_ops =
>    stap_set_semaphore,
>    stap_clear_semaphore,
>    stap_probe_destroy,
> +  stap_type_name,
>    stap_gen_info_probes_table_header,
>    stap_gen_info_probes_table_values,
>  };
> -- 
> 1.7.10.4

Aside from the comment about putting the "type" inside struct probe, the
patch looks good to me.

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