From: "Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>
To: "'Pedro Alves'" <palves@redhat.com>
Cc: "'Joel Brobecker'" <brobecker@adacore.com>, <gdb-patches@sourceware.org>
Subject: [COMMIT-v4] Fix PR 16201: internal error on a cygwin program linked against a DLL with no .data section
Date: Tue, 07 Jan 2014 23:40:00 -0000 [thread overview]
Message-ID: <002e01cf0c01$ce262260$6a726720$@muller@ics-cnrs.unistra.fr> (raw)
In-Reply-To: <52CC6A80.1000906@redhat.com>
> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : mardi 7 janvier 2014 21:59
> À : Pierre Muller
> Cc : 'Joel Brobecker'; gdb-patches@sourceware.org
> Objet : Re: [RFA-v3] Fix PR 16201: internal error on a cygwin program
> linked against a DLL with no .data section
>
> 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.
OK, thanks.
> >
> >> 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.
Yes, but there is all this rva stuff that is also going on on the same
time...
I am pretty sure that duplicate section names are not possible in
a linker generated Windows PE file.
> >
> >>> + 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?
OK, I removed that part completely,
as the problem reported is fixed without.
> > @@ -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.
Changed comment to BFD section number
> > @@ -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 ?)
I also suppose that both arrays are the same, but
I also did not want to take the risk of dereferencing
a NULL pointer...
> Otherwise looks fine to me.
Thanks for the approval,
Pierre
For the record, this is what I committed:
2014-01-08 Pierre Muller <muller@sourceware.org>
Fix PR16201.
* coff-pe-read.c (struct read_pe_section_data): Add index field.
(add_pe_exported_sym): Use SECTION_DATA->INDEX for call
to prim_record_mininal_symbol_and_info.
(add_pe_forwarded_sym): Use known section number of forwarded symbol
in call to prim_record_minimal_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 749c109..0fcd15f 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; /* BFD section number. */
char *section_name; /* Recorded section name. */
};
@@ -93,7 +94,7 @@ read_pe_section_index (const char *section_name)
}
}
-/* Get the index of the named section in our own full arrayi.
+/* Get the index of the named section in our own full array.
text, data and bss in that order. Return PE_SECTION_INDEX_INVALID
if passed an unrecognised 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);
@@ -209,6 +212,7 @@ add_pe_forwarded_sym (const char *sym_name, const char
*forward_dll_name,
int forward_func_name_len = strlen (forward_func_name);
int forward_len = forward_dll_name_len + forward_func_name_len + 2;
char *forward_qualified_name = alloca (forward_len);
+ short section;
xsnprintf (forward_qualified_name, forward_len, "%s!%s",
forward_dll_name,
forward_func_name);
@@ -242,6 +246,7 @@ add_pe_forwarded_sym (const char *sym_name, const char
*forward_dll_name,
vma = SYMBOL_VALUE_ADDRESS (msymbol.minsym);
msymtype = MSYMBOL_TYPE (msymbol.minsym);
+ section = SYMBOL_SECTION (msymbol.minsym);
/* Generate a (hopefully unique) qualified name using the first part
of the dll name, e.g. KERNEL32!AddAtomA. This matches the style
@@ -254,10 +259,12 @@ add_pe_forwarded_sym (const char *sym_name, const char
*forward_dll_name,
qualified_name = xstrprintf ("%s!%s", dll_name, bare_name);
- prim_record_minimal_symbol (qualified_name, vma, msymtype, objfile);
+ prim_record_minimal_symbol_and_info (qualified_name, vma, msymtype,
+ section, objfile);
/* Enter the plain name as well, which might not be unique. */
- prim_record_minimal_symbol (bare_name, vma, msymtype, objfile);
+ prim_record_minimal_symbol_and_info (bare_name, vma, msymtype,
+ section, objfile);
xfree (qualified_name);
xfree (bare_name);
@@ -455,17 +462,25 @@ 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;
+ section_data[sectix].index = bfd_section_index;
}
else
{
@@ -479,6 +494,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)
prev parent reply other threads:[~2014-01-07 23:40 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-11 10:50 [PING RFA] " Pierre Muller
2013-12-11 17:02 ` Joel Brobecker
2013-12-13 21:40 ` [RFA-v2] " Pierre Muller
2013-12-18 3:55 ` Joel Brobecker
[not found] ` <52ab7ec0.c8da420a.12c6.ffffb3f4SMTPIN_ADDED_BROKEN@mx.google.com>
2013-12-20 18:19 ` Pedro Alves
2013-12-22 22:56 ` Pierre Muller
[not found] ` <15250.2735647888$1387752987@news.gmane.org>
2013-12-23 2:31 ` asmwarrior
[not found] ` <52b76e14.8886420a.29e6.ffffddb2SMTPIN_ADDED_BROKEN@mx.google.com>
2014-01-06 18:34 ` Pedro Alves
2014-01-07 11:15 ` [RFA-v3] " Pierre Muller
[not found] ` <52cbe1c5.67ed440a.67e6.7288SMTPIN_ADDED_BROKEN@mx.google.com>
2014-01-07 20:58 ` Pedro Alves
2014-01-07 23:40 ` Pierre Muller [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='002e01cf0c01$ce262260$6a726720$@muller@ics-cnrs.unistra.fr' \
--to=pierre.muller@ics-cnrs.unistra.fr \
--cc=brobecker@adacore.com \
--cc=gdb-patches@sourceware.org \
--cc=palves@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).