From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1311 invoked by alias); 26 Nov 2012 17:22:38 -0000 Received: (qmail 1298 invoked by uid 22791); 26 Nov 2012 17:22:37 -0000 X-SWARE-Spam-Status: No, hits=-1.2 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,RCVD_IN_HOSTKARMA_NO,TW_BJ X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 26 Nov 2012 17:22:32 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 4F3401C7E40; Mon, 26 Nov 2012 12:22:30 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id ZbtgIYCnKhOr; Mon, 26 Nov 2012 12:22:30 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id D28C01C7E2E; Mon, 26 Nov 2012 12:22:29 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id BC1F7C270B; Mon, 26 Nov 2012 18:22:27 +0100 (CET) Date: Mon, 26 Nov 2012 17:22:00 -0000 From: Joel Brobecker To: Pierre Muller , Tom Tromey Cc: 'Pedro Alves' , 'Eli Zaretskii' , gdb-patches@sourceware.org Subject: Re: [RFC-v4] Fix .text section offset for windows DLL (was Calling __stdcall functions in the inferior) Message-ID: <20121126172227.GB3089@adacore.com> References: <83y5jb7rfe.fsf@gnu.org> <006001cdaada$00c81f00$02585d00$@muller@ics-cnrs.unistra.fr> <20121024194517.GK3555@adacore.com> <011901cdb2ab$48076b90$d81642b0$@muller@ics-cnrs.unistra.fr> <20121105171121.GA2972@adacore.com> <50991f5f.8382440a.1100.ffff82abSMTPIN_ADDED@mx.google.com> <509ABA17.30507@redhat.com> <000301cdbd96$f5cd9f10$e168dd30$@muller@ics-cnrs.unistra.fr> <20121122173019.GF9964@adacore.com> <009801cdcb5f$38ecd830$aac68890$@muller@ics-cnrs.unistra.fr> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <009801cdcb5f$38ecd830$aac68890$@muller@ics-cnrs.unistra.fr> User-Agent: Mutt/1.5.21 (2010-09-15) 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 X-SW-Source: 2012-11/txt/msg00674.txt.bz2 > I used alloca here... And tried to use if for the qualified_name > too, but that turned out to be a bad idea... > > The name was apparently still accessed after the function returned, > despite the fact that the symbol_name is copied... (This is probably > in the hash table, but I didn't completely understand how that > works...) I think you might have unearthed a bug, here. This is the code you propose: + qualified_name = xstrprintf ("%s!%s", dll_name, sym_name); + prim_record_minimal_symbol (qualified_name, vma, msymtype, objfile); Following the code: prim_record_minimal_symbol -> prim_record_minimal_symbol_full with copy_name=1 -> SYMBOL_SET_NAMES (msymbol, name, name_len, copy_name, objfile); And symbol_set_names does: | else | { | lookup_len = len; | lookup_name = linkage_name; | linkage_name_copy = linkage_name; | } | | entry.mangled = (char *) lookup_name; | slot = ((struct demangled_name_entry **) | htab_find_slot (objfile->demangled_names_hash, | &entry, INSERT)); Does it looks like a reference to the string might actually get inserted in the htab if not found, thus defeating the mechanism above? Tom might be better suited to answer that question more authoritatively. In the meantime, one approach that might be equally suitable, but without the bug, is if allocate the name on the objfile obstack, and then call prim_record_minimal_symbol_full directly (I am assuming that objfile can never be null, which seems to be the case by my reading of your patch). > > Are we missing a cleanup/xfree? > > I added some, please check that part, as I have no experience at all > with using make_cleanup related functions... In particular, I didn't > really get if it is OK to call do_cleanups with a possibly NULL > argument... It seems like you're going to have to make some other changes to address the crash that was reported, and Pedro sent you a link to the documentation. So I'll wait for that version before reviewing. Hope that's OK. > > > + section_data[PE_SECTION_INDEX_DATA].section_name = ".data"; > > > + section_data[PE_SECTION_INDEX_BSS].ms_type = mst_bss; > > > + section_data[PE_SECTION_INDEX_BSS].section_name = ".bss"; > > > > Also, I think it makes it harder to determine what the contents of the > > table is. I suggest you go back to the static definition above, but > > updated with the extra field. > > Sorry, but here my C knowledge is not sufficent: > section_data is now a pointer allocated on heap, > how can I reconcile this with the old static definition? I cannot remember what I was thinking (or inhaling) at the time. Probably something good (thinking or inhaling), but now gone. Suggestion withdrawn... > > Can you make prim_record_minimal_symbol sym_name parameter a "const", > > and then declare... > > I hope you meant changing add_pe_exported_sym only... > I don't want to touch prim_record_minimal_symbol function which is also > used in other C sources... > > I removed the null_char by handling NULL also in add_pe_exportd_sym > instead of only supporting a pointer to '\0'. Yes, I meant add_pe_exported_sym, nowing that the only potential issue was with the call to prim_record_minimal_symbol - but not an issue since the name parameter is already a const. Your solution is good, although making the sym_name parameter a const is also an improvement. -- Joel