-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[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
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
4d0a735
[lld] Support order cstrings with -order_file_cstring
9d251c4
address comments
f39f908
use -order_file for cstring hashes
7c08999
fix typo
b2c4acb
fix typo2
af0f019
fix the cstr prefix from CSTR: to CSTR;
2ea09c3
Merge branch 'main' into SharonXSharon-lld-order
SharonXSharon 46ddc4a
fix format
2bc748e
fix test by adding # REQUIRES: aarch64
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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 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 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
Oops, something went wrong.
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.
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.
Right now we convert the string hash in the orderfile into an integer (but we don't use the result), then we also convert each hash in the object file to a string to look it up in the priority map. It would be more efficient to create a new priority map,
cstrPriorities
that maps integer hashes to priorities.Uh oh!
There was an error while loading. Please reload this page.
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.
@ellishg Do you mean just for
CSTR;
or do you mean all the other places using:
currently should be changed to something like;
?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.
Ideally we would use
;
in place of:
, but this is a breaking change. Maybe in the future we can support both and then eventually drop support for:
, but that would definitely require an RFC since it would break existing orderfiles.For now, I think we can use the
CSTR;
format.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.
/bikeshedding
I believe the only character disallowed in the symbolname would be the null character (at least for ELF). More practical constraints on symbol names arise from the programming language used to generate the executable. So in this case I believe most c-like languages that we care about disallow
:
and;
in identifier names so they are equal in this regard. Given the existing usage of ':' I'd prefer we continue using the same prefix delimiter.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.
Actually we still support Objective-C which commonly has
:
in the symbol names. I don't think we should land as is.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.
SGTM to proceed with a different delimiter.
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.
oh you're right, forgot about the objC symbol names. but on the other hand, all the existing
:
for CPU and files have already been causing this issue for ObjC symbols names...maybe right now the best trade-off is to use
CSTR;
?