RE: [RFC] Fix .text section offset for windows DLL (was Calling __stdcall functions in the inferior)

["Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>]

  Hi Joel,

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : mercredi 24 octobre 2012 21:45
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFC] Fix .text section offset for windows DLL (was Calling
> __stdcall functions in the inferior)
> 
> 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.

  Thanks for the review.
 
> 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=src
Last change on that file dates by January 6. 2012 


  This was only a RFC,
which explains why I did not bother to change the
printf call yet!

 
> > 2012-10-15  Pierre Muller  <muller@ics.u-strasbg.fr>
> >
> > 	* 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?

  Yes, you are of course right here.

> > +      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.
  Forgot that ruel again... 
> Also, the if condition is idented using spaces instead of tabs...

  I still didn't get a correct way of checking my formatting...
 
> > +	  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?

  I would have like any suggestion about an idea 
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...
 
> > +CORE_ADDR
> > +pe_text_section_offset (struct bfd *abfd)
> 
> Can you add a description of what the function does?
I will try to do this.
 
> > +  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?

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;
> 
> 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.

 
> > +  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...
 Again, this is a plain copy of the code in
read_pe_exported_syms.

> > +  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.

  Ditto...
 
> > @@ -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, "<library name=\"");
> 
> Can you add an empty line after the local variable declarations?
 

 
> > +      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').

  I will try to submit a RFA shortly...

Pierre