From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14140 invoked by alias); 31 Oct 2014 18:56:33 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 14131 invoked by uid 89); 31 Oct 2014 18:56:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.4 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 31 Oct 2014 18:56:28 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s9VIuOdF020253 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 31 Oct 2014 14:56:25 -0400 Received: from localhost (dhcp-10-15-16-169.yyz.redhat.com [10.15.16.169]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s9VIuNXA009330 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Fri, 31 Oct 2014 14:56:24 -0400 From: Sergio Durigan Junior To: "Jose E. Marchesi" Cc: gdb-patches@sourceware.org Subject: Re: [PATCH V3 1/9] Adapt `info probes' to support printing probes of different types. References: <1414504218-31204-1-git-send-email-jose.marchesi@oracle.com> <1414504218-31204-2-git-send-email-jose.marchesi@oracle.com> X-URL: http://blog.sergiodj.net Date: Fri, 31 Oct 2014 18:56:00 -0000 In-Reply-To: <1414504218-31204-2-git-send-email-jose.marchesi@oracle.com> (Jose E. Marchesi's message of "Tue, 28 Oct 2014 14:50:10 +0100") Message-ID: <87oass6pbc.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-10/txt/msg00856.txt.bz2 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 > > * 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/