From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 977 invoked by alias); 25 Oct 2012 12:21:53 -0000 Received: (qmail 957 invoked by uid 22791); 25 Oct 2012 12:21:51 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,MSGID_MULTIPLE_AT,TW_BJ X-Spam-Check-By: sourceware.org Received: from mailhost.u-strasbg.fr (HELO mailhost.u-strasbg.fr) (130.79.200.157) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 25 Oct 2012 12:21:44 +0000 Received: from md13.u-strasbg.fr (md13.u-strasbg.fr [130.79.200.248]) by mailhost.u-strasbg.fr (8.14.3/jtpda-5.5pre1) with ESMTP id q9PCLeh6041656 ; Thu, 25 Oct 2012 14:21:40 +0200 (CEST) (envelope-from pierre.muller@ics-cnrs.unistra.fr) Received: from mailserver.u-strasbg.fr (ms16.u-strasbg.fr [130.79.204.116]) by md13.u-strasbg.fr (8.14.3/jtpda-5.5pre1) with ESMTP id q9PCLdNZ005775 ; Thu, 25 Oct 2012 14:21:39 +0200 (envelope-from pierre.muller@ics-cnrs.unistra.fr) Received: from E6510Muller (gw-ics.u-strasbg.fr [130.79.210.225]) (user=mullerp mech=LOGIN) by mailserver.u-strasbg.fr (8.14.3/jtpda-5.5pre1) with ESMTP id q9PCLdqx015807 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO) ; Thu, 25 Oct 2012 14:21:39 +0200 (envelope-from pierre.muller@ics-cnrs.unistra.fr) From: "Pierre Muller" To: "'Joel Brobecker'" Cc: References: <83a9vs89r9.fsf@gnu.org> <201210120953.q9C9rqfu020865@glazunov.sibelius.xs4all.nl> <834nm07z0s.fsf@gnu.org> <5077FEB9.4030304@redhat.com> <83y5jb7rfe.fsf@gnu.org> <006001cdaada$00c81f00$02585d00$@muller@ics-cnrs.unistra.fr> <20121024194517.GK3555@adacore.com> In-Reply-To: <20121024194517.GK3555@adacore.com> Subject: RE: [RFC] Fix .text section offset for windows DLL (was Calling __stdcall functions in the inferior) Date: Thu, 25 Oct 2012 12:21:00 -0000 Message-ID: <011901cdb2ab$48076b90$d81642b0$@muller@ics-cnrs.unistra.fr> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable 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-10/txt/msg00494.txt.bz2 Hi Joel, > -----Message d'origine----- > De=A0: gdb-patches-owner@sourceware.org [mailto:gdb-patches- > owner@sourceware.org] De la part de Joel Brobecker > Envoy=E9=A0: mercredi 24 octobre 2012 21:45 > =C0=A0: Pierre Muller > Cc=A0: gdb-patches@sourceware.org > Objet=A0: Re: [RFC] Fix .text section offset for windows DLL (was Calling > __stdcall functions in the inferior) >=20 > Hi Pierre, >=20 > I don't know COFF/PE all that well, so I'll just trust you on > the extraction of the information itself. Thanks for taking this on, > by the way. Thanks for the review. =20 > Oh dear - I just realized midway through the review that the patch > appears to be checked in already. I must have missed the associated > emails.... I've continued the review anyways - it's nothing major, > but there are a few nits I noticed. If you don't have time to look > at them, no problem; I'll try to get to them. But do let me know. Oh boy, you scared me there... But definitively, no, I didn't commit this change yet! As confirmed by: http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/coff-pe-read.c?cvsroot=3Ds= rc Last change on that file dates by January 6. 2012=20 This was only a RFC, which explains why I did not bother to change the printf call yet! =20 > > 2012-10-15 Pierre Muller > > > > * coff-pe-read.h (pe_text_section_offset): Declare new function. > > * coff-pe-read.c (pe_as16): New function. > > (read_pe_exported_syms): Use ordinal of function to > > retrieve correct RVA address of function. > > (pe_text_section_offset): New function. > > > > * windows-tdep.c (windows_xfer_shared_library): Use > > pe_text_section_offset function instead of possibly wrong > > 0x1000 constant for .text sextion offset. >=20 > > @@ -336,26 +344,119 @@ read_pe_exported_syms (struct objfile *o > > { > > /* Pointer to the names vector. */ > > unsigned long name_rva =3D pe_as32 (erva + name_rvas + i * 4); > > + /* Retrieve ordinal value */ > > + > > + unsigned long ordinal =3D pe_as16 (erva + ordinals + i * 2); >=20 > Just a nit: Can we keep the formatting consistent between the two > local variables? In other words, no empty line between the comment > and the variable? Yes, you are of course right here. > > + if (!section_found) > > + { > > + char * forward_name =3D (char *) (erva + func_rva); > > + char * funcname =3D (char *) (erva + name_rva); > > + if ((func_rva >=3D export_rva) > > + && (func_rva < export_rva + export_size)) >=20 > Can you add an empty line between the variable declarations and > the rest of the code. Forgot that ruel again...=20 > Also, the if condition is idented using spaces instead of tabs... I still didn't get a correct way of checking my formatting... =20 > > + printf ("%s is a forward to %s\n", funcname, forward_name); >=20 > I don't think a printf is appropriate, here. Is that meant to be > a warning? I would have like any suggestion about an idea=20 about how to implement such a forward... Something like a function called add_pe_forward_symbol Would it require the definition of a new minimal symbol type? If yes, I wouldn't even know where I should add this new type... =20 > > +CORE_ADDR > > +pe_text_section_offset (struct bfd *abfd) >=20 > Can you add a description of what the function does? I will try to do this. =20 > > + unsigned long pe_header_offset, opthdr_ofs, num_entries, i; >=20 > This is a nit as well, but can you rename opthdr_ofs? I'm a little > confused, as the 's' at the end made me think that it was a plural, > and thus that it was a set of offsets. But looking at the type and > at the code, I am thinking now that this is a short for "offset", > except an 'f' would be missing. How about spelling offset entirely? I basically reused the existing code in the function called read_pe_exported_syms, and, specifically, I kept most of the local variables and used the same names... I quite often also abbreviate offset into ofs, but I am not against renaming 'opthdr_ofs' into 'optional_header_offset'. > > + unsigned char *expdata, *erva; >=20 > Should we be using gdb_byte *, in this case? I'm wondering if we > should be adjusting the pe_get* & pe_as* routines as well... The problem is that it is used both for 'char *' for all the names and for RVA (relative virtual addresses) which are more something like a 32-bit unsigned offset. =20 > > + if (!is_pe32 && !is_pe64) > > + { > > + /* This is not a recognized PE format file. Abort now, because > > + the code is untested on anything else. *FIXME* test on > > + further architectures and loosen or remove this test. */ > > + return 0; > > + } >=20 > I think a complaint would be appropriate, here. And I'm wondering > if there might be a better way to check which PE format it is other > than looking at a string... Again, this is a plain copy of the code in read_pe_exported_syms. > > + if (num_entries < 1) /* No exports. */ > > + { > > + return 0; > > + } >=20 > Formatting: Can you remove the curly braces? For one statement, > our codig style says that we should not be using them. Ditto... =20 > > @@ -387,6 +390,9 @@ windows_xfer_shared_library (const char* > > struct gdbarch *gdbarch, struct obstack *obstack) > > { > > char *p; > > + struct bfd * dll; > > + CORE_ADDR text_offset; > > + CORE_ADDR default_text_offset =3D 0x1000; > > obstack_grow_str (obstack, "=20 > Can you add an empty line after the local variable declarations? =20 =20 > > + if (text_offset !=3D default_text_offset) > > + warning (_("DLL %s has .text section at offset %s\n"),so_name, >=20 > Missing space after the coma (just before 'so_name'). I will try to submit a RFA shortly... Pierre