From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from loongson.cn (mail.loongson.cn [114.242.206.163]) by sourceware.org (Postfix) with ESMTP id 0A1DA3858439 for ; Tue, 19 Jul 2022 07:32:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0A1DA3858439 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=loongson.cn Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=loongson.cn Received: from [10.20.4.151] (unknown [10.20.4.151]) by mail.loongson.cn (Coremail) with SMTP id AQAAf9Dxj9IiXtZi7ggoAA--.36086S3; Tue, 19 Jul 2022 15:32:52 +0800 (CST) Subject: Re: [PATCH 1/5 v1] LoongArch: bfd: Add new reloc types. To: WANG Xuerui , binutils@sourceware.org Cc: xuchenghua@loongson.cn, mengqinggang@loongson.cn, Xi Ruoyao , Fangrui Song References: <20220718084316.390672-1-liuzhensong@loongson.cn> <8b637076-eb4c-47e0-2987-ac0973e38bca@xen0n.name> From: liuzhensong Message-ID: Date: Tue, 19 Jul 2022 15:32:50 +0800 User-Agent: Mozilla/5.0 (X11; Linux loongarch64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <8b637076-eb4c-47e0-2987-ac0973e38bca@xen0n.name> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-CM-TRANSID: AQAAf9Dxj9IiXtZi7ggoAA--.36086S3 X-Coremail-Antispam: 1UD129KBjvJXoW3uFy8Aw1DWF4kWrWfGFyfWFg_yoWkurW5pF 1vvr43KrWDWF1fu340qF1UCa43Zr18Aw18JryUJFWkZr4kAryYqF4qqrn0gF17tr4kJr18 ZF18ur1fur1jqrJanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUvv14x267AKxVW8JVW5JwAFc2x0x2IEx4CE42xK8VAvwI8IcIk0 rVWrJVCq3wAFIxvE14AKwVWUJVWUGwA2ocxC64kIII0Yj41l84x0c7CEw4AK67xGY2AK02 1l84ACjcxK6xIIjxv20xvE14v26ryj6F1UM28EF7xvwVC0I7IYx2IY6xkF7I0E14v26F4j 6r4UJwA2z4x0Y4vEx4A2jsIE14v26F4UJVW0owA2z4x0Y4vEx4A2jsIEc7CjxVAFwI0_Gc CE3s1le2I262IYc4CY6c8Ij28IcVAaY2xG8wAqx4xG64xvF2IEw4CE5I8CrVC2j2WlYx0E 2Ix0cI8IcVAFwI0_Jw0_WrylYx0Ex4A2jsIE14v26r4j6F4UMcvjeVCFs4IE7xkEbVWUJV W8JwACjcxG0xvEwIxGrwACjI8F5VA0II8E6IAqYI8I648v4I1lc7I2V7IY0VAS07AlzVAY IcxG8wCY02Avz4vE-syl42xK82IYc2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2 IqxVAqx4xG67AKxVWUJVWUGwC20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v2 6r126r1DMIIYrxkI7VAKI48JMIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2 IY6xkF7I0E14v26r4j6F4UMIIF0xvE42xK8VAvwI8IcIk0rVWrJr0_WFyUJwCI42IY6I8E 87Iv67AKxVWUJVW8JwCI42IY6I8E87Iv6xkF7I0E14v26r4j6r4UJbIYCTnIWIevJa73Uj IFyTuYvjfU5nmRUUUUU X-CM-SenderInfo: holx6xphqv003j6o00pqjv00gofq/ X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00, BODY_8BITS, GIT_PATCH_0, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_PASS, 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: Tue, 19 Jul 2022 07:32:58 -0000 在 2022/7/18 下午6:06, WANG Xuerui 写道: > (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)? This may be caused by hand error, the next commit will revert.