[PATCH] D90132: [ORC] Prototype ORC library reorg + RPC based TargetProcessControl.

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz added a comment.

Last issue: EH-frame registration isn't ready for ELF right? That would be great to fix (see the unticked inline comment).
I can offer to check the mangling prefix details once this had landed.



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/OrcRPCTPCServer.h:559-562
+#ifdef __APPLE__
+        if (*Sym != '\0')
+          ++Sym;
+#endif
----------------
lhames wrote:
> dblaikie wrote:
> > lhames wrote:
> > > dblaikie wrote:
> > > > What's this about?
> > > It's the MachO leading-underscore demangling, but it would be better written as `if (*Sym != '_') ...;`. Updated. 
> > Hmm - it strikes me as odd because it's using a compile-time feature rather than say, an API query to test whether the binary format is MachO. I guess that's because of the context of this being on the machine where the code will execute and thus __APPLE__ is synonymous with MachO at that point? If it were possible to rely on some other property like the type of the object file where the symbol came from, that might make it a bit easier to test portably or less error-prone in some weird future where someone might be testing different object formats, etc?
> To be clear: This isn't the regular JIT lookup path, this is an RPC wrapper for dlsym in the target process. There's no object file to look at. We're taking a JIT symbol string (always linker mangled) and trying to look it up using dlsym, which expects a C symbol name. If the target process is MachO, which is determinable at compile time and synonymous with `__APPLE__`, then we need to apply MachO "demangling", i.e. dropping the leading underscore. A runtime check wouldn't add anything here.
We shouldn't ignore use cases like ELF on `__APPLE__` and ELF on `_WIN32`. Here symbols don't come with the appropriate mangling prefix, which means that the lookup logic becomes a little more complicated:
* `printf` should get prefixed and resolve to `_printf`
* my own function `foo` shouldn't and instead resolve to plain `foo`

I assume that foo will be resolved on the jitlink side, because the symbol is defined in user code. Thus, the request for it won't make it to the jitlink-executor side in the first place. OTOH printf will be looked up here on the jitlink-executor side, so adding the mangling prefix is correct.

There is a similar logic in SelfTargetProcessControl, where we add the GlobalManglingPrefix unconditionally if the process triple's default object format is MachO.

Maybe this is beyond the scope of this initial version, so I'd propose to investigate it in more detail and come up with a follow-up patch.


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:625
+    char *const Args[] = {ExecutorPath.get(), FDSpecifier.get(), nullptr};
+    int RC = execvp(ExecutorPath.get(), Args);
+    if (RC != 0) {
----------------
lhames wrote:
> sgraenitz wrote:
> > lhames wrote:
> > > sgraenitz wrote:
> > > > What is the advantage of using `execvp` over `execv`? (The latter is what lli did.) I think the way it is now, the default "llvm-jitlink-executor" will only be found if the directory is in PATH.
> > > > 
> > > > Changing this to `execv` I can run a hello world example with the default executor setting once I built both binaries. With `execvp` the default fails, but this still works:
> > > > ```
> > > > ./llvm-jitlink -oop-executor=./llvm-jitlink-executor ../hello_world.o
> > > > ```
> > > I like execvp, since it means that lookup for the executor program is going to follow the same paths as the lookup for llvm-jitlink itself. Maybe the best option would be to use execvp but default to a full path to llvm-jitlink-executor?
> > > 
> > > I'd be inclined to update lli to match this logic.
> > > 
> > > I don't have strong feelings about this though. If most people find execv preferable then I am happy to change it. 
> > I have no strong opinion on it either, except that the default setting should work. If we keep execvp, then the default value for `OOPExecutor` should be `./llvm-jitlink-executor` right?
> I think it should default to the same relative path as llvm-jitlink, since that will be the standard build/install path. I've switched to this in the latest version.
Ok I see, this makes it independent from the current working directory. Great!


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:817
 
-  if (!NoExec && !TT.isOSWindows())
+  if (!NoExec && !this->TPC->getTargetTriple().isOSWindows())
     ObjLayer.addPlugin(std::make_unique<EHFrameRegistrationPlugin>(
----------------
sgraenitz wrote:
> I think this only works for `isOSBinFormatMachO()` at the moment. Current symptom when targeting ELF:
> ```
> bin/llvm-jitlink: Missing definition for llvm_orc_registerEHFrameSectionWrapper
> ```
> 
This note here is still open. EH-frame registration didn't work for me on Linux. With the proposed fix it does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90132