Re: [PATCH 1/5 v1] LoongArch: bfd: Add new reloc types.

[liuzhensong <liuzhensong@loongson.cn>]

在 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 <base>`; 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 "<purpose>_<starting bit position>_<destination slot>".
> *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?

This relocation is expected to do some optimizations, do or not won't 
affect correctness. The linker has implemented some specific 
instructions, but the assembler still has some support that has not been 
completed. If it is deleted, the current version linker can not to link 
next version object (maybe soon to be patched).

>
> <snip>
>
>>     /* 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)?