-
Notifications
You must be signed in to change notification settings - Fork 15k
[ORC] Add Executor Resolver Utility #143654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lhames, this looks reasonable to me. I'd like move forward with these changes. Can you take a look?
b3ac7cb to
c8febe2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Lookup still used after this patch? I'm not opposed to replacing raw dylib handles with Resolver objects (as long as it doesn't change the dylib lifetimes, which I don't think it does), but if we are doing that then we can drop Lookup entirely and clean up the lookup wrappers and implementations in the target process library.
c8febe2 to
701f14a
Compare
|
Previously, we were returning an error for a required symbol in As you can see below, the error is being silently consumed: JITBuilder->setPrePlatformSetup([](llvm::orc::LLJIT &J) {
// Try to enable debugging of JIT'd code (only works with JITLink for
// ELF and MachO).
consumeError(llvm::orc::enableDebuggerSupport(J));
return llvm::Error::success();
});The comment here shows that debugger support is only implemented for ELF and Mach-O. That explains why the assertion fails on Windows: no valid address is returned in this case. Instead of asserting, we could return an error when the |
6728d5e to
3e781d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo aside, LGTM. Thanks very much for working on this @SahilPatidar!
|
|
||
| // LLVM_EXECUTIONENGINE_ORC_EXECUTORRESOLUTIONGENERATOR_H |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment.
3e781d6 to
7c858e8
Compare
7c858e8 to
71378fb
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/16530 Here is the relevant piece of the build log for the reference |
This patch adds the new executor-side resolver API as suggested by @lhames. It introduces a
DylibSymbolResolverthat helps resolve symbols for each loaded dylib.Previously, we returned a
DylibHandleto the controller. Now, we wrap the native handle insideDylibSymbolResolverand return aResolverHandleinstead. This makes the code cleaner and separates the symbol resolution logic from raw handle management.