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

[Joel Brobecker <brobecker@adacore.com>]

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  <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?

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

-- 
Joel