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

Sergio Durigan Junior
2014-09-29T21:15:59Z

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