This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [RFA-v2] Fix PR 16201: internal error on a cygwin program linked against a DLL with no .data section
- From: "Pierre Muller" <pierre dot muller at ics-cnrs dot unistra dot fr>
- To: "'Pedro Alves'" <palves at redhat dot com>
- Cc: "'Joel Brobecker'" <brobecker at adacore dot com>, <gdb-patches at sourceware dot org>
- Date: Sun, 22 Dec 2013 23:55:56 +0100
- Subject: RE: [RFA-v2] 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>
> -----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)