[ORC] Add automatic shared library resolver for unresolved symbols. #148410
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
SahilPatidar
changed the title
|
@SahilPatidar, what is still required for this PR to make it ready for review? |
|
Done. We'll improve it based on the review. I had added async locally before, but it would be better if lhames reviews it so we can get a better idea of how to improve it. |
SahilPatidar
force-pushed
the
dyld
branch
3 times, most recently
from
0288f91 to
e0366e0
Compare
vgvassilev
reviewed
Aug 5, 2025
| return *this; | ||
| } | ||
|
|
||
| BloomFilter build(const std::vector<std::string> &symbols) const { |
There was a problem hiding this comment.
Can we avoid the std::string copies? Is there something smarter we can do here?
There was a problem hiding this comment.
can try StringRef or Range?
There was a problem hiding this comment.
Assuming the allocation is done already in the opened file then we could reuse that memory instead of allocating more.
vgvassilev
reviewed
Aug 5, 2025
There was a problem hiding this comment.
Looks like all of the Cling/ROOT-related bits are in place. Can we work on adding a set of tests?
Perhaps @alexander-penev could also take a look as he was one of the originators of this work.
Comment on lines
125
to
130
| while (it != end) { | ||
| const auto &lib = it->second; | ||
| if (lib->getState() == state && lib->getKind() == kind) | ||
| break; | ||
| ++it; | ||
| } |
There was a problem hiding this comment.
Probably could be written as a for loop.
There was a problem hiding this comment.
Like this :
for (; it != end; ++it)
if (it->second->getState() == state && it->second->getKind() == kind)
break;
Comment on lines
119
to
121
| for (const auto &sym : symbols) { | ||
| filter.add(sym); | ||
| } |
There was a problem hiding this comment.
Suggested change
| for (const auto &sym : symbols) { | |
| filter.add(sym); | |
| } | |
| for (const auto &sym : symbols) | |
| filter.add(sym); |
4 tasks
|
This is being tested by the original downstream client and overall looks good. For this PR to move forward we will need a good amount of test cases. |
vgvassilev
reviewed
Aug 14, 2025
Comment on lines
119
to
121
| for (const auto &sym : Symbols) { | ||
| filter.add(sym); | ||
| } |
There was a problem hiding this comment.
Suggested change
| for (const auto &sym : Symbols) { | |
| filter.add(sym); | |
| } | |
| for (const auto &sym : Symbols) | |
| filter.add(sym); |
llvm/include/llvm/ExecutionEngine/Orc/TargetProcess/LibraryResolver.h
Outdated
Show resolved
Hide resolved
Comment on lines
146
to
154
| Iterator begin_, end_; | ||
| State state_; | ||
| PathType kind_; |
There was a problem hiding this comment.
I'd move this before the class definition. @lhames, are you aware if the training underscore is acceptable as a coding convention?
| } | ||
|
|
||
| private: | ||
| Iterator begin_, end_; |
There was a problem hiding this comment.
Suggested change
| Iterator begin_, end_; | |
| Iterator begin_; | |
| Iterator end_; |
Comment on lines
118
to
120
| if (it != m_readlinkCache.end()) { | ||
| return it->second; | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (it != m_readlinkCache.end()) { | |
| return it->second; | |
| } | |
| if (it != m_readlinkCache.end()) | |
| return it->second; |
Comment on lines
132
to
134
| if (it != m_lstatCache.end()) { | ||
| return it->second; | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (it != m_lstatCache.end()) { | |
| return it->second; | |
| } | |
| if (it != m_lstatCache.end()) | |
| return it->second; |
|
|
||
| private: | ||
| // mutable std::shared_mutex m_mutex; | ||
| std::shared_ptr<LibraryPathCache> m_cache; |
Comment on lines
174
to
176
| if (input.starts_with(ph)) { | ||
| return (Twine(value) + input.drop_front(ph.size())).str(); | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (input.starts_with(ph)) { | |
| return (Twine(value) + input.drop_front(ph.size())).str(); | |
| } | |
| if (input.starts_with(ph)) | |
| return (Twine(value) + input.drop_front(ph.size())).str(); |
| } | ||
|
|
||
| private: | ||
| StringMap<std::string> placeholders_; |
|
@vgvassilev I also think that everything from Cling/ROOT (as well as the subsequent rework) seems to be implemented. The improvements made are also reasonable. As @vgvassilev says, it is good to add some of the tests and some new ones. The recommended improvements are also appropriate. |
|
Thanks for the review! Folder structure: The test reads each YAML and creates a I’m figuring out how to make the YAML files portable across platforms. If I don’t find a simple way, then I need to create separate YAML files for each platform. |
|
Maybe some of the tests I wrote before can be found and adapted from here... https://github.com/compiler-research/CppInterOp/pull/308/files |
vgvassilev
reviewed
Aug 22, 2025
There was a problem hiding this comment.
Why not a flat structure, eg. Inputs/{A,B,C}.yaml?
There was a problem hiding this comment.
To test, check if dylib-resolver can find the dependency via @rpath and handle different search path directories.
There was a problem hiding this comment.
Ah, yes, good point. We should also test things like RUNPATH and similar code. Can you run codecoverage locally and list all of the untested branches? I really want good testing coverage for this feature.
There was a problem hiding this comment.
since we currently only support Mach-O.
So I added @rpath and @loader_path handling, plus extra path searching (with and without extensions like A.dylib or just A when searching for a library).
There was a problem hiding this comment.
We have a lot of tests that intent to capture real-world setups here: https://github.com/root-project/cling/tree/master/test/DynamicLibraryManager
Can you take a look and see what's missing from the current tests we have in this PR?
vgvassilev
approved these changes
Sep 6, 2025
Comment on lines
99
to
104
| /** | ||
| * @brief A read-only view of libraries filtered by state and kind. | ||
| * | ||
| * Lets you loop over only the libraries in a map that match a given State | ||
| * and PathType. | ||
| */ |
There was a problem hiding this comment.
Suggested change
| /** | |
| * @brief A read-only view of libraries filtered by state and kind. | |
| * | |
| * Lets you loop over only the libraries in a map that match a given State | |
| * and PathType. | |
| */ | |
| /** | |
| /// A read-only view of libraries filtered by state and kind. | |
| /// | |
| /// Lets you loop over only the libraries in a map that match a given State | |
| /// and PathType. |
|
One more task I need to complete is moving the code from Orc/TargetProcess to Orc/Shared, so that it can be used on the Root side as an API. Also, before merging, can we make sure the unit tests are running correctly? Right now, they only work for Mach-O binaries. I created a fat binary (x86_64 + arm64 + arm64e) YAML, where the x86_64 and arm64e parts were cross-compiled, and I generated the YAML file. However, I’ve only tested it on arm64, not on x86_64. |
Probably that’s a question for @lhames. I am fine with that change considering the fact that we will rework that part when the new orc runtime shapes up.
I can take this PR and run the test suite on Unix 64bit platform. Is that enough? |
The YAML file we have is a fat YAML containing slices for three architectures supported only on macOS. I’m not sure whether a Unix x86_64 system would support this format. |
Are there any other tests in the codebase that follow the same approach? |
lhames
reviewed
Oct 1, 2025
| static constexpr uint32_t bitsPerEntry = 64; | ||
|
|
||
| bool initialized = false; | ||
| uint32_t symbolCount_ = 0; |
There was a problem hiding this comment.
Trailing underscore for member names isn't idiomatic in LLVM. I think it'd make more sense to put that on the constructor parameter.
| @@ -0,0 +1,173 @@ | |||
| //===--- SymbolFilter.h - Utils for Symbol Filter ---*- C++ -*-===// | |||
There was a problem hiding this comment.
Header comment should be 80-cols wide.
Comment on lines
1
to
2
| //===- LibraryResolver.h - Automatic Dynamic Library Symbol Resolution -*- C++ | ||
| //-*-===// |
There was a problem hiding this comment.
Header comment should be 80 cols wide.
Comment on lines
15
to
16
| #ifndef LLVM_EXECUTIONENGINE_ORC_TARGETPROCESS_DYNAMICLOADER_H | ||
| #define LLVM_EXECUTIONENGINE_ORC_TARGETPROCESS_DYNAMICLOADER_H |
There was a problem hiding this comment.
Include guard name should match file name.
| std::string getFullPath() const { return filePath; } | ||
|
|
||
| bool setFilter(BloomFilter F) { | ||
| std::lock_guard<std::shared_mutex> lock(mutex); |
There was a problem hiding this comment.
Note: I think that std::scoped_lock is preferred to std::lock_guard these days. They're equivalent for your purposes though -- if you want to land as is we can replace later in-tree.
Comment on lines
36
to
39
| BloomFilter(uint32_t symbolCount, float falsePositiveRate, HashFunc hashFn) | ||
| : hashFunc(std::move(hashFn)) { | ||
| initialize(symbolCount, falsePositiveRate); | ||
| } |
There was a problem hiding this comment.
LLVM variable names should start with an upper-case letter (https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)
Comment on lines
1
to
2
| //===- LibraryScanner.h - Scanner for Shared Libraries -*- C++ | ||
| //-*-===// |
There was a problem hiding this comment.
Include comment should be 80-cols.
Comment on lines
1
to
2
| //===----- LibraryScanner.cpp - Provide Library Scaning implementation | ||
| //-----===// |
There was a problem hiding this comment.
Header comment should be 80-cols.
vgvassilev
reviewed
Oct 1, 2025
| } | ||
|
|
||
| private: | ||
| Iterator begin_; |
There was a problem hiding this comment.
Can we fix convention here: begin_ and friends?
| @@ -0,0 +1,475 @@ | |||
| //===- LibraryScanner.h - Scanner for Shared Libraries -*- C++ | |||
| //-*-===// | |||
There was a problem hiding this comment.
Can you fix these indentation/alignment?
Comment on lines
1
to
2
| //===----- LibraryScanner.cpp - Provide Library Scaning implementation | ||
| //-----===// |
|
Okay, I’ll fix all the comments. |
lhames
approved these changes
Oct 3, 2025
Comment on lines
208
to
223
| auto enumerateSymbolsIfNeeded = [&]() { | ||
| if (hasEnumerated) | ||
| return; | ||
|
|
||
| hasEnumerated = true; | ||
|
|
||
| LLVM_DEBUG(dbgs() << "Enumerating symbols in library: " << Lib.getFullPath() | ||
| << "\n";); | ||
| SymbolEnumerator::enumerateSymbols( | ||
| Lib.getFullPath(), | ||
| [&](StringRef sym) { | ||
| discoveredSymbols.insert(sym); | ||
| return EnumerateResult::Continue; | ||
| }, | ||
| Opts); | ||
| }; |
There was a problem hiding this comment.
It looks like this is only called once below -- could the body of this lambda just be moved down to the call-point?
Comment on lines
205
to
206
| StringSet<> discoveredSymbols; | ||
| bool hasEnumerated = false; |
There was a problem hiding this comment.
Symbol names should be upper-case, but these cosmetic issues can be fixed up in-tree at this point.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/15619 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/29226 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/203/builds/26516 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/205/builds/25305 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/204/builds/25328 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/32587 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/206/builds/7651 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/80/builds/16779 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/26763 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/26903 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/218/builds/281 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/17648 Here is the relevant piece of the build log for the reference |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Oct 17, 2025
…resolved symbols." (#163943) Reverts llvm/llvm-project#148410 Reverting this change due to a few buildbot/test failures. Will investigate and reapply once the issues are resolved.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/145/builds/10315 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/151/builds/7044 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/97/builds/8899 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/118/builds/8554 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/14754 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/76/builds/13215 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/130/builds/15738 Here is the relevant piece of the build log for the reference |
ronlieb
added a commit
to ROCm/llvm-project
that referenced
this pull request
Oct 24, 2025
…olved symbols. llvm#148410 (llvm#164551)" This reverts commit 4f53413.
dvbuka
pushed a commit
to dvbuka/llvm-project
that referenced
this pull request
Oct 27, 2025
…mbols. llvm#148410 (llvm#164551) This PR reapplies the changes previously introduced in llvm#148410. It introduces a redesigned and rebuilt Cling-based auto-loading workaround that enables scanning libraries and resolving unresolved symbols within those libraries.
dvbuka
pushed a commit
to dvbuka/llvm-project
that referenced
this pull request
Oct 27, 2025
…olved symbols. llvm#148410" (llvm#165069) Reverting llvm#164551 due to persistent build bot failure caused by a path difference issue.
SahilPatidar
added a commit
to SahilPatidar/llvm-project
that referenced
this pull request
Oct 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces a redesigned and rebuilt Cling-based auto-loading workaround, which enables scanning libraries and searching for unresolved symbols within the scanned libraries.