-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
Fix: Distinguish CFI Metadata Checks in MergeFunctions Pass #65963
Conversation
CC: @dexonsmith |
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 an improvement on https://reviews.llvm.org/D154119, in that it doesn't break function sorting.
However:
- It looks the performance is quadratic in the size of the function. There should be a more efficient way to do this. Especially since you have prior knowledge that the functions have sorted as equal.
- It adds a new "compare"-named and -behaving function which violates strict-weak order (since it will return both
A<B
andB<A
). It shouldn't over-promise that it can compare<
when it only decides "yes or no" for equivalency.
A few other comments inline.
Can you also pull in the other reviewers from the Phabricator patch? (And if none of them did significant work on MergeFunctions, perhaps look through Git history to find someone who did...) |
Apparently I need write access to be able to add reviewers, I can't add them on my own :( |
Before giving more low-level feedback, I'm a bit worried this is change is too broad.
Can you clarify whether functions should be excluded from merging if the CFI metadata referenced by Also, why are you restricting all metadata-as-value arguments, instead of just |
Again, thank you for taking the time to review this change!
If they have different identities but are structurally the same, as in the test I created:
Then CFI will create distinct checks for each of these and they are NOT equal and NOT valid to be merged. I am not the author so I can't explain the exact specifics, but IIUC when a function is internal to the module, CFI will use empty, distinct metadata as placeholders for the final checks.
@nikic had suggested on my first draft that "This doesn't looks like a problem specific to the type.test intrinsic. Other calls may have metadata operands as well." and so I made the change more broad to potentially fix other possible bad merges. |
You only answered the parenthesized question :). For the first question, I had a look at the documentation. In the examples, there are no (Just to double-check: is Perhaps we don't need to compare functions at all; instead, we can filter out functions as candidates for merging if they have (non-debug info) intrinsics that reference I suggest:
(Or, is there already a place early where functions are rejected as candidates for merging? If so, maybe this logic should be moved there...) I think you should use logic something like this (I looked at BasicBlock.h to find how to skip debug info):
|
Yes, because the typeid metadata are different.
I like this idea, will put up a new patch. |
401ea8b
to
779eb8e
Compare
It looks so much better now, thank you very much @dexonsmith! |
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.
Nice, LGTM! I have some minor feedback inline.
@@ -404,7 +430,7 @@ bool MergeFunctions::runOnModule(Module &M) { | |||
// If the hash value matches the previous value or the next one, we must | |||
// consider merging it. Otherwise it is dropped and never considered again. | |||
if ((I != S && std::prev(I)->first == I->first) || | |||
(std::next(I) != IE && std::next(I)->first == I->first) ) { | |||
(std::next(I) != IE && std::next(I)->first == I->first)) { |
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.
Nit: you should revert the whitespace fix-up on this line since it's not related to the patch anymore and makes it hard to see what actually changed. (Feel free to commit it separately / before / after as an NFC change if you want to clean it up (no need for review, IMO), as long as it's not squashed with this change.)
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.
The GitHub pull request interface requires "squashing", so if you don't have commit access to push this separately, it'd be better just to leave it out. Sorry for the churn!
779eb8e
to
6c80916
Compare
Thanks for teaching me so much in just 1 diff! |
6c80916
to
e037714
Compare
Thank you @nikic for catching that, growing pains of using Github over phabricator :) |
e037714
to
c13a50b
Compare
Looking good! I don't have merge access so once it gets to @nikic and @dexonsmith's standards I would appreciate a merge :) |
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.
A few more style nits; after that, LGTM again, but let's wait for @nikic to take another look (thanks for catching the missing isDistinct()
check...)
@@ -404,7 +430,7 @@ bool MergeFunctions::runOnModule(Module &M) { | |||
// If the hash value matches the previous value or the next one, we must | |||
// consider merging it. Otherwise it is dropped and never considered again. | |||
if ((I != S && std::prev(I)->first == I->first) || | |||
(std::next(I) != IE && std::next(I)->first == I->first) ) { | |||
(std::next(I) != IE && std::next(I)->first == I->first)) { |
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.
The GitHub pull request interface requires "squashing", so if you don't have commit access to push this separately, it'd be better just to leave it out. Sorry for the churn!
c13a50b
to
2d038ae
Compare
2d038ae
to
ca4fc4b
Compare
ca4fc4b
to
c80c820
Compare
c80c820
to
09ef2fd
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.
LGTM
@nikic @dexonsmith Again thank you for your feedback on this! I've yet to have write access so I'll ask one of you to merge this. :) |
Local branch amd-gfx 05e8466 Merged main:658b655191e9 into amd-gfx:d68837a57682 Remote branch main e06fc2b Fix: Distinguish CFI Metadata Checks in MergeFunctions Pass (llvm#65963)
This diff fixes an issue in the MergeFunctions pass where two different Control Flow Integrity (CFI) metadata checks were incorrectly considered identical. These merges would lead to runtime violations down the line as two separate objects contained a single destructor which itself contained checks for only one of the objects.
Here I update the comparison logic to take into account the metadata at llvm.type.test checks. Now, only truly identical checks will be considered for merging, thus preserving the integrity of each check.
Previous discussion: https://reviews.llvm.org/D154119