From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12232 invoked by alias); 29 Sep 2014 21:15:59 -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 12222 invoked by uid 89); 29 Sep 2014 21:15:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.6 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; Mon, 29 Sep 2014 21:15:57 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s8TLFsA4013926 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 29 Sep 2014 17:15:54 -0400 Received: from localhost (dhcp-10-15-16-169.yyz.redhat.com [10.15.16.169]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s8TLFr98009311 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Mon, 29 Sep 2014 17:15:54 -0400 From: Sergio Durigan Junior To: "Jose E. Marchesi" Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/9] Adapt `info probes' to support printing probes of different types. References: <1411724905-31234-1-git-send-email-jose.marchesi@oracle.com> <1411724905-31234-2-git-send-email-jose.marchesi@oracle.com> X-URL: http://blog.sergiodj.net Date: Mon, 29 Sep 2014 21:15:00 -0000 In-Reply-To: <1411724905-31234-2-git-send-email-jose.marchesi@oracle.com> (Jose E. Marchesi's message of "Fri, 26 Sep 2014 11:48:17 +0200") Message-ID: <87d2ae9lh2.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-09/txt/msg00855.txt.bz2 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 > > * 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 > + > + * 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/