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

[Pedro Alves <palves at redhat dot com>]

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.

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

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

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

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

Otherwise looks fine to me.

-- 
Pedro Alves