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

[Pedro Alves <palves@redhat.com>]

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 '*'

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

-- 
Pedro Alves