From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14816 invoked by alias); 22 Apr 2013 01:25:38 -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 14807 invoked by uid 89); 22 Apr 2013 01:25:38 -0000 X-Spam-SWARE-Status: No, score=-4.6 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; Mon, 22 Apr 2013 01:25:36 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1UU5Ve-0006HL-Qm from Yao_Qi@mentor.com ; Sun, 21 Apr 2013 18:25:34 -0700 Received: from SVR-ORW-FEM-02.mgc.mentorg.com ([147.34.96.206]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Sun, 21 Apr 2013 18:25:34 -0700 Received: from qiyao.dyndns.org (147.34.91.1) by svr-orw-fem-02.mgc.mentorg.com (147.34.96.168) with Microsoft SMTP Server id 14.2.247.3; Sun, 21 Apr 2013 18:25:19 -0700 Message-ID: <51749196.7040205@codesourcery.com> Date: Mon, 22 Apr 2013 13:32:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 MIME-Version: 1.0 To: Nicolas Blanc CC: Subject: Re: [PATCH 1/3] Added command remove-symbol-file. References: <1366098721-18302-1-git-send-email-nicolas.blanc@intel.com> <1366098721-18302-2-git-send-email-nicolas.blanc@intel.com> In-Reply-To: <1366098721-18302-2-git-send-email-nicolas.blanc@intel.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-SW-Source: 2013-04/txt/msg00654.txt.bz2 On 04/16/2013 03:51 PM, Nicolas Blanc wrote: > Added command remove-symbol-file for removing > symbol files added via the add-symbol-file command. > Nicolas, The patch looks good to me, but I am not the people to approve it. Some nits below, > 2013-18-03 Nicolas Blanc > > * breakpoint.c (disable_breakpoints_in_free_objfile): Created > function for disabling breakoints in objfiles upon FREE_OBJFILE > notifications. > * doc/observer.text: Created FREE_OBJFILE event. Change to doc/observer.texi should be moved to a separate changelog entry. > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index f374368..d5c7b21 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > > +/* Disable any breakpoints and tracepoints in OBJFILE upon > + notification of FREE_OBJFILE. Only apply to enabled breakpoints, ^^ Miss one space after period. > + disabled ones can just stay disabled. */ > + > +static void > +disable_breakpoints_in_free_objfile (struct objfile * objfile) > +{ > + struct bp_location *loc, **locp_tmp; > + > + if (objfile == NULL) > + return; > + > + /* If the file is a shared library not loaded by the user then > + SOLIB_UNLOADED was notified and DISABLE_BREAKPIONTS_IN_UNLOADED_SHLIB > + was called. In that case there is no need to take action again. */ > + if ((objfile->flags & OBJF_SHARED) && !(objfile->flags & OBJF_USERLOADED)) > + return; > + > + ALL_BP_LOCATIONS (loc, locp_tmp) > + { > + struct obj_section *osect; > + CORE_ADDR loc_addr = loc->address; > + > + /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL. */ > + struct breakpoint *b = loc->owner; > + > + if (loc->shlib_disabled != 0) > + continue; > + > + if (objfile->pspace != loc->pspace) > + continue; > + > + if (!is_tracepoint(b)) > + { > + if (b->type != bp_breakpoint > + && b->type != bp_jit_event > + && b->type != bp_hardware_breakpoint) > + continue; > + > + if (loc->loc_type != bp_loc_hardware_breakpoint > + && loc->loc_type != bp_loc_software_breakpoint) > + continue; > + } > + > + ALL_OBJFILE_OSECTIONS (objfile, osect) > + { > + if (obj_section_addr (osect) <= loc_addr > + && loc_addr < obj_section_endaddr (osect)) > + { > + loc->shlib_disabled = 1; > + loc->inserted = 0; > + observer_notify_breakpoint_modified (loc->owner); Nit: this observer will be called multiple times for a single breakpoint LOC->owner, if it has multiple breakpoint locations in this objfile. It is ideal to call observer only once for a breakpoint. Your patch is OK to me as well. > diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi > index adb7085..7b420ea 100644 > --- a/gdb/doc/observer.texi > +++ b/gdb/doc/observer.texi > @@ -138,6 +138,10 @@ Called with @var{objfile} equal to @code{NULL} to indicate > previously loaded symbol table data has now been invalidated. > @end deftypefun > > +@deftypefun void free_objfile (struct objfile *@var{objfile}) > +The symbol file specified by @var{objfile} is about to be freed. I feel that "symbol file" is not accurate. Maybe "object file"? > +@end deftypefun > + > @deftypefun void new_thread (struct thread_info *@var{t}) > The thread specified by @var{t} has been created. > @end deftypefun > index 2cc33d0..e6f62ba 100644 > --- a/gdb/printcmd.c > +++ b/gdb/printcmd.c > @@ -1928,21 +1928,22 @@ disable_display_command (char *args, int from_tty) > an item by re-parsing .exp_string field in the new execution context. */ > > static void > -clear_dangling_display_expressions (struct so_list *solib) > +clear_dangling_display_expressions (struct objfile *objfile) > { > - struct objfile *objfile = solib->objfile; > struct display *d; > + struct program_space *pspace; > > /* With no symbol file we cannot have a block or expression from it. */ > if (objfile == NULL) > return; > + pspace = objfile->pspace; > if (objfile->separate_debug_objfile_backlink) > objfile = objfile->separate_debug_objfile_backlink; > - gdb_assert (objfile->pspace == solib->pspace); > + gdb_assert (objfile->pspace == pspace); > This assert is always true. Probably, we need to move this assert to the callers of "free_objfile" when argument is solib->objfile. > for (d = display_chain; d != NULL; d = d->next) > { > - if (d->pspace != solib->pspace) > + if (d->pspace != pspace) > continue; > > if (lookup_objfile_from_block (d->block) == objfile > @@ -2474,7 +2475,7 @@ _initialize_printcmd (void) > > current_display_number = -1; > > - observer_attach_solib_unloaded (clear_dangling_display_expressions); > + observer_attach_free_objfile (clear_dangling_display_expressions); > > add_info ("address", address_info, > _("Describe where symbol SYM is stored.")); > diff --git a/gdb/solib.c b/gdb/solib.c > index 6978677..b0c2f04 100644 > --- a/gdb/solib.c > +++ b/gdb/solib.c > @@ -1442,6 +1442,30 @@ gdb_bfd_lookup_symbol (bfd *abfd, > return symaddr; > } > > +/* Upon notification of FREE_OBJFILE remove any reference > + to any user-added file that is about to be freed. */ A blank line is needed here. > +static void > +remove_user_added_objfile (struct objfile *objfile) > +{ > + struct so_list *gdb; > + > + if (!objfile) > + return; > + > + if (!(objfile->flags & OBJF_USERLOADED) > + || !(objfile->flags & OBJF_SHARED)) > + return; > + > + gdb = so_list_head; > + while (gdb) > + { > + if (gdb->objfile == objfile) > + gdb->objfile = NULL; > + gdb = gdb->next; > + } > +} > + > diff --git a/gdb/symfile.c b/gdb/symfile.c > index 3e66bd1..273c07a 100644 > --- a/gdb/symfile.c > +++ b/gdb/symfile.c > @@ -2364,6 +2372,56 @@ add_symbol_file_command (char *args, int from_tty) > } > > > + > +/* This function removes a symbol file that was added via add-symbol-file. */ A blank line is needed here. > +static void > +remove_symbol_file_command (char *args, int from_tty) > +{ -- Yao (齐尧)