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

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

On Friday, September 26 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.

Thanks for the patch, Jose.  Comments below.

> gdb:
>
> 2014-09-25  Jose E. Marchesi  <jose.marchesi@oracle.com>
>
> 	* probe.c (print_ui_out_not_applicables): New function.
>         (get_num_probes_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.
> ---
>  gdb/ChangeLog |   10 +++++++++
>  gdb/probe.c   |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 67 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index dbd222d..7cc4f00 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,13 @@
> +2014-09-25  Jose E. Marchesi  <jose.marchesi@oracle.com>
> +
> +	* probe.c (print_ui_out_not_applicables): New function.
> +	(get_num_probes_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.
> +

BTW, we usually don't include ChangeLog diff's in the message, because
they make it hard to apply the patch after some time (even hours!).  I
am mentioning this because I had several conflicts when I applied yours
here ;-).  (Of course, one can always delete the ChangeLog diff's from
the patches before applying them, but it is also annoying).

> diff --git a/gdb/probe.c b/gdb/probe.c
> index 859e6e7..5458372 100644
> --- a/gdb/probe.c
> +++ b/gdb/probe.c
> @@ -411,6 +411,33 @@ 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"));
> +    }

You can remove the brackets, since it's a one-statement for.

> +  
> +  do_cleanups (c);
> +}
> +
>  /* Helper function to print extra information about a probe and an objfile
>     represented by PROBE.  */
>  
> @@ -483,6 +510,23 @@ get_number_extra_fields (const struct probe_ops *pops)
>    return n;
>  }
>  
> +/* Helper function that returns the number of probes in PROBES having
> +   the given POPS.  */
> +
> +static int
> +get_num_probes_with_pops (VEC (bound_probe_s) *probes,
> +			  const struct probe_ops *pops)
> +{
> +  int res = 0;
> +  struct bound_probe *probe;
> +  int ix;
> +
> +  for (ix = 0; VEC_iterate (bound_probe_s, probes, ix, probe); ++ix)
> +    res += (probe->probe->pops == pops);

Please, remove the parens, we don't use them in this case.

> +
> +  return res;
> +}

Also, this function could be simplified.  You are not using the count
anywhere, so why not just look for the first match of
"probe->probe->pops == pops", and return 1 if it is found (0 otherwise)?
The function name should be adjusted in this case.

> +
>  /* See comment in probe.h.  */
>  
>  void
> @@ -518,6 +562,8 @@ info_probes_for_ops (const char *arg, int from_tty,
>  	}
>      }
>  
> +  probes = collect_probes (objname, provider, probe_name, pops);
> +  
>    if (pops == NULL)
>      {
>        const struct probe_ops *po;
> @@ -530,15 +576,16 @@ 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 (get_num_probes_with_pops (probes, po))
> +	  ui_out_extra_fields += get_number_extra_fields (po);

Here is the only place you are doing an implicit comparison.  I was
going to tell you to compare explicitly, i.e., "get_num_probes_with_pops
(probes, po) > 0", but when you implement the simplification I suggested
above, there is no need anymore.

>      }
>    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);

This call to "make_cleanup" should be moved up, too.

>    make_cleanup_ui_out_table_begin_end (current_uiout,
>  				       4 + ui_out_extra_fields,
> @@ -572,10 +619,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 (get_num_probes_with_pops (probes, po) > 0)
> +	  gen_ui_out_table_header_info (probes, po);
>      }
>    else
>      gen_ui_out_table_header_info (probes, pops);
> @@ -605,6 +654,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 (get_num_probes_with_pops (probes, po) > 0)
> +	      print_ui_out_not_applicables (po);

Hm, this loop is kind of confusing.  I have a few ideas to make it
clearer, but that's for another patch :-).

>  	}

Both places will need to be adjusted when you simplify the
get_num_probes_with_pops function.

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