From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31419 invoked by alias); 7 Jan 2014 20:58:51 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 31408 invoked by uid 89); 7 Jan 2014 20:58:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 07 Jan 2014 20:58:49 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s07KwgX9016177 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 7 Jan 2014 15:58:43 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s07KweLE007296; Tue, 7 Jan 2014 15:58:41 -0500 Message-ID: <52CC6A80.1000906@redhat.com> Date: Tue, 07 Jan 2014 20:58:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Pierre Muller CC: "'Joel Brobecker'" , gdb-patches@sourceware.org Subject: Re: [RFA-v3] Fix PR 16201: internal error on a cygwin program linked against a DLL with no .data section References: <004801cef65e$cb82d1b0$62887510$@muller@ics-cnrs.unistra.fr> <20131211170204.GD3227@adacore.com> <52ab7ec0.c8da420a.12c6.ffffb3f4SMTPIN_ADDED_BROKEN@mx.google.com> <52B48A28.2000402@redhat.com> <52b76e14.8886420a.29e6.ffffddb2SMTPIN_ADDED_BROKEN@mx.google.com> <52CAF71D.3050008@redhat.com> <52cbe1c5.67ed440a.67e6.7288SMTPIN_ADDED_BROKEN@mx.google.com> In-Reply-To: <52cbe1c5.67ed440a.67e6.7288SMTPIN_ADDED_BROKEN@mx.google.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-01/txt/msg00165.txt.bz2 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