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

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

sgraenitz added a comment.

> I'm curious to get your take on the library breakdown and corresponding headers too.

There's a number of includes still pointing to `#include "llvm/ExecutionEngine/Orc/TargetProcess/XY"`. Otherwise, yes LGTM.

> This achieves the initial goal that executors should not need to link Orc

+1 very reasonable!



================
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:
> > 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90132