From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 51368 invoked by alias); 20 Oct 2016 05:05:29 -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 50301 invoked by uid 89); 20 Oct 2016 05:05:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 spammy=H*r:112, marc-andre, Laperle, MarcAndre X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 20 Oct 2016 05:05:23 +0000 Received: by simark.ca (Postfix, from userid 112) id ABF511E486; Thu, 20 Oct 2016 01:05:21 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 280421E124; Thu, 20 Oct 2016 01:05:20 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 20 Oct 2016 05:05:00 -0000 From: Simon Marchi To: Marc-Andre Laperle Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 3/3] Add -file-list-shared-libraries MI command In-Reply-To: <1473712054-30417-3-git-send-email-marc-andre.laperle@ericsson.com> References: <1473712054-30417-1-git-send-email-marc-andre.laperle@ericsson.com> <1473712054-30417-3-git-send-email-marc-andre.laperle@ericsson.com> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.0 X-IsSubscribed: yes X-SW-Source: 2016-10/txt/msg00599.txt.bz2 The patch looks good to me in general. I wrote a few minor comments inline. On 2016-09-12 16:27, Marc-Andre Laperle wrote: > @@ -108,3 +111,67 @@ mi_cmd_file_list_exec_source_files (char > *command, char **argv, int argc) > > ui_out_end (uiout, ui_out_type_list); > } > + > +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; > + } > + > + if (pattern != NULL) > + { > + char *re_err = re_comp (pattern); const ? > + > + 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"); It's not a big deal, but you could use make_cleanup_ui_out_list_begin_end, which avoids accidentally forgetting the corresponding ui_out_end. > + > + 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); Same here, but with make_cleanup_ui_out_tuple_begin_end. > + 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"); > + } You might be able to just remove the else. I think ui_out_field_skip is only relevant for tables (which was used in the code that you took inspiration from), since you need to explicitly tell if you want to skip a table cell. For tuples, if you want to omit a field, you just don't emit it. > diff --git a/gdb/solib.h b/gdb/solib.h > index 75490b6..ee621ce 100644 > --- a/gdb/solib.h > +++ b/gdb/solib.h > @@ -73,6 +73,24 @@ extern void no_shared_libraries (char *ignored, int > from_tty); > extern void set_solib_ops (struct gdbarch *gdbarch, > const struct target_so_ops *new_ops); > > +/* Synchronize GDB's shared object list with inferior's. > + > + Extract the list of currently loaded shared objects from the > + inferior, and compare it with the list of shared objects currently > + in GDB's so_list_head list. Edit so_list_head to bring it in sync > + with the inferior's new list. > + > + If we notice that the inferior has unloaded some shared objects, > + free any symbolic info GDB had read about those shared objects. > + > + Don't load symbolic info for any new shared objects; just add them > + to the list, and leave their symbols_loaded flag clear. > + > + If FROM_TTY is non-null, feel free to print messages about what > + we're doing. */ I noticed that this comment, which you moved from the .c to the .h, refers so_list_head. so_list_head is a define present in the .c, so the comment makes less sense now. What about bringing the define to the .h, and at the same time you could use it in the definition of ALL_SO_LIBS? > + 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." For consistency with all tests, I would suggest changing the test name to "get the list of shared libraries" (no period at the end, non-capitalized, neutral verb tense which I don't the name). > } > > -mi_expect_stop solib-event .* .* .* .* .* "check for solib event" > +test_stop_on_solib_events > +test_file_list_shared_libraries > +mi_gdb_exit I often see some tests ending with gdb_exit or mi_gdb_exit, but I don't think it's really required. The code in lib/gdb.exp (e.g. gdb_exit) should take care of that, but I could be mistaken.