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

[Joel Brobecker <brobecker at adacore dot com>]

> 2012-11-25  Pierre Muller  <muller@sourceware.org>
> 
>         * coff-pe-read.h (pe_text_section_offset): Declare new function.
>         * coff-pe-read.c (debug_coff_pe_read): New static variable.
>         (struct read_pe_section_data): Add section_name field.
>         (IMAGE_SCN_CNT_CODE): New macro, if not already defined.
>         (IMAGE_SCN_CNT_INITIALIZED_DATA): Ditto.
>         (IMAGE_SCN_CNT_UNINITIALIZED_DATA): Ditto.
>         (get_pe_section_index): New function.
>         (struct pe_sections_info): New type.
>         (get_section_vmas): Use new struct pe_sections_info.
>         (add_pe_exported_sym): Handle unnamed exported function.
>         (add_pe_forwarded_sym): New function.
>         (read_pe_truncate_name): Truncate at last dot.
>         (pe_as16): New function.
>         (read_pe_exported_syms): Use ordinal of function to
>         retrieve correct RVA address of function and handle
>         forwarded symbol.
>         (pe_text_section_offset): New function.
>         (show_debug_coff_pe_read): New function.
>         (_initialize_coff_pe_read): New function adding
>         'set/show debug coff_pe_read' commands.
> 
>         * windows-tdep.c (windows_xfer_shared_library): Use
>         pe_text_section_offset function instead of possibly wrong
>         0x1000 constant for .text sextion offset.

Looks good - OK to commit after the following minor corrections
have been applied. For the record, I have tested this patch on
x86-windows against AdaCore's GDB testsuite, no regression.

Thank you!

> +/* Get the index of the named section in our own full arrayi.

Small typo at the end if "array".

> +get_pe_section_index (const char *section_name,
> +		      struct read_pe_section_data *sections,
> +		      int nb_sections)
> +{
> +  int i;
> +  for (i = 0; i < nb_sections; i++)

Missing empty line after variable declarations...

> +   DLL_NAME is the internal name of the DLL file,
> +   OBJFILE is the objfile struct of DLL_NAME.  */
> +
> +
> +static int
> +add_pe_forwarded_sym (const char *sym_name, const char *forward_dll_name,

Can you delete the second empty line?

> +  char * last_point = strrchr (dll_name, '.');

No space between '*' and 'last_point'.

> +	  otherix++;
> +	  section_data = xrealloc (section_data, otherix
> +				   * sizeof (struct read_pe_section_data));
> +	  name = xstrdup (sec_name);
> +	  section_data[otherix - 1].section_name = name;
> +	  make_cleanup (xfree, name);
> +	  section_data[otherix - 1].rva_start = vaddr;
> +	  section_data[otherix - 1].rva_end = vaddr + vsize;
> +	  section_data[otherix - 1].vma_offset = 0;
> +	  if (characteristics & IMAGE_SCN_CNT_CODE)
> +	    section_data[otherix - 1].ms_type = mst_text;
> +	  else if (characteristics & IMAGE_SCN_CNT_INITIALIZED_DATA)
> +	    section_data[otherix - 1].ms_type = mst_data;
> +	  else if (characteristics & IMAGE_SCN_CNT_UNINITIALIZED_DATA)
> +	    section_data[otherix - 1].ms_type = mst_bss;
> +	  else
> +	    section_data[otherix - 1].ms_type = mst_unknown;

A possible suggestion: It seems simpler to increment otherix
at the end rather than at the beginning, and thus have:

        section_data = xrealloc (section_data, (otherix + 1) [...]);
        [...]
        section_data[otherix].rva_end = vaddr + vsize;
        section_data[otherix].vma_offset = 0;
        [...]
        otherix++;

> +      /* First handle forward cases.  */
> +      if ((func_rva >= export_rva)
> +	  && (func_rva < export_rva + export_size))

You don't need the extra parentheses...

-- 
Joel