This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH V3 1/9] Adapt `info probes' to support printing probes of different types.
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: "Jose E. Marchesi" <jose dot marchesi at oracle dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 31 Oct 2014 14:56:23 -0400
- Subject: Re: [PATCH V3 1/9] Adapt `info probes' to support printing probes of different types.
- Authentication-results: sourceware.org; auth=none
- References: <1414504218-31204-1-git-send-email-jose dot marchesi at oracle dot com> <1414504218-31204-2-git-send-email-jose dot marchesi at oracle 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/