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

  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.

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.
---
 gdb/ChangeLog    |   14 ++++++++++
 gdb/probe.c      |   76 +++++++++++++++++++++++++++++++++++++++++++++++-------
 gdb/probe.h      |    8 +++++-
 gdb/stap-probe.c |   12 ++++++++-
 4 files changed, 98 insertions(+), 12 deletions(-)
  

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.
  

On 10/31/2014 06:56 PM, Sergio Durigan Junior wrote:
> 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.

Not sure I understood it (and probably doesn't matter much), seems like
that'd waste memory storing the same info over and over?

Thanks,
Pedro Alves
  

On Friday, November 14 2014, Pedro Alves wrote:

> On 10/31/2014 06:56 PM, Sergio Durigan Junior wrote:
>> 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.
>
> Not sure I understood it (and probably doesn't matter much), seems like
> that'd waste memory storing the same info over and over?

We would be wasting sizeof (char *) for each probe created, indeed, but
I think this is clearer (also for debugging purposes) than having all
the necessary machinery to call a function that will return the same
info...

Anyway, I won't oppose the current approach too, I was just curious :-P.
  

On 11/14/2014 11:08 PM, Sergio Durigan Junior wrote:
> On Friday, November 14 2014, Pedro Alves wrote:
> 
>> On 10/31/2014 06:56 PM, Sergio Durigan Junior wrote:
>>> 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.
>>
>> Not sure I understood it (and probably doesn't matter much), seems like
>> that'd waste memory storing the same info over and over?
> 
> We would be wasting sizeof (char *) for each probe created, indeed, but
> I think this is clearer (also for debugging purposes) than having all
> the necessary machinery to call a function that will return the same
> info...

IMO, that's not cleaner, because the type name is a property of
the type, not of the instance, so it makes sense to get it from the
type (thus through an ops method).  Writing the equivalent C++ may
make it clearer.  Something like:

struct probe
{
...
   virtual const char *type_name () = 0;
};

struct stap_probe : public probe
{
...
   virtual const char *type_name () { return "stap"; }
};

struct dtrace_probe : public probe
{
...
   virtual const char *type_name () { return "dtrace"; }
};


And then in info_probes_for_ops:

      const char *probe_type = probe->probe->type_name ();

> 
> Anyway, I won't oppose the current approach too, I was just curious :-P.


Thanks,
Pedro Alves
  

On Monday, November 17 2014, Pedro Alves wrote:

> On 11/14/2014 11:08 PM, Sergio Durigan Junior wrote:
>> On Friday, November 14 2014, Pedro Alves wrote:
>> 
>>> On 10/31/2014 06:56 PM, Sergio Durigan Junior wrote:
>>>> 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.
>>>
>>> Not sure I understood it (and probably doesn't matter much), seems like
>>> that'd waste memory storing the same info over and over?
>> 
>> We would be wasting sizeof (char *) for each probe created, indeed, but
>> I think this is clearer (also for debugging purposes) than having all
>> the necessary machinery to call a function that will return the same
>> info...
>
> IMO, that's not cleaner, because the type name is a property of
> the type, not of the instance, so it makes sense to get it from the
> type (thus through an ops method).  Writing the equivalent C++ may
> make it clearer.  Something like:

Fair enough, looking from this perspective I am convinced.

> struct probe
> {
> ...
>    virtual const char *type_name () = 0;
> };
>
> struct stap_probe : public probe
> {
> ...
>    virtual const char *type_name () { return "stap"; }
> };
>
> struct dtrace_probe : public probe
> {
> ...
>    virtual const char *type_name () { return "dtrace"; }
> };
>
>
> And then in info_probes_for_ops:
>
>       const char *probe_type = probe->probe->type_name ();
>
>> 
>> Anyway, I won't oppose the current approach too, I was just curious :-P.
>
>
> Thanks,
> Pedro Alves