From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26666 invoked by alias); 29 May 2013 09:38:09 -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 26651 invoked by uid 89); 29 May 2013 09:38:09 -0000 X-Spam-SWARE-Status: No, score=-4.7 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_BJ autolearn=ham version=3.3.1 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 29 May 2013 09:38:08 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1UhcpY-00048m-O6 from Luis_Gustavo@mentor.com ; Wed, 29 May 2013 02:38:04 -0700 Received: from NA1-MAIL.mgc.mentorg.com ([147.34.98.181]) by svr-orw-fem-01.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Wed, 29 May 2013 02:38:04 -0700 Received: from [172.30.64.149] ([172.30.64.149]) by NA1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.3959); Wed, 29 May 2013 02:38:03 -0700 Message-ID: <51A5CC72.2090000@codesourcery.com> Date: Wed, 29 May 2013 09:38:00 -0000 From: Luis Machado Reply-To: lgustavo@codesourcery.com User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: Nicolas Blanc CC: gdb-patches@sourceware.org, palves@redhat.com, tromey@redhat.com, eliz@gnu.org, yao@codesourcery.com Subject: Re: [patch v4 1/3] Create remove-symbol-file command. References: <1369818805-14288-1-git-send-email-nicolas.blanc@intel.com> <1369818805-14288-2-git-send-email-nicolas.blanc@intel.com> In-Reply-To: <1369818805-14288-2-git-send-email-nicolas.blanc@intel.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-05/txt/msg00985.txt.bz2 Hi, A few nits and a suggestion at the end. On 05/29/2013 11:13 AM, Nicolas Blanc wrote: > Create remove-symbol-file command for removing > symbol files added via the add-symbol-file command. > > 2013-18-03 Nicolas Blanc > > * breakpoint.c (is_addr_in_objfile): Create static helper function. Shouldn't this say "New static inline function." instead? > (disable_breakpoints_in_freed_objfile): Create function for disabling > breakpoints in objfiles upon free_objfile notifications. Likewise? > * solib.c (remove_user_added_objfile): Create function for removing > dangling references upon notification of free_objfile. Likewise? > * symfile.c (add_symbol_file_command): Set OBJFILE->LOW_ADDR. > (remove_symbol_file_command): Create command for removing symbol files. Likewise? > +/* Return 1 if ADDR maps in one of the sections of OBJFILE and 0 > + otherwise. OBJFILE must be valid. */ "maps into one of the sections" maybe? > + > +static inline int > +is_addr_in_objfile (CORE_ADDR addr, const struct objfile * objfile) > +{ > + struct obj_section *osect; If OBJFILE must be valid, how about calling gdb_assert() here to ensure it is indeed valid? Unless not having a valid OBJFILE is a possibility. > diff --git a/gdb/objfiles.h b/gdb/objfiles.h > index 93149e2..644e780 100644 > --- a/gdb/objfiles.h > +++ b/gdb/objfiles.h > @@ -204,6 +204,10 @@ struct objfile > > char *name; > > + /* The base address of the object file, which is often assumed to be > + the address of the text section. The remove-symbol-file command > + uses this field to identify the object file to remove. */ > + > CORE_ADDR addr_low; > > /* Some flag bits for this objfile. No space between the variable and its comment block. > diff --git a/gdb/symfile.c b/gdb/symfile.c > index e9609b2..7c5989c 100644 > --- a/gdb/symfile.c > +++ b/gdb/symfile.c > @@ -2175,6 +2175,8 @@ add_symbol_file_command (char *args, int from_tty) > int expecting_sec_name = 0; > int expecting_sec_addr = 0; > char **argv; > + struct objfile *objf; > + CORE_ADDR addr_low; > > struct sect_opt > { > @@ -2307,12 +2309,17 @@ add_symbol_file_command (char *args, int from_tty) > } > section_addrs->num_sections = sec_num; > > + addr_low = section_addrs->other[0].addr; > + Looks a little cryptic without a comment stating what this is/will be used for. > + > +/* This function removes a symbol file that was added via add-symbol-file. */ > + > +static void > +remove_symbol_file_command (char *args, int from_tty) > +{ > + CORE_ADDR addr = 0; > + struct objfile* objf; > + struct gdbarch *gdbarch = get_current_arch (); > + > + dont_repeat (); > + > + if (args == NULL) > + error (_("USAGE: remove-symbol-file ")); > + > + addr = parse_and_eval_address (args); > + > + ALL_OBJFILES (objf) > + { > + if (objf->flags & OBJF_USERLOADED && objf->addr_low == addr) > + break; > + } > + > + if (objf == NULL) > + error (_("no user-added symbol file for .text_addr = 0x%s"), > + phex_nz (addr, sizeof (addr))); > + > + printf_unfiltered (_("Remove symbol table from file \"%s\"\ > + at .text_addr = %s\n"), > + objf->name, paddress (gdbarch, addr)); > + > + if (from_tty && (!query ("%s", ""))) > + error (_("Not confirmed.")); > + > + free_objfile (objf); > + clear_symtab_users (0); > + > +} Empty line before the end of the function should be gone. > @@ -3734,6 +3781,12 @@ with the text. SECT is a section name to be loaded at SECT_ADDR."), > &cmdlist); > set_cmd_completer (c, filename_completer); > > + c = add_cmd ("remove-symbol-file", class_files, > + remove_symbol_file_command, _("\ > +Remove a symbol file loaded via the add-symbol-file command.\n\ > +Usage: remove-symbol-file ADDR.\nADDR is the starting address of the\ > + text section of the file to remove."), &cmdlist); > + > c = add_cmd ("load", class_files, load_command, _("\ > Dynamically load FILE into the running program, and record its symbols\n\ > for access from GDB.\n\ > I'm thinking, with a command called "remove-symbol-file" i would expect to provide some kind of filename to this command. Should the user also be able to state a DSO name here and have it unloaded? Maybe have the name translated to the base address used to unload the library internally? Luis