[COMMIT-v4] Fix PR 16201: internal error on a cygwin program linked against a DLL with no .data section

["Pierre Muller" <pierre dot muller at ics-cnrs dot unistra dot fr>]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : mardi 7 janvier 2014 21:59
> À : Pierre Muller
> Cc : 'Joel Brobecker'; gdb-patches@sourceware.org
> Objet : Re: [RFA-v3] Fix PR 16201: internal error on a cygwin program
> linked against a DLL with no .data section
> 
> On 01/07/2014 11:15 AM, Pierre Muller wrote:
> >> On 12/22/2013 10:55 PM, Pierre Muller wrote:
> >>> @@ -455,17 +458,34 @@ read_pe_exported_syms (struct objfile
> *objfile)
> >>>        unsigned long characteristics = pe_get32 (dll, secptr1 +
> 36);
> >>>        char sec_name[SCNNMLEN + 1];
> >>>        int sectix;
> >>> +      unsigned int bfd_section_index;
> >>> +      asection *section;
> >>>
> >>>        bfd_seek (dll, (file_ptr) secptr1 + 0, SEEK_SET);
> >>>        bfd_bread (sec_name, (bfd_size_type) SCNNMLEN, dll);
> >>>        sec_name[SCNNMLEN] = '\0';
> >>>
> >>>        sectix = read_pe_section_index (sec_name);
> >>> +      section = bfd_get_section_by_name (dll, sec_name);
> >>
> >> Can't coff have sections with duplicate names?
> >   I did not find anything in the PE COFF description
> > that explicitly said that each section should have a unique name
> > but I always assumed that the assembler/linker would
> > always group all sections with the same name.
> 
> Usually, but it's also not usually mandatory.  We're reading
> a linked PE file, so I'm really not sure.  In any case,
> relying on section names usually indicates something is being
> done wrong (and GDB is full of that, unfortunately)...  Given that
> bfd itself creates sections from the PE's sections, I'd guess
> the indexes should match, maybe with some offset.
> 
> Anyway, I don't want to invest time to try this out myself.  Fine
> with me to leave it looking up by name for now, if you'd like.

 OK, thanks.
 
> >
> >> If so,
> >> then it'd be better to match the section some other way,
> >> I guess by address?
> >
> >   I would not know how to do this...
> 
> You'd just walk over the sections, and compare addresses.
> Look for "bfd->sections" in symfile.c for example.  But
> anyway, it might be that duplicate sections would be
> overlapping, so that wouldn't be the ideal match.

 Yes, but there is all this rva stuff that is also going on on the same
time...
I am pretty sure that duplicate section names are not possible in
a linker generated Windows PE file.
 
> >
> >>> +      if (section)
> >>> +       bfd_section_index = section->index;
> >>> +      else
> >>> +       bfd_section_index = -1;
> >>>
> >>>        if (sectix != PE_SECTION_INDEX_INVALID)
> >>>         {
> >>>           section_data[sectix].rva_start = vaddr;
> >>>           section_data[sectix].rva_end = vaddr + vsize;
> >>> +         /* For .text, .data and .bss section
> >>> +             set corresponding sect_index_XXX,
> >>> +             even if it was already set before.  */
> >>> +         if (sectix == PE_SECTION_INDEX_TEXT)
> >>> +           objfile->sect_index_text = sectix;
> >>> +         if (sectix == PE_SECTION_INDEX_DATA)
> >>> +           objfile->sect_index_data = sectix;
> >>> +         if (sectix == PE_SECTION_INDEX_BSS)
> >>> +           objfile->sect_index_bss = sectix;
> >>> +         section_data[sectix].index = bfd_section_index;
> >>
> >> Do you still need this part?
> >   This is still an improvement as it sets
> > these sect_index_XXX fields that might be needed
> > elsewhere in the code.
> 
> It's the "might" part that I don't like.  If you don't need
> it, I'd rather remove it, as it might be hiding some other
> similar problem elsewhere.  It's not clear to me overriding
> is the best choice.  And if those aren't set, won't
> init_objfile_sect_indices / symfile_find_segment_sections
> end up setting them anyway?

  OK, I removed that part completely,
as the problem reported is fixed without.
 
> > @@ -53,6 +53,7 @@ struct read_pe_section_data
> >    unsigned long rva_end;	/* End offset within the pe.  */
> >    enum minimal_symbol_type ms_type;	/* Type to assign symbols in
> >  					   section.  */
> > +  unsigned int index;		/* Section number.  */
> 
> Which index?  bfd or PE ?  That should be clear in the comment,
> at least.
  Changed comment to BFD section number
 
> > @@ -455,17 +462,34 @@ read_pe_exported_syms (struct objfile *objfile)
> >        unsigned long characteristics = pe_get32 (dll, secptr1 + 36);
> >        char sec_name[SCNNMLEN + 1];
> >        int sectix;
> > +      unsigned int bfd_section_index;
> > +      asection *section;
> >
> >        bfd_seek (dll, (file_ptr) secptr1 + 0, SEEK_SET);
> >        bfd_bread (sec_name, (bfd_size_type) SCNNMLEN, dll);
> >        sec_name[SCNNMLEN] = '\0';
> >
> >        sectix = read_pe_section_index (sec_name);
> > +      section = bfd_get_section_by_name (dll, sec_name);
> > +      if (section)
> > +	bfd_section_index = section->index;
> > +      else
> > +	bfd_section_index = -1;
> 
> (See?  It looks quite odd to me to need to handle the case
> of bfd not creating section listed in the PE header.  I'd
> assume bfd reads the same section list when creating
> it's own list of sections ?)

  I also suppose that both arrays are the same, but
I also did not want to take the risk of dereferencing
a NULL pointer...
 
> Otherwise looks fine to me.

  Thanks for the approval,
 
Pierre

For the record, this is what I committed:

2014-01-08  Pierre Muller  <muller@sourceware.org>

	Fix PR16201.
	* coff-pe-read.c (struct read_pe_section_data): Add index field.
	(add_pe_exported_sym): Use SECTION_DATA->INDEX for call
	to prim_record_mininal_symbol_and_info.
	(add_pe_forwarded_sym): Use known section number of forwarded symbol
	in call to prim_record_minimal_symbol_and_info.
	(read_pe_exported_syms): Set index field of section_data.

diff --git a/gdb/coff-pe-read.c b/gdb/coff-pe-read.c
index 749c109..0fcd15f 100644
--- a/gdb/coff-pe-read.c
+++ b/gdb/coff-pe-read.c
@@ -53,6 +53,7 @@ struct read_pe_section_data
   unsigned long rva_end;	/* End offset within the pe.  */
   enum minimal_symbol_type ms_type;	/* Type to assign symbols in
 					   section.  */
+  unsigned int index;		/* BFD section number.  */
   char *section_name;		/* Recorded section name.  */
 };
 
@@ -93,7 +94,7 @@ read_pe_section_index (const char *section_name)
     }
 }
 
-/* Get the index of the named section in our own full arrayi.
+/* Get the index of the named section in our own full array.
    text, data and bss in that order.  Return PE_SECTION_INDEX_INVALID
    if passed an unrecognised section name.  */
 
@@ -175,11 +176,13 @@ add_pe_exported_sym (const char *sym_name,
 			" for entry \"%s\" in dll \"%s\"\n"),
 			section_data->section_name, sym_name, dll_name);
 
-  prim_record_minimal_symbol (qualified_name, vma,
-			      section_data->ms_type, objfile);
+  prim_record_minimal_symbol_and_info (qualified_name, vma,
+				       section_data->ms_type,
+				       section_data->index, objfile);
 
   /* Enter the plain name as well, which might not be unique.  */
-  prim_record_minimal_symbol (bare_name, vma, section_data->ms_type,
objfile);
+  prim_record_minimal_symbol_and_info (bare_name, vma,
section_data->ms_type,
+				       section_data->index, objfile);
   if (debug_coff_pe_read > 1)
     fprintf_unfiltered (gdb_stdlog, _("Adding exported symbol \"%s\""
 			" in dll \"%s\"\n"), sym_name, dll_name);
@@ -209,6 +212,7 @@ add_pe_forwarded_sym (const char *sym_name, const char
*forward_dll_name,
   int forward_func_name_len = strlen (forward_func_name);
   int forward_len = forward_dll_name_len + forward_func_name_len + 2;
   char *forward_qualified_name = alloca (forward_len);
+  short section;
 
   xsnprintf (forward_qualified_name, forward_len, "%s!%s",
forward_dll_name,
 	     forward_func_name);
@@ -242,6 +246,7 @@ add_pe_forwarded_sym (const char *sym_name, const char
*forward_dll_name,
 
   vma = SYMBOL_VALUE_ADDRESS (msymbol.minsym);
   msymtype = MSYMBOL_TYPE (msymbol.minsym);
+  section = SYMBOL_SECTION (msymbol.minsym);
 
   /* Generate a (hopefully unique) qualified name using the first part
      of the dll name, e.g. KERNEL32!AddAtomA.  This matches the style
@@ -254,10 +259,12 @@ add_pe_forwarded_sym (const char *sym_name, const char
*forward_dll_name,
 
   qualified_name = xstrprintf ("%s!%s", dll_name, bare_name);
 
-  prim_record_minimal_symbol (qualified_name, vma, msymtype, objfile);
+  prim_record_minimal_symbol_and_info (qualified_name, vma, msymtype,
+				       section, objfile);
 
   /* Enter the plain name as well, which might not be unique.  */
-  prim_record_minimal_symbol (bare_name, vma, msymtype, objfile);
+  prim_record_minimal_symbol_and_info (bare_name, vma, msymtype,
+				       section, objfile);
   xfree (qualified_name);
   xfree (bare_name);
 
@@ -455,17 +462,25 @@ read_pe_exported_syms (struct objfile *objfile)
       unsigned long characteristics = pe_get32 (dll, secptr1 + 36);
       char sec_name[SCNNMLEN + 1];
       int sectix;
+      unsigned int bfd_section_index;
+      asection *section;
 
       bfd_seek (dll, (file_ptr) secptr1 + 0, SEEK_SET);
       bfd_bread (sec_name, (bfd_size_type) SCNNMLEN, dll);
       sec_name[SCNNMLEN] = '\0';
 
       sectix = read_pe_section_index (sec_name);
+      section = bfd_get_section_by_name (dll, sec_name);
+      if (section)
+	bfd_section_index = section->index;
+      else
+	bfd_section_index = -1;
 
       if (sectix != PE_SECTION_INDEX_INVALID)
 	{
 	  section_data[sectix].rva_start = vaddr;
 	  section_data[sectix].rva_end = vaddr + vsize;
+	  section_data[sectix].index = bfd_section_index;
 	}
       else
 	{
@@ -479,6 +494,7 @@ read_pe_exported_syms (struct objfile *objfile)
 	  section_data[otherix].rva_start = vaddr;
 	  section_data[otherix].rva_end = vaddr + vsize;
 	  section_data[otherix].vma_offset = 0;
+	  section_data[otherix].index = bfd_section_index;
 	  if (characteristics & IMAGE_SCN_CNT_CODE)
 	    section_data[otherix].ms_type = mst_text;
 	  else if (characteristics & IMAGE_SCN_CNT_INITIALIZED_DATA)