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?
>
> <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)?
This may be caused by hand error, the next commit will revert.