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

Joel Brobecker
2012-11-22T17:30:43Z

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