[ORC] Refactor member-loading in StaticLibraryDefinitionGenerator. #141546
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
This refactor was motivated by two issues identified in out-of-tree builds:
1. Some implementations of the VisitMembersFunction utility (often used to
implement -all_load semantics, e.g. in the in-tree loadAllObjectFileMembers
method) were assuming that buffers for archive members were null-terminated,
which they are not in general. This was triggering occasional assertions.
2. Archives may include multiple members with the same file name, e.g. if
constructed by appending files with the same name:
% llvm-ar crs libfoo.a foo.o
% llvm-ar q libfoo.a foo.o
% llvm-ar t libfoo.a
foo.o
foo.o
While confusing, these members may be safe to link (provided that they're
individually valid and don't define duplicate symbols). In ORC however the
archive member name may be used to construct an ORC initializer symbol, which
must also be unique. In that case the duplicate member names lead to a
duplicate definition error even if the members define unrelated symbols.
In addition to these bugs, StaticLibraryDefinitionGenerator had grown a
collection of all member buffers (ObjectFilesMap), a BumpPtrAllocator that was
redundantly storing synthesized archive member names (these are copied into the
MemoryBuffers created for each Object, but were never freed in the allocator),
and a set of COFF-specific import files.
To fix the bugs above and simplify StaticLibraryDefinitionGenerator this patch
makes the following changes:
1. StaticLibraryDefinitionGenerator::VisitMembersFunction is generalized to
take a reference to the containing archive, and the index of the member
within the archive. It now returns an Expected<bool> indicating whether
the member visited should be treated as loadable, not loadable, or as
invalidating the entire archive.
2. A static StaticLibraryDefinitionGenerator::createMemberBuffer method is
added which creates MemoryBuffers with unique names of the form
`<archive-name>[<index>](<member-name>)`. This defers construction of
member names until they're loaded, allowing the BumpPtrAllocator (with its
redundant name storage) to be removed.
3. The ObjectFilesMap (symbol name -> memory-buffer-ref) is replaced with a
SymbolToMemberIndexMap (symbol name -> index) which should be smaller and
faster to construct.
4. The 'loadability' result from VisitMemberFunctions is now taken into
consideration when building the SymbolToMemberIndexMap so that members that
have already been loaded / filtered out can be skipped, and do not take up
any ongoing space.
5. The COFF ImportedDynamicLibraries member is moved out into the
COFFImportFileScanner utility, which can be used as a VisitMemberFunction.
This fixes the bugs described above; and should lower memory consumption
slightly, especially for archives with many files and / or symbol where most
files are eventually loaded.
Should fix the testcases on Windows.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/64/builds/3857 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/161/builds/6215 Here is the relevant piece of the build log for the reference |
|
@lhames this seems to be breaking the Windows on Arm bots. Could you please check? |
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
…vm#141546) This refactor was motivated by two bugs identified in out-of-tree builds: 1. Some implementations of the VisitMembersFunction type (often used to implement special loading semantics, e.g. -all_load or -ObjC) were assuming that buffers for archive members were null-terminated, which they are not in general. This was triggering occasional assertions. 2. Archives may include multiple members with the same file name, e.g. when constructed by appending files with the same name: % llvm-ar crs libfoo.a foo.o % llvm-ar q libfoo.a foo.o % llvm-ar t libfoo.a foo.o foo.o While confusing, these members may be safe to link (provided that they're individually valid and don't define duplicate symbols). In ORC however, the archive member name may be used to construct an ORC initializer symbol, which must also be unique. In that case the duplicate member names lead to a duplicate definition error even if the members define unrelated symbols. In addition to these bugs, StaticLibraryDefinitionGenerator had grown a collection of all member buffers (ObjectFilesMap), a BumpPtrAllocator that was redundantly storing synthesized archive member names (these are copied into the MemoryBuffers created for each Object, but were never freed in the allocator), and a set of COFF-specific import files. To fix the bugs above and simplify StaticLibraryDefinitionGenerator this patch makes the following changes: 1. StaticLibraryDefinitionGenerator::VisitMembersFunction is generalized to take a reference to the containing archive, and the index of the member within the archive. It now returns an Expected<bool> indicating whether the member visited should be treated as loadable, not loadable, or as invalidating the entire archive. 2. A static StaticLibraryDefinitionGenerator::createMemberBuffer method is added which creates MemoryBuffers with unique names of the form `<archive-name>[<index>](<member-name>)`. This defers construction of member names until they're loaded, allowing the BumpPtrAllocator (with its redundant name storage) to be removed. 3. The ObjectFilesMap (symbol name -> memory-buffer-ref) is replaced with a SymbolToMemberIndexMap (symbol name -> index) which should be smaller and faster to construct. 4. The 'loadability' result from VisitMemberFunctions is now taken into consideration when building the SymbolToMemberIndexMap so that members that have already been loaded / filtered out can be skipped, and do not take up any ongoing space. 5. The COFF ImportedDynamicLibraries member is moved out into the COFFImportFileScanner utility, which can be used as a VisitMemberFunction. This fixes the bugs described above; and should lower memory consumption slightly, especially for archives with many files and / or symbol where most files are eventually loaded.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 refactor was motivated by two issues identified in out-of-tree builds:
Some implementations of the VisitMembersFunction utility (often used to implement -all_load semantics, e.g. in the in-tree loadAllObjectFileMembers method) were assuming that buffers for archive members were null-terminated, which they are not in general. This was triggering occasional assertions.
Archives may include multiple members with the same file name, e.g. if constructed by appending files with the same name: % llvm-ar crs libfoo.a foo.o % llvm-ar q libfoo.a foo.o % llvm-ar t libfoo.a foo.o foo.o
While confusing, these members may be safe to link (provided that they're
individually valid and don't define duplicate symbols). In ORC however the
archive member name may be used to construct an ORC initializer symbol, which
must also be unique. In that case the duplicate member names lead to a
duplicate definition error even if the members define unrelated symbols.
In addition to these bugs, StaticLibraryDefinitionGenerator had grown a collection of all member buffers (ObjectFilesMap), a BumpPtrAllocator that was redundantly storing synthesized archive member names (these are copied into the MemoryBuffers created for each Object, but were never freed in the allocator), and a set of COFF-specific import files.
To fix the bugs above and simplify StaticLibraryDefinitionGenerator this patch makes the following changes:
<archive-name>[<index>](<member-name>). This defers construction of member names until they're loaded, allowing the BumpPtrAllocator (with its redundant name storage) to be removed.This fixes the bugs described above; and should lower memory consumption slightly, especially for archives with many files and / or symbol where most files are eventually loaded.