From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x631.google.com (mail-pl1-x631.google.com [IPv6:2607:f8b0:4864:20::631]) by sourceware.org (Postfix) with ESMTPS id A36F43858D28 for ; Mon, 18 Apr 2022 16:02:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A36F43858D28 Received: by mail-pl1-x631.google.com with SMTP id t12so12679906pll.7 for ; Mon, 18 Apr 2022 09:02:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=xZOLf/T1zCRWOw57CrwtBgEU5oyqTPbVTDSgWMI71sw=; b=s5yknzSKJVst+ztjm9WCvMq7f++5RAtSxV58U4h7tOWC+Y9vPdDmT6EF8vvtZoYvo2 7K6WtDi61IUy3wtshgk4CbNofKXpNDcV23rm7f6Rd4/ca4ABbMaHIM73CVcBJVuCcM/g Mep9dRD7yXRlx9M1Frg2u2F4PEWc+mWODx25qjnfZvl02N8wE+ESVDa7A2mK/l5T9lEO 1mDT3NInJrfAs81YeTRoUUZgipSDQ3bt4QbIkbk5wXx0dibgsqRbGDJNWFNmXLQGvUr3 18Dv6R/l0UuzD+EoHfRKF5BC6GTqVItT3XKqjOBA34+tdqKnbIXK1UB46qzsCdEBqWow BKqQ== X-Gm-Message-State: AOAM533hkkNvM1aGkRHACg3PJ+KamL4VyC91v9UnSIr9vQibROjRjbos 8ZAfNMnZ1pBmiykZyNjp0RMPZ1dzldqxcQReu0OyKW95mjk= X-Google-Smtp-Source: ABdhPJxn1CiSOFDXQXoHDuQs/p0XEiqPVpwFyEdRC+izJg7NifLwA8xXf5TMYY/qmLiTplZgrhIBkRgAajQO/5E9Vcw= X-Received: by 2002:a17:903:2487:b0:159:bce:4e1a with SMTP id p7-20020a170903248700b001590bce4e1amr1920691plw.4.1650297740517; Mon, 18 Apr 2022 09:02:20 -0700 (PDT) MIME-Version: 1.0 References: <1650070699.hyefqn4i18.none.ref@localhost> <1650070699.hyefqn4i18.none@localhost> In-Reply-To: <1650070699.hyefqn4i18.none@localhost> From: "H.J. Lu" Date: Mon, 18 Apr 2022 09:01:44 -0700 Message-ID: Subject: Re: Non-zero RELA section contents To: "Alex Xu (Hello71)" Cc: Binutils , Jan Beulich Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3025.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Mon, 18 Apr 2022 16:02:23 -0000 On Fri, Apr 15, 2022 at 9:44 PM Alex Xu (Hello71) via Binutils wrote: > > Hi, > > In ELF RELA relocations, the addend is stored in the relocation instead > of the section contents. This was the premise of commit 17c6c3b99156 > ("x86-64/ELF: clear src_mask for all reloc types"); the value in the > section contents is not used. > > I recently encountered a difficult-to-trace bug in 7-Zip, eventually > determined to be caused by JWasm-family assemblers incorrectly putting > the addend in the section contents, and setting the RELA addend to -4. > Previous versions of binutils added the existing value to the relocation > result, producing a working program; new versions of binutils overwrite > it, producing a non-working program. > > Now, I want to be clear: while the relevant specifications are not clear > about what the linker should do in this case, they are reasonably clear > that the assembler should not generate this. The assembler is absolutely > wrong. > > With that being said, I think it would be a good idea for ld to either > revert to the previous behavior or issue a warning or error when > detecting such malformed object files. I think a warning at the least is > appropriate, because binaries are either silently incorrect on the old > version, or silently incorrect on the new version, and silently > incorrect relocations are extremely hard to diagnose. One argument in > favor of reverting to the previous behavior is it is better to preserve > backwards compatibility in BFD linker when the new behavior is not > clearly superior (e.g. faster or closer to spec). One argument in favor > of the new behavior is that it is consistent with LLD and probably gold. > > The following patch adds a warning: > > diff --git a/bfd/reloc.c b/bfd/reloc.c > index 5098e0ab09f..aecdb21ec59 100644 > --- a/bfd/reloc.c > +++ b/bfd/reloc.c > @@ -1509,6 +1509,11 @@ _bfd_relocate_contents (reloc_howto_type *howto, > relocation >>= (bfd_vma) rightshift; > relocation <<= (bfd_vma) bitpos; > > + if (!howto->src_mask && (x & howto->dst_mask)) > + _bfd_error_handler > + (_("warning: %pB: existing value for %s relocation is %lx, expected 0"), > + input_bfd, howto->name, x); > + > /* Add RELOCATION to the right bits of X. */ > x = ((x & ~howto->dst_mask) > | (((x & howto->src_mask) + relocation) & howto->dst_mask)); > > I have tested this patch on a single test file which printed the desired > warning. I believe %lx is probably wrong, but I don't know how to fix > it. I suspect this patch may cause a large number of warnings in certain > cases; however, I think adding a warning here is valid, because the > result is either broken with old binutils or broken with new binutils > (or both), and furthermore, proper assemblers should never generate this > case in the first place. > > I think the primary counterargument is that the point of "either broken > with old binutils or broken with new binutils" primarily applies to > x86-64/ELF, and may not apply to other targets. I am insufficiently > familiar with other targets to say whether this is the case. If so, it > may be better to add this warning only for x86-64/ELF. It isn't wrong to have non-zero value at relocation offset for RELA targets. One can encode optional information at relocation offset for linker as an extension. But it is wrong to store addend value at relocation offset for RELA targets. -- H.J.