From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12470 invoked by alias); 26 Apr 2013 17:33: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 12460 invoked by uid 89); 26 Apr 2013 17:33:38 -0000 X-Spam-SWARE-Status: No, score=-7.7 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS,TW_BJ autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 26 Apr 2013 17:33:37 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3QHXWsB015037 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 26 Apr 2013 13:33:32 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r3QHXUHK000697; Fri, 26 Apr 2013 13:33:31 -0400 Message-ID: <517ABA6A.5070400@redhat.com> Date: Fri, 26 Apr 2013 20:23:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: Nicolas Blanc CC: gdb-patches@sourceware.org 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=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-04/txt/msg00818.txt.bz2 On 04/16/2013 08:51 AM, Nicolas Blanc wrote: > 2013-18-03 Nicolas Blanc > > * breakpoint.c (disable_breakpoints_in_free_objfile): Created > function for disabling breakoints in objfiles upon FREE_OBJFILE > notifications. Present tense "create", etc. (throughout the log entry). Typo breakoints. > * doc/observer.text: Created FREE_OBJFILE event. > * objfiles.c (free_objfile): Notify FREE_OBJFILE. > * printcmd.c (clear_dangling_display_expressions): Act upon FREE_OBJFILE > events instead of SOLIB_UNLOADED events. > (_initialize_printcmd): Register observer for FREE_OBJFILE instead > of SOLIB_UNLOADED notifications. > * solib.c (remove_user_added_objfile): Created function for removing > dangling references upon notification of FREE_OBJFILE. > * symfile.c (add_symbol_file_command): Set OBJFILE->LOW_ADDRESS. LOW_ADDR. > (remove_symbol_file_command): Created command for removing symbol files. > (_initialize_symfile): Added remove-symbol-file. > +/* Upon notification of FREE_OBJFILE remove any reference > + to any user-added file that is about to be freed. */ Why only user-added files? > +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; > + } Or rather/also, this looks a bit weird to me. Can we ever really ever find a user-loaded file in the so_list_head list? What would that mean? IIRC, the only way to get a OBJF_USERLOADED|OBJF_SHARED objfile is through "dll-symbols" (dll_symbol_command), but that doesn't create any entry in the shared library list. BTW, calling the variable "gdb" was probably copied from solib.c:update_solib_list. It makes sense to call it that to contrast with "inferior", but not here. Just call it something like "so". That while loop can be written simpler as a for loop: for (so = so_list_head; so != NULL; so = so->next) if (so->objfile == objfile) so->objfile = NULL; > +static void > +disable_breakpoints_in_free_objfile (struct objfile * objfile) This is clearly mirroring the naming of disable_breakpoints_in_unloaded_shlib. Should be "in freed objfile". "in free objfile" would mean something else. > + /* If the file is a shared library not loaded by the user then > + SOLIB_UNLOADED was notified and DISABLE_BREAKPIONTS_IN_UNLOADED_SHLIB Typo BREAKPIONTS. Actually, uppercase is used when referring to a value of a variable (see GNU coding standards), which is not the case here. > + error (_("USAGE: remove-symbol-file ")); I'd s/low// here. text_address is clear and common enough that having "low" there makes me go "what does low mean here?" Or just
even. > + if (objf->flags & OBJF_USERLOADED && objf->addr_low == addr) As I mentioned, the .text address may not be the lower address in the object at all, so this "addr_low" confused me. I'd be happier with naming the field for what ig really is, something like "add_symbol_file_addr", with a comment indicating this is related to "add-symbol-file", but I see you're reusing an existing variable. At the very least, the variable's definition should gain a comment explaining its overloading for add-symbol-file. -- Pedro Alves