From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id E744C384803A for ; Thu, 4 Mar 2021 09:00:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E744C384803A X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 0F376AAC5; Thu, 4 Mar 2021 09:00:58 +0000 (UTC) Subject: Re: [PATCH 3/6] bfd: refine handling of relocations between debugging sections To: Alan Modra Cc: Binutils References: <67c184ec-e370-46ee-46d3-bd001ef80445@suse.com> <20210304061023.GR6042@bubble.grove.modra.org> From: Jan Beulich Message-ID: <41f2d3d5-c98b-7db5-8c91-26b7ceb780e3@suse.com> Date: Thu, 4 Mar 2021 10:00:58 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210304061023.GR6042@bubble.grove.modra.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3033.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: binutils@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 Mar 2021 09:01:00 -0000 On 04.03.2021 07:10, Alan Modra wrote: > On Tue, Mar 02, 2021 at 10:48:30AM +0100, Jan Beulich via Binutils wrote: >> Preliminary remark: While relevant for Dwarf in particular, I would >> assume other debug info formats have similar implications. If not, I've >> no idea how to correctly deal with the Dwarf case. >> >> Dwarf wants references between the various .debug_* sections to be >> section relative. ELF, however, has section relative relocations on only >> very few architectures. Hence normal 32-bit / 64-bit data relocations >> get used (the ones with correspond to BFD_RELOC_{32,64}). For ELF output >> this is not a problem by default as all these sections get placed at VA >> zero. For PE output using VA 0 is not an option, as that would place the >> section below the image base (see also "bfd: don't silently wrap or >> truncate PE image section RVAs"). And even for ELF output this can be a >> problem if these sections get assigned real VAs, e.g. when a program or >> library wants to be able to access its own debug info. > > So this is for linking ELF with debug info into PE output? It's unavoidable for this case, yes, but as said even for ELF it can turn out to be necessary (when the .debug_* sections get assigned a non-zero [and large enough] VA). >> --- a/bfd/reloc.c >> +++ b/bfd/reloc.c >> @@ -749,6 +749,30 @@ bfd_perform_relocation (bfd *abfd, >> else >> output_base = reloc_target_output_section->vma; >> >> + /* Most architectures have no section relative ELF relocations. They use >> + ordinary ones instead for representing section relative references between >> + debugging sections, which works fine as long as the section VMA gets set >> + to zero. While this is the default for ELF output (albeit not a >> + requirement), in particular PE doesn't even allow zero VMAs for any of the >> + sections. */ >> + if(output_base && !howto->pc_relative >> + && bfd_get_flavour (abfd) == bfd_target_elf_flavour >> + && (reloc_target_output_section->flags >> + & input_section->flags & SEC_DEBUGGING)) >> + { >> + /* Since this is a heuristic, apply further checks in an attempt to >> + exclude relocation types other than simple base ones. */ >> + unsigned int size = bfd_get_reloc_size (howto); >> + >> + if (size && !(size & (size - 1)) >> + && !(howto->bitsize & (howto->bitsize - 1)) >> + && !howto->bitpos && !howto->rightshift >> + && !howto->negate && !howto->partial_inplace >> + && !(howto->src_mask & (howto->src_mask + 1)) >> + && !(howto->dst_mask & (howto->dst_mask + 1))) >> + output_base = 0; >> + } >> + >> output_base += symbol->section->output_offset; >> >> /* If symbol addresses are in octets, convert to bytes. */ > > When we need this sort of horrible hack, it's time to redesign. Well, I was fearing that, but I have to admit I have no idea what would need doing where. Surely the "horrible" aspects could be reduced by limiting the number of extra checks - as said in comment and description, I've added them to reduce risk of mistakenly zapping output_base. But I definitely agree that no matter how much massaging would be done, it's probably going to remain ugly without finding an entirely different approach. This said, I had to admit that I was actually inspired by this code a few lines up from where I did the addition: /* Convert input-section-relative symbol value to absolute. */ if ((output_bfd && ! howto->partial_inplace) || reloc_target_output_section == NULL) output_base = 0; I may not fully understand the reasons behind it, but it as well did look like a hack to me. Jan