[JITLink][RISCV] Only generate PLT entries for external symbols

[jobnoorman]

Summary

R_RISCV_CALL has been deprecated. [1] Both GCC and LLVM seem to not
generate it anymore and always use R_RISCV_CALL_PLT (even for calls that
do not need a PLT entry). Generating PLT entries based on relocation
type is not recommended and a better heuristic is to only generate them
when the target symbol is preemptable [2]. This patch implements this by
only generating PLT entries for undefined symbols.

[1] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/340
[2] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/98

[jobnoorman]

Rebase, remove dependency on abandoned revision.

[MaskRay]

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp

74

Use R_RISCV_CALL_PLT?

80

I don't know the object file model of jitlink, but adding a condition is definitely moving toward the right direction.

isExternal looks a bit strange. If the jitlink built object files are used as executables, !isDefined() should be a good enough proxy.

[StephenFan]

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp

80

It seems isExternal can exclude absolute symbols.

[jobnoorman]

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp

74

That indeed feels more correct (given the deprecation of R_RISCV_CALL) but for now both of them work so I'm not sure if that change should be part of this patch.

80

Indeed, so I would suggest to keep it like this.

[MaskRay]

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp

80

An absolute symbol is defined in ELF. isExternal seems Mach-O specific and is not meaningful for ELF.

[lhames]

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp

80

isExternal (in this context) is a LinkGraph concept -- it's not format specific. It just means that the Symbol isn't defined by this graph, it's defined somewhere else.

In this context absolute symbols are not external -- they are defined by the graph (though they don't have any content associated with them).

Calls to absolutes should use a PLT (at least when JITing) as there's no way to know that the graph will be allocated within direct-branch range of the absolute.

[jobnoorman]

Change condition from isExternal() to !isDefined() to make sure PLT entries
are also created for absolute symbols.

[jobnoorman]

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp

80

Updated to !isDefined() to include absolute symbols.

[lhames]

LGTM. Thanks @jobnoorman!

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp

void fixGOTEdge(Edge &E, Symbol &GOTEntry) {
     // with (R_RISCV_PCREL_HI20, R_RISCV_PCREL_LO12)
     // Therefore, here just change the R_RISCV_GOT_HI20 to R_RISCV_PCREL_HI20
     E.setKind(R_RISCV_PCREL_HI20);
     E.setTarget(GOTEntry);
   }
 
   void fixPLTEdge(Edge &E, Symbol &PLTStubs) {
     assert(E.getKind() == R_RISCV_CALL_PLT && "Not a R_RISCV_CALL_PLT edge?");
     E.setKind(R_RISCV_CALL);
     E.setTarget(PLTStubs);
   }
 
   bool isExternalBranchEdge(Edge &E) const {
-    return E.getKind() == R_RISCV_CALL_PLT;
+    return (E.getKind() == R_RISCV_CALL || E.getKind() == R_RISCV_CALL_PLT) &&
+           !E.getTarget().isDefined();
   }
 
 private:
   Section &getGOTSection() const {
     if (!GOTSection)
       GOTSection = &G.createSection("$__GOT", orc::MemProt::Read);
     return *GOTSection;
   }

llvm/test/ExecutionEngine/JITLink/RISCV/ELF_pc_indirect.s

 # RUN: rm -rf %t && mkdir -p %t
 # RUN: llvm-mc -triple=riscv64 -position-independent -filetype=obj \
 # RUN:     -o %t/elf_riscv64_sm_pic_reloc.o %s
 # RUN: llvm-mc -triple=riscv32 -position-independent -filetype=obj \
 # RUN:     -o %t/elf_riscv32_sm_pic_reloc.o %s
 # RUN: llvm-jitlink -noexec \
 # RUN:     -slab-allocate 100Kb -slab-address 0x1ff00000 -slab-page-size 4096 \
-# RUN:     -abs external_func=0x1 -abs external_data=0x2 \
+# RUN:     -abs external_data=0x2 \
 # RUN:     -check %s %t/elf_riscv64_sm_pic_reloc.o
 # RUN: llvm-jitlink -noexec \
 # RUN:     -slab-allocate 100Kb -slab-address 0x1ff00000 -slab-page-size 4096 \
-# RUN:     -abs external_func=0x1 -abs external_data=0x2 \
+# RUN:     -abs external_data=0x2 \
 # RUN:     -check %s %t/elf_riscv32_sm_pic_reloc.o
 #
 # Test ELF small/PIC relocations
 
         .text
         .file   "testcase.c"
 
 # Empty main entry point.
# jitlink-check: decode_operand(test_pcrel32_s+4, 2)[11:0] = (external_data - test_pcrel32_s)[11:0]
         .type   test_pcrel32_s,@function
 test_pcrel32_s:
         auipc a0, %pcrel_hi(external_data)
         sw  a0, %pcrel_lo(test_pcrel32_s)(a0)
 
         .size   test_pcrel32_s, .-test_pcrel32_s
 
 # Test R_RISCV_CALL
-# jitlink-check: decode_operand(test_call, 1) = ((external_func - test_call) + 0x800)[31:12]
-# jitlink-check: decode_operand(test_call+4, 2)[11:0] = (external_func - test_call)[11:0]
-        .globl test_call, external_func
+# jitlink-check: decode_operand(test_call, 1) = ((internal_func - test_call) + 0x800)[31:12]
+# jitlink-check: decode_operand(test_call+4, 2)[11:0] = (internal_func - test_call)[11:0]
+        .globl test_call
         .p2align  1
         .type  test_call,@function
 test_call:
-        .reloc ., R_RISCV_CALL, external_func
+        .reloc ., R_RISCV_CALL, internal_func
         auipc ra, 0
         jalr ra
         ret
         .size test_call, .-test_call
+
+# Test R_RISCV_CALL_PLT
+# jitlink-check: decode_operand(test_call_plt, 1) = ((internal_func - test_call_plt) + 0x800)[31:12]
+# jitlink-check: decode_operand(test_call_plt+4, 2)[11:0] = (internal_func - test_call_plt)[11:0]
+        .globl test_call_plt
+        .p2align  1
+        .type  test_call_plt,@function
+test_call_plt:
+        .reloc ., R_RISCV_CALL_PLT, internal_func
+        auipc ra, 0
+        jalr ra
+        ret
+        .size test_call_plt, .-test_call_plt
+
+       .globl   internal_func
+       .p2align  1
+       .type    internal_func,@function
+internal_func:
+       ret
+       .size internal_func, .-internal_func