This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC-v3] Fix .text section offset for windows DLL (was Calling __stdcall functions in the inferior)
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Pierre Muller <pierre dot muller at ics-cnrs dot unistra dot fr>
- Cc: 'Pedro Alves' <palves at redhat dot com>, 'Eli Zaretskii' <eliz at gnu dot org>, gdb-patches at sourceware dot org
- Date: Thu, 22 Nov 2012 09:30:19 -0800
- Subject: Re: [RFC-v3] Fix .text section offset for windows DLL (was Calling __stdcall functions in the inferior)
- References: <834nm07z0s.fsf@gnu.org> <5077FEB9.4030304@redhat.com> <83y5jb7rfe.fsf@gnu.org> <006001cdaada$00c81f00$02585d00$@muller@ics-cnrs.unistra.fr> <20121024194517.GK3555@adacore.com> <011901cdb2ab$48076b90$d81642b0$@muller@ics-cnrs.unistra.fr> <20121105171121.GA2972@adacore.com> <50991f5f.8382440a.1100.ffff82abSMTPIN_ADDED@mx.google.com> <509ABA17.30507@redhat.com> <000301cdbd96$f5cd9f10$e168dd30$@muller@ics-cnrs.unistra.fr>
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