From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27845 invoked by alias); 29 Apr 2013 19:26:02 -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 27835 invoked by uid 89); 29 Apr 2013 19:26:02 -0000 X-Spam-SWARE-Status: No, score=-6.8 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,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; Mon, 29 Apr 2013 19:26:01 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3TJPr4Y025686 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 29 Apr 2013 15:25:53 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r3TJPp3N016736; Mon, 29 Apr 2013 15:25:52 -0400 Message-ID: <517EC93F.2020409@redhat.com> Date: Tue, 30 Apr 2013 09:28: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: "Blanc, Nicolas" 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> <517ABA6A.5070400@redhat.com> <388084C8C1E6A64FA36AD1D656E4856619DF0224@IRSMSX102.ger.corp.intel.com> In-Reply-To: <388084C8C1E6A64FA36AD1D656E4856619DF0224@IRSMSX102.ger.corp.intel.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-04/txt/msg00882.txt.bz2 On 04/29/2013 06:30 PM, Blanc, Nicolas wrote: > Hi Pedro, > >>> +/* Upon notification of FREE_OBJFILE remove any reference >>> + to any user-added file that is about to be freed. */ >> >> Why only user-added files? > > I choose to restrict the scope of the command to user-added files in order to > limit potential unforeseen side-effects in a first time. This restriction > can be lifted in the future if needed. That's not what I meant. I agree with restrict the scope of the command to user-added files, and I'd probably voice against it if the patch did otherwise. The question is why does this piece of code need to be aware of this? As in, the "free_objfile" notification is called irrespective of the objfile being a user loaded objfile or not. So a reader seeing this has to wonder "okay, then if a !userloaded objfile is freed, what is making sure we don't end up with stale references to that freed objfile, if not this function?" We need at least a comment clarifying this. > >>> +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. > > This is a good question. I added this function because update_solib_list handles the case of user-added files: > > /* Unless the user loaded it explicitly, free SO's objfile. */ > if (gdb->objfile && ! (gdb->objfile->flags & OBJF_USERLOADED) > && !solib_used (gdb)) > free_objfile (gdb->objfile); > > If the user-loaded check above is needed then remove_user_added_objfile() is also needed. > But I don't think it is currently possible that user-loaded files end up in so_list_head either. git blame points at this: http://sourceware.org/ml/gdb-patches/2000-03/msg00273.html Which seems to be the initial implementation for dlclose support, very early shared library support code. There's lots of discussion around that patch, including several different implementations of the support: In both http://sourceware.org/ml/gdb-patches/2000-03/ and: http://sourceware.org/ml/gdb/2000-03/ (look for dlclose and unloading shared objects). An earlier version of the patch didn't have that bit, but I didn't find the rationale for adding it in the final version. The reason I can think of, is that the user may do: (gdb) add-symbol-file /foo/bar.so ADDR and if the program itself later loads /foo/bar.so, then the code that loads the objfile for the DSO finds that an objfile for bar.so has already been loaded, and reuses it. At the time of that initial patch in 2000, the code that checked whether the objfile was already loaded just compared names: /* Have we already loaded this shared object? */ ALL_OBJFILES (so->objfile) { if (strcmp (so->objfile->name, so->so_name) == 0) return 1; } So that seems likely to be the rationale to me. The current code hasn't changed that much. It's in solib_read_symbols: /* Have we already loaded this shared object? */ ALL_OBJFILES (so->objfile) { if (filename_cmp (so->objfile->name, so->so_name) == 0 && so->objfile->addr_low == so->addr_low) break; } if (so->objfile != NULL) break; ... so->objfile = symbol_file_add_from_bfd (so->abfd, flags, sap, OBJF_SHARED, NULL); so->objfile->addr_low = so->addr_low; We just gained the addr_low extra check. However, it still seems possible the solib code reuses a user added objfile... I think we should define precisely what does it mean when the user does add-symbol-file and then the program loads the same file. Sorry, I haven't thought on this as much as I wanted, but I have to go now. I'll possibly respond more tomorrow. > >>> +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. > > Ok > >> + 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. > > Ok, I am using text_addr now. > >> + 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. > > I understand your point. GDB's implementation\doc assume that addr_low is the text address. > It' not only the case for add-symbol-file but also for info shared. > > So I've added the following comment to struct objfile for clarification: > > /* The base address of the object file, which by default is 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; > > I hope this helps. > > Thanks, > > Nicolas > > -- > Pedro Alves > > Intel GmbH > Dornacher Strasse 1 > 85622 Feldkirchen/Muenchen, Deutschland > Sitz der Gesellschaft: Feldkirchen bei Muenchen > Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk > Registergericht: Muenchen HRB 47456 > Ust.-IdNr./VAT Registration No.: DE129385895 > Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052 > -- Pedro Alves