[PATCH] D138135: [lld][ELF] Support LoongArch

Mon Jul 10 04:05:13 PDT 2023

xen0n added inline comments.

================
Comment at: lld/ELF/Arch/LoongArch.cpp:341
+  case R_LARCH_NONE:
+  case R_LARCH_MARK_LA:
+  case R_LARCH_MARK_PCREL:
----------------
xen0n wrote:
> MaskRay wrote:
> > What are R_LARCH_MARK_LA and R_LARCH_MARK_PCREL? They are untested.
> AFAIK they serve as markers for external consumers only: `R_LARCH_MARK_LA` is emitted by binutils for each `la.abs`, and used by (old) Linux and grub to perform their own relocation; `R_LARCH_MARK_PCREL` is unused so far. Are test cases still needed in this case?
I've now added `R_LARCH_MARK_LA`'s in the `loongarch-abs64.s` cases. I can't seem to find any usage of `R_LARCH_MARK_PCREL` in the wild, not even in the old world binaries, so I'm not sure if removing support for it (instead of treating as no-op per common sense) would be better.

================
Comment at: lld/ELF/Arch/LoongArch.cpp:37
+// as negative, because the instructions consuming it (ld, st, addi, etc.)
+// all sign-extend the immediate, unlike AArch64. The higher bits need
+// tweaking too, due to potential usage in patterns like:
----------------
MaskRay wrote:
> "The higher bits need ..." appears to describe `result -= 0x100000000` but is not clear. An example will work better
Thanks, I've discovered even the BFD implementation is wrong for some of the edge cases. (This implementation needs a fix as well.) I've added an example and fixed the logic in the latest revision.

================
Comment at: lld/ELF/Arch/LoongArch.cpp:360
+  case R_LARCH_PCALA_LO12:
+    // This may well be just R_ABS and nothing more, but unfortunately some
+    // people had the brilliant idea of reusing the R_LARCH_PCALA_LO12 reloc on
----------------
MaskRay wrote:
> This wording may sound irony for a public project. Also, the paragraph is too long.
> 
> We could just R_ABS, but the JIRL instruction reuses the relocation type for a different purpose.
> This questionable use case is part of glibc 2.37 Scrt1.o (is it true?), which is linked into user programs, so we have to work around it for a while, even if a new relocation type may be introduced in the future.
> 
> I wonder why the medium code model designer decides to use pcalau12i+jirl. Range extension thunks are much better.
I added the rant following `R_386_GOT32` and `R_386_GOT32X`'s precedent. I agree though that such wording would better belong somewhere else (e.g. in an issue raised in the ABI specs repo) and I'll use your suggested wording instead.

================
Comment at: lld/ELF/Arch/LoongArch.cpp:394
+    // [1]: https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=9f482b73f41a9a1bbfb173aad0733d1c824c788a
+    // [2]: https://github.com/loongson/LoongArch-Documentation/pull/69
+    return isJirl(read32le(loc)) ? R_PLT : R_ABS;
----------------
MaskRay wrote:
> This link gives "This repository has been archived by the owner on Apr 20, 2023. It is now read-only.". Is it stale?
> 
> Is R_LARCH_JIRL_LO12 still planned?
No, AFAIK the repo's information is still current and the repo isn't going away. They archived it for non-technical reasons (maybe not giving the impression that people outside of Loongson can change their official docs at will, before their CLA automation is ready).

I've nevertheless archived the page with archive.org and replaced with a new PR to the document's current home. I expect the discussion to continue but only after they get their CLA mechanism finished.

================
Comment at: lld/ELF/Arch/LoongArch.cpp:437
+    return R_LOONGARCH_TLSGD_PAGE_PC;
+  case R_LARCH_PCALA_HI20:
+    // Why not R_LOONGARCH_PAGE_PC, majority of references don't go through PLT
----------------
MaskRay wrote:
> It seems that https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html  is wrong about `GR[rd] = PC + SignExtend({si20, 12'b0}, GRLEN)`.
> 
> The low 12-bits of GR[rd] is zeroed.
You may be looking at `pcaddu12i`? That's the same as RISCV `auipc`. The `PCALA` relocs all use `pcalau12i` that indeed has the lower bits zeroing behavior mentioned. See [[ https://github.com/loongson/LoongArch-Documentation/blob/main/docs/LoongArch-Vol1-EN/basic-integer-instructions/overview-of-basic-integer-instructions/arithmetic-operation-instructions.adoc#pcaddi-pcaddu121-pcaddu18l-pcalau12i | the corresponding documentation source ]].

================
Comment at: lld/ELF/Arch/LoongArch.cpp:451
+    // apart that relationship is not certain anymore), and programmer mistakes
+    // (e.g. as outlined in https://github.com/loongson/LoongArch-Documentation/pull/69).
+    //
----------------
MaskRay wrote:
> https://github.com/loongson/LoongArch-Documentation/pull/69 is now a stale link as the repo is archived and may be deleted in the future.
The repo isn't going anywhere, as explained in another reply, but I've replaced with a new refreshed PR on the currently maintained LA ABI specs repo nevertheless (I've replicated all info there so no context is lost even if the original repo is deleted).

================
Comment at: lld/test/ELF/loongarch-pc-aligned.s:8
+
+# RUN: ld.lld %t/a.la32.o -T %t/case1.t -o %t/case1.la32
+# RUN: ld.lld %t/a.la64.o -T %t/case1.t -o %t/case1.la64
----------------
MaskRay wrote:
> You can use something like `--section-start .rodata=0x11000 --section-start=.text=0x11ffc` to replace the linker script.
> You can use something like `--section-start .rodata=0x11000 --section-start=.text=0x11ffc` to replace the linker script.

Sorry, but as I've previously explained as soon as I replace the linker script with the `--section-start` arguments that really should be equivalent, I instead get errors like `ld.lld: error: output file too large: 18446744073709494880 bytes`. This doesn't happen with the BFD linker so I suspect something is wrong with the common logic, although I haven't debugged yet.

For now though I've discovered your suggestion somehow works (by ensuring the base address to be always greater than `defaultMaxPageSize` i.e. `0x10000`), and I've amended the cases.

================
Comment at: lld/test/ELF/loongarch-pc-aligned.s:20
+
+# RUN: ld.lld %t/a.la32.o -T %t/case2.t -o %t/case2.la32
+# RUN: ld.lld %t/a.la64.o -T %t/case2.t -o %t/case2.la64
----------------
MaskRay wrote:
> I hope that we can reduce the number of ld.lld invocations. 
> 
> Does it help to increase the number of instructions in `.text`?
> ```
>   pcalau12i $a0, %pc_hi20(x)
>   pcalau12i $a0, %pc_hi20(x)
>   ld.w      $a0, $a0, %pc_lo12(x)
>   ld.w      $a0, $a0, %pc_lo12(x)
> ```
Probably no, I have to ensure sufficiently different PC and target values for each case, in order to be able to cover the edge cases. Adding a trivial number of instructions won't help (i.e. I can't put the target value 4GiB away without utilizing `--section-start` or linker scripts anyway).

Do you mean I should just leave the PC alone and use `--defsym` for testing multiple offsets at once?

================
Comment at: lld/test/ELF/loongarch-pc-aligned.s:130
+# RUN: llvm-objdump -d --no-show-raw-insn %t/extreme2 | FileCheck %s --check-prefix=EXTREME2
+# EXTREME2:      addi.d    $t0, $zero, -820
+# EXTREME2-NEXT: lu32i.d   $t0, -69907
----------------
MaskRay wrote:
> This is complex. This needs a comment to explain how these immediate mean.
I've (programmatically) generated more cases for complete coverage of `getLoongArchPageDelta` (previously `getLoongArchPageOffset`), complete with explanations for every immediate there.

================
Comment at: lld/test/ELF/loongarch-pc-aligned.s:169
+_start:
+  addi.d    $t0, $zero, %pc_lo12(x)
+  lu32i.d   $t0, %pc64_lo20(x)
----------------
MaskRay wrote:
> You can define a macro to repeat the code sequence 3 times so that we don't need many `extreme*.t`
The linker script inputs are all gone in the latest revision. Can't seem to reduce lld invocations though, happy to see any suggestion.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138135/new/

https://reviews.llvm.org/D138135