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

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

sgraenitz added a comment.

Great that there is so fast progress! Haven't reviewed in detail (yet), but it looks like there are no conceptual changes compared to the proposal we know? A few notes inline.



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/Shared/TargetProcessControlTypes.h:110
+  WrapperFunctionResult(WrapperFunctionResult &&Other)
+      : R({0, {.ValuePtr = nullptr}, nullptr}) {
+    std::swap(R, Other.R);
----------------
I am getting compile warnings here too (Apple clang-1100.0.33.12). On my machine it says "designated initializers are a C99 feature" :-) One more in OrcRPCTPCServer.h


================
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) {
----------------
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
```


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:627
+    if (RC != 0) {
+      errs() << "unable to launch out-of-process executor\n";
+      exit(1);
----------------
Maybe this could be:
```
      errs() << "unable to launch out-of-process executor: "
             << ExecutorPath.get() << "\n";
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90132