[PATCH] D144276: [ORC] Introduce SetUpExecutorNativePlatform utility.

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/COFFPlatform.h:45
+         JITDylib &PlatformJD,
+         std::unique_ptr<MemoryBuffer> OrcRuntimeArchiveBuffer,
+         LoadDynamicLibrary LoadDynLibrary, bool StaticVCRuntime = false,
----------------
bzcheeseman wrote:
> The other platforms use a DefinitionGenerator, any particular reason to not do that here?
`COFFPlatform` currently uses the archive twice: Once for the usual purpose, and a second time to get the "atexit" implementation object that it adds for each `JITDylib`.

As an aside: it turns out that this atexit approach is incorrect (at least for ELF and MachO, I suspect for COFF too): We're using it to run atexits when the library containing the call to atexit is closed, but we should be running them when the library containing the atexit argument is closed. To fix this we should implement dladdr in the ORC runtime and use it to determine what library contains the argument function. 


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/LLJIT.h:456
+/// native target for the executor.
+class SetUpExecutorNativePlatform {
+public:
----------------
sunho wrote:
> Something worth adding to this interface for COFF side of view is add constructor that receive vcruntime path. Without manually shipping them program will not run for people that do not have vc toolchain installed in their computer. Even better instead of path receive memorybuffer so that ORC users can embed vcruntime libs.
> 
> Maybe this class is not right place to add though as its supposed to be generic it seems but we do have this kind of use case.
I meant to reply earlier: this is a great idea.

@bzcheeseman this is the comment that I wanted to address before I landed the patch, I've just been too busy.

I think we want this class to contain optional paths for all necessary runtimes. E.g. if we added

```
  SetUpExecutorNativePlatform &addVCRuntimePath(std::string VCRuntimePath) {
    this->VCRuntimePath = std::move(VCRuntimePath);
    return *this;
  }
```
and a `VCRuntimePath` member, then in `lli` we could write:

```
Builder.setPlatformSetUp(
  orc::SetUpExecutorNativePlatform(
    OrcRuntime, std::move(PlatformPrepare))
  .addVCRuntimePath("/path/to/VCRuntime"));
```

`SetUpExecutorNativePlatform` could take the union of info for all platforms, then check that it has the paths that it needs to instantiate the required platform at runtime, or error out with an explanation, e.g. "Executor is running Windows but VC runtime path not specified".


================
Comment at: llvm/lib/ExecutionEngine/Orc/LLJIT.cpp:1013
+
+  // FIXME: ORCPlatformSupport isn't going to work for COFF.
+  J.setPlatformSupport(std::make_unique<ORCPlatformSupport>(J));
----------------
sunho wrote:
> It does work in COFF. COFF orc runtime implements a wrapper to provide the same initialize interface like *nix. Room for better design but it does work right now.
Oh cool! Thanks for implementing that @sunho. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144276