This is an archive of the discontinued LLVM Phabricator instance.

[ORC] Introduce SetUpExecutorNativePlatform utility.
ClosedPublic

Authored by lhames on Feb 17 2023, 9:38 AM.

Details

Summary

Simplifies the process of building an LLJIT instance that supports the native
platform features (initializers, TLV, etc.).

SetUpExecutorNativePlatform can be passed to LLJITBuilder::setPlatformSetUp
method. It takes a reference to the ORC runtime (as a path or an in-memory
archive) and automatically sets the platform for LLJIT's ExecutionSession based
on the executor process's triple.

This is a rough draft -- we'll need more polish and some tests before this
lands. In particular we need to re-work ORCPlatformSupport to handle
COFF/Windows. We could take this opportunity to do away with the whole
LLJITPlatformSupport concept. It really only exists to implement initialize
and deinitialize, so I think that we could instead define standard functions
"orc_rt_lljit_initialize" and "orc_rt_lljit_deinitialize" and alias them
to platform-specific implementations (a wrapper for dlopen on *nix, a wrapper
for LoadLibrary for Windows, etc.).

Diff Detail

Unit TestsFailed

Event Timeline

lhames created this revision.Feb 17 2023, 9:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2023, 9:38 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
lhames requested review of this revision.Feb 17 2023, 9:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2023, 9:38 AM

I guess this is more generic apporach than https://reviews.llvm.org/D134610.

sunho added inline comments.Feb 17 2023, 11:30 AM
llvm/lib/ExecutionEngine/Orc/LLJIT.cpp
1013

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.

sunho added inline comments.Feb 17 2023, 11:37 AM
llvm/include/llvm/ExecutionEngine/Orc/LLJIT.h
456

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.

bzcheeseman added inline comments.
llvm/include/llvm/ExecutionEngine/Orc/COFFPlatform.h
45

The other platforms use a DefinitionGenerator, any particular reason to not do that here?

lhames added inline comments.Mar 1 2023, 1:49 PM
llvm/include/llvm/ExecutionEngine/Orc/COFFPlatform.h
45

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.

llvm/include/llvm/ExecutionEngine/Orc/LLJIT.h
456

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".

llvm/lib/ExecutionEngine/Orc/LLJIT.cpp
1013

Oh cool! Thanks for implementing that @sunho. :)

bzcheeseman added inline comments.Mar 2 2023, 6:05 PM
llvm/include/llvm/ExecutionEngine/Orc/LLJIT.h
456

I'm happy to do this as a follow up patch if you want, it seems pretty simple.

bzcheeseman accepted this revision.Mar 3 2023, 11:13 AM

I'm happy to help fix up that one remaining issue (@sunho I might ask you for help and will definitely ask you for a review), would love to get this in :)

This revision is now accepted and ready to land.Mar 3 2023, 11:13 AM
This revision was landed with ongoing or failed builds.Mar 19 2023, 3:30 PM
This revision was automatically updated to reflect the committed changes.

Accidentally landed this before it was ready -- my apologies.

I'm discussing the outstanding COFF issues with @sunho now, and I need to figure out where this went wrong on Linux (there were a number of bot failures there), but I hope to have this landed soon and I think it'll be a significant quality-of-life improvement for LLJIT, especially on Windows.

For the Linux failure it looks like test/ExecutionEngine/OrcLazy/emulated-tls.ll just needed -lljit-platform=Inactive added to preserve the existing failure mode. I'll add that when I update the review this evening.

lhames reopened this revision.Mar 26 2023, 10:08 PM
This revision is now accepted and ready to land.Mar 26 2023, 10:08 PM
lhames updated this revision to Diff 508478.Mar 26 2023, 10:08 PM

Update testcase, LLJIT.cpp LoadAndLinkDynLibrary utility.

Apologies for the delay. I've finally gotten back to this and hope to land it next week.

@sunho -- I've added an addVCRuntime member to SetUpExecutorNativePlatform. Any suggestions on the best way to populate it in lli? Should we add a -vc-runtime=<path> option? Is there a reasonable default search order that we could look in? Do we even need to set this by default, or is it optional?

lhames added a comment.Apr 4 2023, 7:41 PM

Quick update: I'm almost done with this -- I just need to fix a couple of outstanding bugs in ELFNixPlatform and then I think this should be good to land.

Quick update: I'm almost done with this -- I just need to fix a couple of outstanding bugs in ELFNixPlatform and then I think this should be good to land.

Thanks for the update! We've been working around by using a temp file, but I'm really excited to get rid of that code :)

This revision was landed with ongoing or failed builds.Apr 6 2023, 9:44 PM
This revision was automatically updated to reflect the committed changes.
lhames added a comment.Apr 7 2023, 7:43 PM

Re-re-committed in 231107a8b5b. Hopefully it sticks this time.

Thanks for your patience with this @bzcheeseman! I'm keen to hear how it works for you when you get a chance to try it.

lhames added a comment.Apr 7 2023, 8:04 PM

Follow-up to remove references to dead test in c5dbbe5e2a2f. Hopefully this fixes the bot failures, e.g. at https://lab.llvm.org/buildbot/#/builders/77/builds/26156.