From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2338 invoked by alias); 23 Nov 2016 13:06:27 -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 2276 invoked by uid 89); 23 Nov 2016 13:06:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.8 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=na, subheading, @subheading, UD:A 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 ESMTP; Wed, 23 Nov 2016 13:06:15 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D575380510; Wed, 23 Nov 2016 13:06:14 +0000 (UTC) Received: from [127.0.0.1] (ovpn03.gateway.prod.ext.phx2.redhat.com [10.5.9.3]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAND6D12022614; Wed, 23 Nov 2016 08:06:14 -0500 Subject: Re: [PATCH 3/3] Add -file-list-shared-libraries MI command To: Marc-Andre Laperle , gdb-patches@sourceware.org References: <1473712054-30417-1-git-send-email-marc-andre.laperle@ericsson.com> <1473712054-30417-3-git-send-email-marc-andre.laperle@ericsson.com> From: Pedro Alves Message-ID: <176aae27-f7c3-c563-141d-35eb1f69ee02@redhat.com> Date: Wed, 23 Nov 2016 13:06:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1473712054-30417-3-git-send-email-marc-andre.laperle@ericsson.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-11/txt/msg00669.txt.bz2 Hi Marc-Andre, Great that you're tackling this. Thanks much for digging into GDB. :-) I like this. As for following GDB's code standards, it's almost perfect. But I have questions on the MI output, though. See below. On 09/12/2016 09:27 PM, Marc-Andre Laperle wrote: > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index d1a5e7c..c6b2133 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -31401,26 +31401,35 @@ The @value{GDBN} equivalent is @samp{info sources}. > (gdb) > @end smallexample > > -@ignore > @subheading The @code{-file-list-shared-libraries} Command > @findex -file-list-shared-libraries > > @subsubheading Synopsis > > @smallexample > - -file-list-shared-libraries > + -file-list-shared-libraries [ @var{regexp} ] > @end smallexample > > List the shared libraries in the program. > +With a regular expression @var{regexp}, only those libraries whose > +names match @var{regexp} are listed. > > @subsubheading @value{GDBN} Command > > The corresponding @value{GDBN} command is @samp{info shared}. > > @subsubheading Example > -N.A. > +@smallexample > +(gdb) > +-file-list-exec-source-files > +^done,shared-libraries=[ > +@{from="0x72815989",to="0x728162c0",syms-read="1",name="/lib/libfoo.so"@}, > +@{from="0x76ee48c0",to="0x76ee9160",syms-read="1",name="/lib/libbar.so"@}] I don't see where the documentation describes what the attributes of each list element are. Sorry if I missed it. I find it surprising that the attributes output don't match attributes output by =library-loaded ? I'd think they should match. That'd simplify the documentation too, as one place would refer to the other for attributes list. BTW, =library-loaded doesn't output from/to. ISTR that that was discussed and left out, because from/to assume there's a contiguous range to report, while that's not true on all targets (i.e., assumes a single segment). So it seems to me that that's a separate discussion/patch would better be addressed in both places. > +void > +mi_cmd_file_list_shared_libraries (char *command, char **argv, int argc) > +{ > + struct ui_out *uiout = current_uiout; > + const char *pattern; > + struct so_list *so = NULL; > + struct gdbarch *gdbarch = target_gdbarch (); > + > + switch (argc) > + { > + case 0: > + pattern = NULL; > + break; > + case 1: > + pattern = argv[0]; > + break; > + default: > + error (_("Usage: -file-list-shared-libraries [REGEXP]")); > + break; error doesn't return. No need for the break. > + } > + > + if (pattern != NULL) > + { > + char *re_err = re_comp (pattern); > + > + if (re_err != NULL) > + error (_("Invalid regexp: %s"), re_err); > + } > + > + update_solib_list (1); > + > + /* Print the table header. */ > + ui_out_begin (uiout, ui_out_type_list, "shared-libraries"); > + > + ALL_SO_LIBS (so) > + { > + if (so->so_name[0] == '\0') > + continue; > + if (pattern != NULL && !re_exec (so->so_name)) > + continue; > + > + ui_out_begin (uiout, ui_out_type_tuple, NULL); > + > + if (so->addr_high != 0) > + { > + ui_out_field_core_addr (uiout, "from", gdbarch, so->addr_low); > + ui_out_field_core_addr (uiout, "to", gdbarch, so->addr_high); > + } > + else > + { > + ui_out_field_skip (uiout, "from"); > + ui_out_field_skip (uiout, "to"); > + } > + > + ui_out_field_int (uiout, "syms-read", so->symbols_loaded ? 1 : 0); > + > + ui_out_field_string (uiout, "name", so->so_name); So seems to me that the inner body of this loop would be better calling a function that is shared with =library-loaded. > + > --- a/gdb/solist.h > +++ b/gdb/solist.h > @@ -23,6 +23,11 @@ > /* For domain_enum domain. */ > #include "symtab.h" > > +#define ALL_SO_LIBS(so) \ > + for (so = current_program_space->so_list; \ > + so; \ Write explicit 'so != NULL'. > + so = so->next) > + > /* Forward declaration for target specific link map information. This > struct is opaque to all but the target specific file. */ > struct lm_info; > diff --git a/gdb/testsuite/gdb.mi/mi-solib.exp b/gdb/testsuite/gdb.mi/mi-solib.exp > index 2227987..4c40ba4 100644 > --- a/gdb/testsuite/gdb.mi/mi-solib.exp > +++ b/gdb/testsuite/gdb.mi/mi-solib.exp > @@ -48,27 +48,47 @@ if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != "" > > mi_delete_breakpoints > mi_gdb_reinitialize_dir $srcdir/$subdir > -mi_gdb_reinitialize_dir $srcdir/$subdir > mi_gdb_load ${binfile} > > mi_load_shlibs $binfile_lib > > -mi_gdb_test "777-gdb-set stop-on-solib-events 1" "777\\^done" \ > - "set stop-on-solib-events" > +proc test_stop_on_solib_events {} { Looks like a good candidate for the the new proc_with_prefix. > + mi_gdb_test "777-gdb-set stop-on-solib-events 1" "777\\^done" \ > + "set stop-on-solib-events" > > -# We use "run" rather than "-exec-run" here in order to test that CLI > -# commands still cause the correct MI output to be generated. > -mi_run_with_cli > + # We use "run" rather than "-exec-run" here in order to test that CLI > + # commands still cause the correct MI output to be generated. > + mi_run_with_cli > > -# Also test that the CLI solib event note is output. > -set test "CLI prints solib event" > -gdb_expect { > - -re "~\"Stopped due to shared library event \\(no libraries added or removed\\)\\\\n" { > - pass "$test" > - } > - timeout { > - fail "$test (timeout)" > + # Also test that the CLI solib event note is output. > + set test "CLI prints solib event" > + gdb_expect { > + -re "~\"Stopped due to shared library event \\(no libraries added or removed\\)\\\\n" { > + pass "$test" > + } > + timeout { > + fail "$test (timeout)" > + } > } > + > + mi_expect_stop solib-event .* .* .* .* .* "check for solib event" > + > + # Unset solib events to avoid interfering with other tests. > + mi_gdb_test "778-gdb-set stop-on-solib-events 0" "778\\^done" \ > + "unset stop-on-solib-events" > +} > + > +proc test_file_list_shared_libraries {} { > + global libname > + global binfile > + > + mi_continue_to main > + > + mi_gdb_test "222-file-list-shared-libraries" \ > + "222\\^done,shared-libraries=\\\[.*\{from=\".*\",to=\".*\",syms-read=\"1\",name=\".*${libname}.so\"\}.*]" \ > + "Getting a list of shared libraries." Lowercase, no period at end, and use imperative: "get list of shared libraries" Thanks, Pedro Alves