[PATCH] D149525: [JITLink][RISCV] Only generate PLT entries for external symbols

[Job Noorman via Phabricator via llvm-commits]

jobnoorman added inline comments.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:74
     assert(E.getKind() == R_RISCV_CALL_PLT && "Not a R_RISCV_CALL_PLT edge?");
     E.setKind(R_RISCV_CALL);
     E.setTarget(PLTStubs);
----------------
MaskRay wrote:
> Use `R_RISCV_CALL_PLT`?
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.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:80
+    return (E.getKind() == R_RISCV_CALL || E.getKind() == R_RISCV_CALL_PLT) &&
+           E.getTarget().isExternal();
   }
----------------
StephenFan wrote:
> MaskRay wrote:
> > 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.
> It seems `isExternal` can exclude absolute symbols.
Indeed, so I would suggest to keep it like this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149525