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

[Joel Brobecker <brobecker@adacore.com>]

Hello Pierre,

> 2012-11-08  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.
> 	(pe_as16): New function.
> 	(IMAGE_SCN_CNT_CODE): New macro, if not already defined.
> 	(IMAGE_SCN_CNT_INITIALIZED_DATA): Ditto.
> 	(IMAGE_SCN_CNT_UNINITIALIZED_DATA): Ditto.
> 	(add_pe_exported_sym): Handle unnamed exported function.
> 	(add_pe_forwarded_sym): 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.

Just a few minor comments...

> +/* Extract for ABFD the offset of the .text section.
             ^^^^^ from?
> +   Returns default value 0x1000 if information is not found.  */
> +extern CORE_ADDR pe_text_section_offset (struct bfd *abfd);

> +#ifndef IMAGE_SCN_CNT_CODE
> +# define IMAGE_SCN_CNT_CODE 0x20
> +#endif
> +#ifndef IMAGE_SCN_CNT_INITIALIZED_DATA
> +# define IMAGE_SCN_CNT_INITIALIZED_DATA 0x40
> +#endif
> +#ifndef IMAGE_SCN_CNT_UNINITIALIZED_DATA
> +# define IMAGE_SCN_CNT_UNINITIALIZED_DATA 0x80
> +#endif

Do you have an idea of when these macros might not be defined?
(and where they are normally coming from?). It'd be nice to add
a comment providing the answer to those questions.

>  static void
>  add_pe_exported_sym (char *sym_name,
>  		     unsigned long func_rva,
> +		     int ordinal,
>  		     const struct read_pe_section_data *section_data,
>  		     const char *dll_name, struct objfile *objfile)

Can you update the funtion documentation to explain what each
parameter is?

> +  if ((section_data->ms_type == mst_unknown) && debug_coff_pe_read)
> +    printf_filtered (_("Unknown section type for \"%s\" for entry \"%s\" \
> +in dll \"%s\"\n"),
> +		     section_data->section_name, sym_name, dll_name);

There was a discussion in the past about continuations of string
messages, and we decided to avoid putting the continuation of the
first column, as it affects the -p switch of the diff. I think you
can simply do:

    printf_filtered (_("Unknown section type for \"%s\" for entry"
                       " \"%s\" in dll \"%s\"\n"),
		     section_data->section_name, sym_name, dll_name);

> +  if (debug_coff_pe_read > 1)
> +    printf_filtered (_("Adding exported symbol \"%s\" in dll \"%s\"\n"),
> +		     sym_name, dll_name);

Can you use printf_unfiltered for debug traces? There are several
instances of this...

> +/* Create a minimal symbol entry for an exported forward symbol.
> +   Returns 1 if the forwarded function was found 0 otherwise.  */

"Return" (our style is more "do this, do that", rather than "does this,
does that"). Can you also document what each parameter is?

> +  int dll_name_len = strlen (dll_name);
> +  char *forward_minimal_name = xmalloc (forward_dll_name_len +
> +					forward_func_name_len + 2);

Can you use alloca, here? It avoids the need to xfree it later.
If you need to xmalloc, then you need to xfree it, and I think
that you'd be safer using a cleanup - some of the functions you
use might throw an error.

> +  strncpy (forward_minimal_name, forward_dll_name, forward_dll_name_len);
> +  forward_minimal_name[forward_dll_name_len] = '!';
> +  strcpy (forward_minimal_name + forward_dll_name_len + 1, forward_func_name);

You also have the "concat" function, if that makes things a little
simple for you.

> +      int i;
> +      for (i = 0; i < forward_dll_name_len; i++)

Empty line between variable declaration and the rest of the code...

> +  if (debug_coff_pe_read > 1)
> +    printf_filtered (_("Adding forwarded exported symbol \"%s\" "
> +		       "in dll \"%s\", pointing to \"%s\"\n"),
> +		     sym_name, dll_name, forward_minimal_name);
[l..]
> +      if (debug_coff_pe_read)
> +	printf_filtered (_("Unable to find function \"%s\" in dll \"%s\" "
> +			   ", forward of \"%s\" in dll \"%s\"\n"),
> +			 forward_func_name, forward_dll_name, sym_name,
> +			 dll_name);

printf_unfiltered.

> +  qualified_name = xmalloc (dll_name_len + strlen (sym_name) + 2);

Same remarks as for "forward_minimal_name"...

> +  char * last_point = strrchr (dll_name, '.');
> +  if (last_point != NULL)
> +    *last_point = '\0';

Empty line after var declarations...

> -  struct read_pe_section_data section_data[PE_SECTION_TABLE_SIZE]
> -    = { {0, 1, 0, mst_text},
> -  {0, 1, 0, mst_data},
> -  {0, 1, 0, mst_bss}
> -  };
> +  struct read_pe_section_data *section_data;
[...]
> +  section_data = xzalloc (PE_SECTION_TABLE_SIZE
> +			 * sizeof (struct read_pe_section_data));
> +

Are we missing a cleanup/xfree?

> +  for (i=0; i < PE_SECTION_TABLE_SIZE; i++)
> +    {
> +      section_data[i].vma_offset = 0;
> +      section_data[i].rva_start = 1;
> +      section_data[i].rva_end = 0;
> +    };
> +  section_data[PE_SECTION_INDEX_TEXT].ms_type = mst_text;
> +  section_data[PE_SECTION_INDEX_TEXT].section_name = ".text";
> +  section_data[PE_SECTION_INDEX_DATA].ms_type = mst_data;
> +  section_data[PE_SECTION_INDEX_DATA].section_name = ".data";
> +  section_data[PE_SECTION_INDEX_BSS].ms_type = mst_bss;
> +  section_data[PE_SECTION_INDEX_BSS].section_name = ".bss";

Also, I think it makes it harder to determine what the contents of the
table is. I suggest you go back to the static definition above, but
updated with the extra field.

> -      if (vaddr <= export_rva && vaddr + vsize > export_rva)
> +      if ((strcmp (sname, ".edata") == 0)
> +	  || ((vaddr <= export_opthdrrva)
> +	      && (export_opthdrrva < vaddr + vsize)))

The extra parentheses around the numerical comparison operators are
unnecessary, I believe. I think the following is equivalent:

      if (strcmp (sname, ".edata") == 0
	  || (vaddr <= export_opthdrrva && export_opthdrrva < vaddr + vsize))

> +	  else if ((export_opthdrrva != vaddr) && debug_coff_pe_read)

Same here...

> +      /* Retrieve ordinal value */

Missing period at end of comment.

> +      /* This is relatived to ordinal value. */

Missing second space after the period...

> +      if ((func_rva >= export_rva)
> +	  && (func_rva < export_rva + export_size))

Unnecessary parentheses.

> +	  if (sep)
> +	    {
> +	      int len = (int) (sep - forward_name);
> +	      forward_dll_name = xmalloc (len + 1);

Missing empty line after variable declaration.  You might want to
use a cleanup, just in case something in add_pe_forwarded_sym calls
error.

> +	      static char null_char = '\0';
> +
> +	      add_pe_exported_sym (&null_char, func_rva, ordinal,
> +				   section_data, dll_name, objfile);

Why does this have to be static?

Can you make prim_record_minimal_symbol sym_name parameter a "const",
and then declare...

        const char *empty_name = "";

... and pass that to add_pe_exported_sym?


> +      char sname[8];
[...]
> +      bfd_bread (sname, (bfd_size_type) 8, abfd);

Use sizeof (sname) instead of litteral 8?

> +static void
> +show_debug_coff_pe_read (struct ui_file *file, int from_tty,
> +		struct cmd_list_element *c, const char *value)

This function needs a short description ("implements ..." is good
enough).

-- 
Joel