From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28953 invoked by alias); 24 Oct 2012 19:45:28 -0000 Received: (qmail 28945 invoked by uid 22791); 24 Oct 2012 19:45:27 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,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; Wed, 24 Oct 2012 19:45:21 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 5DAAA2E060; Wed, 24 Oct 2012 15:45:20 -0400 (EDT) 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 jBHvQbDcMTNN; Wed, 24 Oct 2012 15:45:20 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 3CD8D1C7E2F; Wed, 24 Oct 2012 15:45:20 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 730FAC88A1; Wed, 24 Oct 2012 15:45:17 -0400 (EDT) Date: Wed, 24 Oct 2012 19:45:00 -0000 From: Joel Brobecker To: Pierre Muller Cc: gdb-patches@sourceware.org Subject: Re: [RFC] Fix .text section offset for windows DLL (was Calling __stdcall functions in the inferior) Message-ID: <20121024194517.GK3555@adacore.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <006001cdaada$00c81f00$02585d00$@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-10/txt/msg00480.txt.bz2 Hi Pierre, 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. 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. > 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. > @@ -336,26 +344,119 @@ read_pe_exported_syms (struct objfile *o > { > /* Pointer to the names vector. */ > unsigned long name_rva = pe_as32 (erva + name_rvas + i * 4); > + /* Retrieve ordinal value */ > + > + unsigned long ordinal = pe_as16 (erva + ordinals + i * 2); 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? > + if (!section_found) > + { > + char * forward_name = (char *) (erva + func_rva); > + char * funcname = (char *) (erva + name_rva); > + if ((func_rva >= export_rva) > + && (func_rva < export_rva + export_size)) Can you add an empty line between the variable declarations and the rest of the code. Also, the if condition is idented using spaces instead of tabs... > + printf ("%s is a forward to %s\n", funcname, forward_name); I don't think a printf is appropriate, here. Is that meant to be a warning? > +CORE_ADDR > +pe_text_section_offset (struct bfd *abfd) Can you add a description of what the function does? > + unsigned long pe_header_offset, opthdr_ofs, num_entries, i; 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? > + unsigned char *expdata, *erva; 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... > + 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; > + } 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... > + if (num_entries < 1) /* No exports. */ > + { > + return 0; > + } Formatting: Can you remove the curly braces? For one statement, our codig style says that we should not be using them. > @@ -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 = 0x1000; > obstack_grow_str (obstack, " + if (text_offset != default_text_offset) > + warning (_("DLL %s has .text section at offset %s\n"),so_name, Missing space after the coma (just before 'so_name'). -- Joel