-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[lld][macho] Support order cstrings with -order_file #140307
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
Changes from 1 commit
4d0a735
9d251c4
f39f908
7c08999
b2c4acb
af0f019
2ea09c3
46ddc4a
2bc748e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -388,3 +388,74 @@ macho::PriorityBuilder::buildInputSectionPriorities() { | |||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| return sectionPriorities; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| void macho::PriorityBuilder::parseOrderFileCString(StringRef path) { | ||||||||||||||||||||||||||||||||||||
| std::optional<MemoryBufferRef> buffer = readFile(path); | ||||||||||||||||||||||||||||||||||||
| if (!buffer) { | ||||||||||||||||||||||||||||||||||||
| error("Could not read cstring order file at " + path); | ||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| MemoryBufferRef mbref = *buffer; | ||||||||||||||||||||||||||||||||||||
| int priority = std::numeric_limits<int>::min(); | ||||||||||||||||||||||||||||||||||||
| for (StringRef line : args::getLines(mbref)) { | ||||||||||||||||||||||||||||||||||||
| if (line.empty()) | ||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||
| uint32_t hash = 0; | ||||||||||||||||||||||||||||||||||||
| if (!to_integer(line, hash)) | ||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
| uint32_t hash = xxh3_64bits(str) & 0x7fffffff; |
Outdated
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.
This performs the lookup twice, which could be a noticeable performance hit. Instead:
auto [it, inserted] = cStringPriorities.try_emplace(hash, 0);
// If actually inserted, update with the new priority
if (inserted)
it->second = ++priority;
Outdated
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.
@alx32 if we want to use try_emplace() we can do this change if we are ok with setting the priority first and then incrementing
| auto it = cStringPriorities.find(hash); | |
| if (it == cStringPriorities.end()) | |
| cStringPriorities[hash] = ++priority; | |
| auto [it, wasInserted] = cStringPriorities.try_emplace(hash, priority); | |
| if (wasInserted) | |
| ++priority; |
ellishg marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
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.
If cStringOrderFilePath is empty, then cStringPriorities will be empty. So I think you can just remove this code entirely.
| std::vector<StringPiecePair> orderedStringPieces; | |
| if (config->cStringOrderFilePath.empty()) { | |
| for (CStringInputSection *isec : inputs) { | |
| for (const auto &[stringPieceIdx, piece] : | |
| llvm::enumerate(isec->pieces)) { | |
| if (!piece.live) | |
| continue; | |
| orderedStringPieces.emplace_back(isec, stringPieceIdx); | |
| } | |
| } | |
| return orderedStringPieces; | |
| } |
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.
yes, i was thinking adding this early return can avoid the unnecessary checks of cStringPriorities when cStringOrderFilePath is empty, but perhaps those operations aren't really expensive?
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.
DenseMap::find() returns pretty quickly if it's empty. I don't think it would impact performance
llvm-project/llvm/include/llvm/ADT/DenseMap.h
Lines 650 to 654 in 286ab11
| template <typename LookupKeyT> BucketT *doFind(const LookupKeyT &Val) { | |
| BucketT *BucketsPtr = getBuckets(); | |
| const unsigned NumBuckets = getNumBuckets(); | |
| if (NumBuckets == 0) | |
| return nullptr; |
Uh oh!
There was an error while loading. Please reload this page.