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

["Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : vendredi 20 décembre 2013 19:19
> À : Pierre Muller
> Cc : 'Joel Brobecker'; gdb-patches@sourceware.org
> Objet : Re: [RFA-v2] Fix PR 16201: internal error on a cygwin program
> linked against a DLL with no .data section
> 
> On 12/13/2013 09:39 PM, Pierre Muller wrote:
> >
> 
> >> I just re-read the code, and I really think it would be better if
> >> someone who actually understands the general framework could
> comment.
> >> The problem seems, as you stated, relatively well understood, but
> >> I am not sure how we are expected to fix it.
> >>
> >>> 2013-11-27  Pierre Muller  <muller@sourceware.org>
> >>>
> >>>         PR 16201
> >>>         coff-pe-read.c (read_pe_exported_syms): Set
> sect_index_text,
> 
> Missing '*'
Whoops, 
> >>>         sect_index_data and sect_index_bss of objfile struct, even
> if
> >>>         there is no canonical '.text', '.data' or '.bss' named
> >> section.
> >>
> >> My only comment is that the patch could gain from some additional
> >> comments explaining _why_ you're forcing the sect_index field
> >> ("event if already set before"), and what you are trying to achieve.
> >
> > Here is a new version in which I try to explain
> > more clearly that if we find the canonical
> >  '.text', '.data' or '.bss' section names,
> > we should use these sections to set sect_index_XXX.
> > Otherwise, we use the first section that is used later with
> > for which we set ms_type to mst_XXX to also set sect_index_XXX.
> >   This ensure that sect_index_XXX is always set if
> > any exported symbol is in inserted using
> > prim_rcord_minimal_symbol with ms_type parameter set to mst_XXX
> >
> > I hope this clarifies the patch .
> >
> 
> So in the DLL in question, there was no .data section, but
> there was another section with IMAGE_SCN_CNT_INITIALIZED_DATA set.
> What was this section?  From the PR:
> 
> $ objdump -h icudt49.dll
> 
> icudt49.dll:     file format pei-i386
> 
> Sections:
> Idx Name          Size      VMA       LMA       File off  Algn
>   0 .rdata        0111f4fa  10001000  10001000  00000400  2**2
>                   CONTENTS, ALLOC, LOAD, READONLY, DATA
>   1 .rsrc         00000458  11121000  11121000  0111fa00  2**2
>                   CONTENTS, ALLOC, LOAD, READONLY, DATA
> 
> From the PR, we see the dll exported a icudt49_dat symbol:
> 
> ...
> #1  0x0054ae16 in prim_record_minimal_symbol
> (name=name@entry=0x8019db78 "icudt49!icudt49_dat",
>     address=address@entry=1585713152, ms_type=mst_data,
>     objfile=objfile@entry=0x8027a9c8)
> ...
> 
> So the fix is this part:
> 
>           else if (characteristics & IMAGE_SCN_CNT_INITIALIZED_DATA)
> -           section_data[otherix].ms_type = mst_data;
> +           {
> +             section_data[otherix].ms_type = mst_data;
> +             if (objfile->sect_index_data == -1)
> +             objfile->sect_index_data = otherix;
> +           }
> 
> 
> It's not clear to me that forcing sect_index_... when the
> canonical section is found is better than using the
> first / lowest section that looks like code/data/bss.  I'd
> suggest just taking the first found.  IOW, do:
> 
>              if (objfile->sect_index_data == -1)
>              objfile->sect_index_data = otherix;
> 
> in the other branch too.
> 
> But, hmmm, don't we know the symbol's section?
> Wouldn't it be even better to make add_pe_exported_sym
> call prim_record_minimal_symbol_and_info directly,
> rather than prim_record_minimal_symbol ?
Of course, I didn't dig down to the level where I should have seen this...
Here is an updated patch.
As I was not sure that the bfd index order was the same as the PE index
order, I played it safe.

  Comments?

Pierre Muller


2013-12-22  Pierre Muller  <muller@sourceware.org>

        Fix PR16201.
        * coff-pe-read.c (section_data): Add index field.
        (add_pe_exported_sym): Use SECTION_DATA->INDEX for call
        to prim_record_mininal_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 91ee3f6..e04e9ff 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;          /* Section number.  */
   char *section_name;          /* Recorded 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);
@@ -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);
+      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;
        }
       else
        {
@@ -479,6 +499,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)