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

[WANG Xuerui <i.swmail@xen0n.name>]

(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)?