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

[Job Noorman via Phabricator via llvm-commits]

jobnoorman marked 4 inline comments as done.
jobnoorman added inline comments.


================
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();
   }
----------------
lhames wrote:
> MaskRay wrote:
> > jobnoorman wrote:
> > > 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.
> > An absolute symbol is defined in ELF. `isExternal` seems Mach-O specific and is not meaningful for ELF.
> `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.
Updated to `!isDefined()` to include absolute symbols.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149525