This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA-v3] Fix PR 16201: internal error on a cygwin program linked against a DLL with no .data section
- From: Pedro Alves <palves at redhat dot com>
- To: Pierre Muller <pierre dot muller at ics-cnrs dot unistra dot fr>
- Cc: "'Joel Brobecker'" <brobecker at adacore dot com>, gdb-patches at sourceware dot org
- Date: Tue, 07 Jan 2014 20:58:40 +0000
- Subject: Re: [RFA-v3] Fix PR 16201: internal error on a cygwin program linked against a DLL with no .data section
- Authentication-results: sourceware.org; auth=none
- References: <004801cef65e$cb82d1b0$62887510$ at muller@ics-cnrs.unistra.fr> <20131211170204 dot GD3227 at adacore dot com> <52ab7ec0 dot c8da420a dot 12c6 dot ffffb3f4SMTPIN_ADDED_BROKEN at mx dot google dot com> <52B48A28 dot 2000402 at redhat dot com> <52b76e14 dot 8886420a dot 29e6 dot ffffddb2SMTPIN_ADDED_BROKEN at mx dot google dot com> <52CAF71D dot 3050008 at redhat dot com> <52cbe1c5 dot 67ed440a dot 67e6 dot 7288SMTPIN_ADDED_BROKEN at mx dot google 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