From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailbox.box.xen0n.name (mail.xen0n.name [115.28.160.31]) by sourceware.org (Postfix) with ESMTPS id 47D6E3858D28 for ; Mon, 18 Jul 2022 10:07:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 47D6E3858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=xen0n.name Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=xen0n.name DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=xen0n.name; s=mail; t=1658138820; bh=sYSi5gyHmZbSMNL/1AQbzmEy1ai3N+5lS66fEJp0aXU=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=GTSBKF4r0YsV26cfvheGJU+Lp7C4lgEY2a5b6+dZwJJvRGRZNB1KK0o0/giQOKJ8v McgoktMeG6tnJ69FGOuPgS279KugXKvb265LdZ9RTqzDs78pF+hk0KvtxcreyAfOq8 4h8/IJ+4mxIwnfMGQWQpYiCEhmgGmWDKNS4lnvm4= Received: from [100.100.57.190] (unknown [220.248.53.61]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mailbox.box.xen0n.name (Postfix) with ESMTPSA id 74DDE60610; Mon, 18 Jul 2022 18:07:00 +0800 (CST) Message-ID: <8b637076-eb4c-47e0-2987-ac0973e38bca@xen0n.name> Date: Mon, 18 Jul 2022 18:06:59 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:104.0) Gecko/20100101 Thunderbird/104.0a1 Subject: Re: [PATCH 1/5 v1] LoongArch: bfd: Add new reloc types. To: liuzhensong , binutils@sourceware.org Cc: xuchenghua@loongson.cn, mengqinggang@loongson.cn, Xi Ruoyao , Fangrui Song References: <20220718084316.390672-1-liuzhensong@loongson.cn> Content-Language: en-US From: WANG Xuerui In-Reply-To: <20220718084316.390672-1-liuzhensong@loongson.cn> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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 Jul 2022 10:07:08 -0000 (Adding Ruoyao and MaskRay to CC, who might be interested in this development as well, as it concerns the linker implementation.) On 2022/7/18 16:43, liuzhensong wrote: > This is the v1 version of patches to support new relocs for LoongArch. > > The new reloc types docnments are on: > https://github.com/loongson/LoongArch-Documentation/pull/57/files > > The testsuite status: > > === binutils Summary === > > # of expected passes 241 > # of unsupported tests 6 > > === gas Summary === > > # of expected passes 270 > # of unsupported tests 7 > > === ld Summary === > > # of expected passes 1457 > # of expected failures 11 > # of untested testcases 1 > # of unsupported tests 154 > I assume the content above are the "cover letter". BTW: you can generate a proper patch series with `git format-patch -vN --cover-letter `; I think the mails in this series were manually edited by you since the "v1" is wrongly placed (it's usually "[PATCH vN NN/NN]"). > Add new reloc types support, the new relocate type > definition depends which operation needs of linker. "Define new reloc types according to linker needs" could be enough. > > > --- > bfd/bfd-in2.h | 36 ++ > bfd/elfnn-loongarch.c | 1194 ++++++++++++++++++++++------------ > bfd/elfxx-loongarch.c | 1354 +++++++++++++++++++++++++++++---------- > bfd/elfxx-loongarch.h | 4 + > bfd/libbfd.h | 36 ++ > bfd/reloc.c | 79 +++ > include/elf/loongarch.h | 142 +++- > 7 files changed, 2095 insertions(+), 750 deletions(-) > > diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h > index 4e1182e93d4..a71b3e82057 100644 > --- a/bfd/bfd-in2.h > +++ b/bfd/bfd-in2.h > @@ -6274,6 +6274,42 @@ assembler and not (currently) written to any object files. */ > BFD_RELOC_LARCH_SUB24, > BFD_RELOC_LARCH_SUB32, > BFD_RELOC_LARCH_SUB64, > + BFD_RELOC_LARCH_B16, > + BFD_RELOC_LARCH_B21, > + BFD_RELOC_LARCH_B26, > + BFD_RELOC_LARCH_ABS_HI20, > + BFD_RELOC_LARCH_ABS_LO12, > + BFD_RELOC_LARCH_ABS64_LO20, > + BFD_RELOC_LARCH_ABS64_HI12, > + BFD_RELOC_LARCH_PCALA_HI20, > + BFD_RELOC_LARCH_PCALA_LO12, > + BFD_RELOC_LARCH_PCALA64_LO20, > + BFD_RELOC_LARCH_PCALA64_HI12, > + BFD_RELOC_LARCH_GOT_PC_HI20, > + BFD_RELOC_LARCH_GOT_PC_LO12, > + BFD_RELOC_LARCH_GOT64_PC_LO20, > + BFD_RELOC_LARCH_GOT64_PC_HI12, > + BFD_RELOC_LARCH_GOT64_HI20, > + BFD_RELOC_LARCH_GOT64_LO12, > + BFD_RELOC_LARCH_GOT64_LO20, > + BFD_RELOC_LARCH_GOT64_HI12, > + BFD_RELOC_LARCH_TLS_LE_HI20, > + BFD_RELOC_LARCH_TLS_LE_LO12, > + BFD_RELOC_LARCH_TLS_LE64_LO20, > + BFD_RELOC_LARCH_TLS_LE64_HI12, > + BFD_RELOC_LARCH_TLS_IE_PC_HI20, > + BFD_RELOC_LARCH_TLS_IE_PC_LO12, > + BFD_RELOC_LARCH_TLS_IE64_PC_LO20, > + BFD_RELOC_LARCH_TLS_IE64_PC_HI12, > + BFD_RELOC_LARCH_TLS_IE64_HI20, > + BFD_RELOC_LARCH_TLS_IE64_LO12, > + BFD_RELOC_LARCH_TLS_IE64_LO20, > + BFD_RELOC_LARCH_TLS_IE64_HI12, > + BFD_RELOC_LARCH_TLS_LD_PC_HI20, > + BFD_RELOC_LARCH_TLS_LD64_HI20, > + BFD_RELOC_LARCH_TLS_GD_PC_HI20, > + BFD_RELOC_LARCH_TLS_GD64_HI20, I think I've voiced my concerns over the naming of these ops multiple times already; the primary comment ([1]; in English) was posted back in May but no one in your team responded. Reproducing the content (and adjusting a little) here: Overall in a good direction (and IMHO the direction everyone should have taken in the first place), thanks! A few suggestions though: Some of the names can be a bit hard to understand, for example ABS_LO12, ABS_HI20, ABS64_LO20 and ABS64_HI12; these are ambiguous on a first look, do you use the latter two instead of the former for 64-bit absolute relocs? (The answer is "no"; you use all 4. The latter two actually express the higher 32-bit half.) After having trained your intuition to work in "the Loongson way", you'll then have to mentally add the "lower/higher half" and "width" together to get a picture of the bit-field being expressed. It could be better to somehow directly show the 0(LO)/12/32/52 inside those names. Plus, currently it's implied that a particular relocation type is somehow bound to a specific instruction, that's being used by the reference implementation (binutils) for the type today. This is being a bit too specific, and could change, as future instructions emerge with similar or identical format. (BTW no one really understand what "ALA" means in "PCALA"; the insn is named "PCALAU12I", the "U12I" part is similar to "LU12I.W" thus understandable, and PC is obvious, but no explanation for "ALA" exists anywhere). It's better to express the "destination" not in terms of instruction names or vague "LO12/HI20/LO20/HI12" specifiers, but in terms of actual *instruction word slots* being filled. This way it's immediately obvious for anyone what place in the insn word the reloc is going to write to, without having to dig into actual linker code. However, you don't officially have a naming scheme for these, so I guess that's why you have to invent names (and be different with others & prior work). I have drafted my scheme back in November 2021, which I posted at [2] a while ago; the approach is extremely similar (IMO there's only one possible way to implement the more traditional scheme), but my version has adopted the operand slot naming scheme of loongarch-opcodes ([3]; the convention document is edited and posted at [4] for inclusion in official LoongArch docs collection) and the more direct LO/12/32/52 names for bit-field offsets (width is implied by the destination slot). Hope this would be useful. Again, I may sound like a bikeshedder, but IMO better names benefit everyone in the long term. I for one definitely don't want to see all four of "LO12/HI20/LO20/HI12" (they refer to the two 32-bit halves of a 64-bit value, not two ways to segment one 32-bit value), nor "B16/B21/B26" (which are more different than you might think). [1]: https://github.com/loongson/LoongArch-Documentation/pull/50 [2]: https://github.com/loongson/binutils-gdb/issues/134#issuecomment-1110510582 [3]: https://github.com/loongson-community/loongarch-opcodes [4]: https://github.com/loongson/LoongArch-Documentation/pull/56 FYI, I did make a list of my suggested names for these reloc types ("BFD_RELOC_LARCH_" abbreviated to "B_R_L_"): Original name           Suggested name -------------           -------------- B_R_L_B16               B_R_L_PCREL_SK16 *1 B_R_L_B21               B_R_L_PCREL_SD5K16 B_R_L_B26               B_R_L_PCREL_SD10K16 B_R_L_ABS_LO12          B_R_L_ABS_0_SK12 B_R_L_ABS_HI20          B_R_L_ABS_12_SJ20 B_R_L_ABS64_LO20        B_R_L_ABS_32_SJ20 B_R_L_ABS64_HI12        B_R_L_ABS_52_SK12 B_R_L_PCALA_LO12        B_R_L_PCALA_0_SK12 *2 B_R_L_PCALA_HI20        B_R_L_PCALA_12_SJ20 B_R_L_PCALA64_LO20      B_R_L_PCALA_32_SJ20 *3 B_R_L_PCALA64_HI12      B_R_L_PCALA_52_SK12 B_R_L_GOT_PC_LO12       B_R_L_GOT_PCALA_0_SK12 *4 B_R_L_GOT_PC_HI20       B_R_L_GOT_PCALA_12_SJ20 B_R_L_GOT64_PC_LO20     B_R_L_GOT_PCALA_32_SJ20 B_R_L_GOT64_PC_HI12     B_R_L_GOT_PCALA_52_SK12 B_R_L_GOT64_LO12        B_R_L_GOT_ABS_0_SK12 *5 B_R_L_GOT64_HI20        B_R_L_GOT_ABS_12_SJ20 B_R_L_GOT64_LO20        B_R_L_GOT_ABS_32_SJ20 B_R_L_GOT64_HI12        B_R_L_GOT_ABS_52_SK12 B_R_L_TLS_LE_LO12       B_R_L_TLS_LE_ABS_0_SK12 B_R_L_TLS_LE_HI20       B_R_L_TLS_LE_ABS_12_SJ20 B_R_L_TLS_LE64_LO20     B_R_L_TLS_LE_ABS_32_SJ20 B_R_L_TLS_LE64_HI12     B_R_L_TLS_LE_ABS_52_SK12 B_R_L_TLS_IE_PC_LO12    B_R_L_TLS_IE_PCALA_0_SK12 B_R_L_TLS_IE_PC_HI20    B_R_L_TLS_IE_PCALA_12_SJ20 B_R_L_TLS_IE64_PC_LO20  B_R_L_TLS_IE_PCALA_32_SJ20 B_R_L_TLS_IE64_PC_HI12  B_R_L_TLS_IE_PCALA_52_SK12 B_R_L_TLS_IE64_LO12     B_R_L_TLS_IE_ABS_0_SK12 B_R_L_TLS_IE64_HI20     B_R_L_TLS_IE_ABS_12_SJ20 B_R_L_TLS_IE64_LO20     B_R_L_TLS_IE_ABS_32_SJ20 B_R_L_TLS_IE64_HI12     B_R_L_TLS_IE_ABS_52_SK12 B_R_L_TLS_LD_PC_HI20    B_R_L_TLS_LD_PCALA_12_SJ20 *6 B_R_L_TLS_LD64_HI20     B_R_L_TLS_LD_ABS_12_SJ20 B_R_L_TLS_GD_PC_HI20    B_R_L_TLS_GD_PCALA_12_SJ20 B_R_L_TLS_GD64_HI20     B_R_L_TLS_GD_ABS_12_SJ20 *1 PCREL is explicitly called out. As you can see, every reloc type looks like "__". *2 I've reordered the types so that in each group of 4 reloc types it's always from LSB to MSB. And while I don't know what "PCALA" means, I've kept them all, and distinct from "PCREL" because the jumps and PCADDU12I don't clear the lowest 12 bits of destination, thus immediates for use with PCALAU12I needs to be differentiated. *3 With the starting bit position explicit called out, no "64" distinction needs to be made. *4 This (and others) actually uses the PCALAU12I, so should be named accordingly. *5 This (and others) seems like an absolute reloc (it uses the same LU12I.W + xxx + LU32I.D + LU52I.D sequence), so ABS is added to name. *6 I can't immediately make sense of the last 4 reloc types, but I've renamed these too. I would have to dig deeper for understanding the expected use case. And I hope with this proposal, the (bikeshedding) discussion could be well-founded and constructive. > + BFD_RELOC_LARCH_RELAX, I see this is just being reserved but without any actual behavior specified. Is it better to just leave it out for now? > > /* Processor specific flags for the ELF header e_flags field. */ > @@ -102,9 +238,9 @@ END_RELOC_NUMBERS (R_LARCH_count) > #define EF_LOONGARCH_ABI_ILP32_SINGLE_FLOAT 0x6 > #define EF_LOONGARCH_ABI_ILP32_DOUBLE_FLOAT 0x7 > > -#define EF_LOONGARCH_ABI_MASK 0x7 > -#define EF_LOONGARCH_ABI_ILP32_MASK 0x4 > -#define EF_LOONGARCH_ABI_FLOAT_MASK 0x3 > +#define EF_LOONGARCH_ABI_MASK 0x7 > +#define EF_LOONGARCH_ABI_ILP32_MASK 0x4 > +#define EF_LOONGARCH_ABI_FLOAT_MASK 0x3 > #define EF_LOONGARCH_ABI_SOFT_FLOAT_MASK 0x1 > #define EF_LOONGARCH_ABI_SINGLE_FLOAT_MASK 0x2 > #define EF_LOONGARCH_ABI_DOUBLE_FLOAT_MASK 0x3 This hunk looks like pure cosmetic change, could this be split out (and get committed very quickly unlike the other major changes)?