This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


  I attach a new version of my patch to
cope with variable offset of .text section  for Windows OS DLL's.

  I hope I have taken all advices of Joel in account.
In fact, I was unable to satisfy one suggestion...
See below, the answer about my lack of C knowledge. 


  Hoping we are getting closer...

Pierre Muller


(unchanged) ChangeLog entry:

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.
        (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.

> -----Message d'origine-----
> De?: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé?: jeudi 22 novembre 2012 18:30
> À?: Pierre Muller
> Cc?: 'Pedro Alves'; 'Eli Zaretskii'; gdb-patches@sourceware.org
> Objet?: Re: [RFC-v3] Fix .text section offset for windows DLL (was Calling
> __stdcall functions in the inferior)
> 
> 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?
  Of course, thanks for catching this.

> > +   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.


  These are windows specific macros,
but can also be compiled on any other target, if one
of the windows target is included in the target list.
  Thus, I do not really know what type of comment I should add here...

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

  Done.
 
> > +  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);
  Modified as suggested.

> > +  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...

Modified using fprintf_unfitered (gdb_stdlog, as per Pedro's suggestion...
 
> > +/* 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.
  I used alloca here...
And tried  to use if for the qualified_name too, but that
turned out to be a bad idea...
  The name was apparently still accessed after the function 
returned, despite the fact that the symbol_name is copied...
(This is probably in the hash table, but I didn't completely
understand how that works...)
 
> > +  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.

  xsnprintf ("%s!%s", should be even simpler...
 
> > +      int i;
> > +      for (i = 0; i < forward_dll_name_len; i++)
> 
> Empty line between variable declaration and the rest of the code...

  Still forgetting basics :(
Fixed.
 
> > +  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"...

  OK, I change it to alloca...
and did the same in the existing add_pe_exportd_sym function
But as I said above, this was a bad idea :(

> > +  char * last_point = strrchr (dll_name, '.');
> > +  if (last_point != NULL)
> > +    *last_point = '\0';
> 
> Empty line after var declarations...

  Fixed.

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

  I added some, please check that part, as I have
no experience at all with using make_cleanup  
related functions...
  In particular, I didn't really get if it is OK to call
do_cleanups with a possibly NULL argument...
 
> > +  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.

  Sorry, but here my C knowledge is not sufficent:
section_data is now a pointer allocated on heap,
how can I reconcile this with the old static definition?

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

  I hope you meant changing add_pe_exported_sym only...
I don't want to touch prim_record_minimal_symbol function which is also
used in other C sources...
 
  I removed the null_char by handling NULL also in add_pe_exportd_sym
instead of only supporting a pointer to '\0'.

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

  Yes...
> > +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

Attachment: fix-dll-offset-v5.patch
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]