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

Thu Jun 8 21:27:55 PDT 2023

MaskRay requested changes to this revision.
MaskRay added a comment.
This revision now requires changes to proceed.

Thank you for the contribution. This is very well-written. I've put up some comments and will review some technical corners more carefully.

================
Comment at: lld/ELF/Arch/LoongArch.cpp:31
+static uint64_t getLoongArchPage(uint64_t p) {
+  return p & ~static_cast<uint64_t>(0xFFF);
+}
----------------
It seems that our codebase is not consistent with 0x[a-f] and 0x[A-F], but [a-f] is more common and probably should be preferred for new code.

================
Comment at: lld/ELF/Arch/LoongArch.cpp:83
+
+const uint64_t dtpOffset = 0;
+
----------------
Delete this variable and adjust use sites.

================
Comment at: lld/ELF/Arch/LoongArch.cpp:227
+    if ((getEFlags(f) & EF_LOONGARCH_OBJABI_MASK) != EF_LOONGARCH_OBJABI_V1)
+      error(toString(f) + ": unsupported object file ABI version");
+  }
----------------
This is untested. You can reuse `loongarch-elf-flags.s`

================
Comment at: lld/ELF/Arch/LoongArch.cpp:260
+void LoongArch::writeGotHeader(uint8_t *buf) const {
+  if (config->is64)
+    write64le(buf, mainPart->dynamic->getVA());
----------------
glibc rtld does not use this field. We can delete this override. 

================
Comment at: lld/ELF/Arch/LoongArch.cpp:341
+  case R_LARCH_NONE:
+  case R_LARCH_MARK_LA:
+  case R_LARCH_MARK_PCREL:
----------------
What are R_LARCH_MARK_LA and R_LARCH_MARK_PCREL? They are untested.

================
Comment at: lld/ELF/Arch/LoongArch.cpp:382
+    // back in September 2022 [2]. But the pull request has not received any
+    // comment since then, and object code with this usage will soon appear with
+    // the release of glibc 2.37, so we have no choice but to carry the same
----------------
https://www.gnu.org/software/libc/ glibc 2.37 has been release. This sentence should be revised.

================
Comment at: lld/ELF/Arch/LoongArch.cpp:487
+  case R_LARCH_TLS_GD_HI20: {
+    // TODO: checkInt(loc, SignExtend64(hi, bits) >> 12, 20, rel);
+    write32le(loc, setJ20(read32le(loc), extractBits(val, 31, 12)));
----------------
xen0n wrote:
> xen0n wrote:
> > MQ-mengqing wrote:
> > > Why so many TODOs here?
> > > 
> > Because this patch is clearly marked as `[WIP]`? I'm just posting the in-progress work early in order to not overwhelm everyone later in the review cycle.
> I've confirmed no relocs other than `R_LARCH_B{16,21,26}` need overflow/alignment checking (because the widest i.e. 64-bit relocs reuse all three lower reloc types, the lower types cannot check for overflow, and the highest type doesn't overflow at all). All such TODOs are now removed.
These cases are probably not useful.
Reporting `unknown relocation ` for deprecated/essentially-unused relocation types should be fine. 

================
Comment at: lld/ELF/Driver.cpp:172
           .Cases("elf32lppc", "elf32lppclinux", {ELF32LEKind, EM_PPC})
+          .Case("elf32loongarch", {ELF32LEKind, EM_LOONGARCH})
           .Case("elf64btsmip", {ELF64BEKind, EM_MIPS})
----------------
`elf32loongarch` is untested. Perhaps add RUN lines to emulation-loongarch.s.  loongarch-elf-flags.s is possibly not needed.

================
Comment at: lld/ELF/InputSection.cpp:682
+  case R_LOONGARCH_GOT_PAGE_PC:
+    // The R_LARCH_GOT64_PC_* relocs are also reused for TLS GD.
+    if (sym.hasFlag(NEEDS_TLSGD))
----------------
Is `The R_LARCH_GOT64_PC_* relocs are also reused for TLS GD.` accurate? Is this just `R_LOONGARCH_GOT_PAGE_PC`?

================
Comment at: lld/ELF/InputSection.cpp:684
+    if (sym.hasFlag(NEEDS_TLSGD))
+      // Same as R_LOONGARCH_GOT_TLSGD_PC in this case.
+      return getLoongArchPageOffset(in.got->getGlobalDynAddr(sym) + a, p);
----------------
`R_LOONGARCH_GOT_TLSGD_PC` does not exist.

================
Comment at: lld/ELF/Relocations.cpp:1065
       in.mipsGot->addEntry(*sec->file, sym, addend, expr);
+    } else if (config->emachine == EM_LOONGARCH) {
+      // Many LoongArch TLS relocs reuse the R_LOONGARCH_GOT type, in which case
----------------
This can be simplified as `else if (!sym.isTls() || config->emachine != EM_LOONGARCH)`

================
Comment at: lld/ELF/ScriptParser.cpp:442
       .Case("elf32-msp430", {ELF32LEKind, EM_MSP430})
+      .Case("elf32-loongarch", {ELF32LEKind, EM_LOONGARCH})
+      .Case("elf64-loongarch", {ELF64LEKind, EM_LOONGARCH})
----------------
Test this with `OUTPUT_FORMAT` linker script command.

================
Comment at: lld/test/ELF/loongarch-branch-b16.s:10
+# RUN: llvm-objdump -d %t.la64 | FileCheck %s --check-prefix=CHECK-64
+# CHECK-32: 00 04 00 58   beq     $zero, $zero, 4
+# CHECK-32: 00 fc ff 5f   bne     $zero, $zero, -4
----------------
The encoding is unneeded detail and may change if someone decides to remove spaces between hex pairs. Run llvm-objdump with `--no-show-raw-insn`

================
Comment at: lld/test/ELF/loongarch-pc-aligned.s:15
+#--- case1.t
+SECTIONS {
+ .rodata 0x1000: { *(.rodata) }
----------------
Consider `--section-start`

================
Comment at: lld/test/ELF/loongarch-pcala-lo12-jirl-shared-error.s:1
+# REQUIRES: loongarch
+
----------------
`foo-error.s` can usually be added into `foo.s` by leveraging `split-file`.

================
Comment at: lld/test/ELF/loongarch-tls-gd-and-ie.s:1
+# REQUIRES: loongarch
+
----------------
What does loongarch-tls-gd-and-ie.s mean?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138135