Fix: Distinguish CFI Metadata Checks in MergeFunctions Pass

[oskarwirga]

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

[oskarwirga]

CC: @dexonsmith

[dexonsmith]

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 and B<A). It shouldn't over-promise that it can compare < when it only decides "yes or no" for equivalency.

A few other comments inline.

[dexonsmith]

CC: @dexonsmith

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...)

[oskarwirga]

CC: @dexonsmith

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 :(

[dexonsmith]

Before giving more low-level feedback, I'm a bit worried this is change is too broad.

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.

Can you clarify whether functions should be excluded from merging if the CFI metadata referenced by llvm.type.test matches exactly, or only if they are different? (What if they have different identities (distinct) but are structurally equivalent?)

Also, why are you restricting all metadata-as-value arguments, instead of just llvm.type.test intrinsics?

[oskarwirga]

Again, thank you for taking the time to review this change!

Can you clarify whether functions should be excluded from merging if the CFI metadata referenced by llvm.type.test matches exactly, or only if they are different? (What if they have different identities (distinct) but are structurally equivalent?)

If they have different identities but are structurally the same, as in the test I created:

!10 = !{i64 16, !11}
!11 = distinct !{}
!21 = !{i64 16, !22}
!22 = distinct !{}

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.

Also, why are you restricting all metadata-as-value arguments, instead of just llvm.type.test intrinsics?

@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.

[dexonsmith]

Can you clarify whether functions should be excluded from merging if the CFI metadata referenced by llvm.type.test matches exactly, or only if they are different? (What if they have different identities (distinct) but are structurally equivalent?)

If they have different identities but are structurally the same, as in the test I created:

!10 = !{i64 16, !11}
!11 = distinct !{}
!21 = !{i64 16, !22}
!22 = distinct !{}

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.

You only answered the parenthesized question :).

For the first question, I had a look at the documentation. In the examples, there are no distinct nodes. Looks like global types that come up like this do not use distinct, and are candidates for merging. distinct must come up for function-local types. This is likely why merging isn't valid.

(Just to double-check: is llvm.type.test being (correctly) rejected for function merging for global types (not distinct) when it's not structurally equal? (I.e., pointing at two different global types.) I'm assuming "yes" below.)

---

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 distinct metadata. You're never comparing metadata. You're just checking it.

I suggest:

• Move the helper function to MergeFunctions.cpp and make it a top-level static function (local to the file)
• Rename it to hasDistinctMetadataIntrinsic
• Use it as a filter to skip over / reject a function

(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):

lang=c++
for (const BasicBlock &BB : ...) {
  for (const Instruction &I : BB.instructionsWithoutDebug()) {
    if (!isa<IntrinsicInst>(I))
      continue; // Only intrinsics can reference metadata.

    // Iterate through operands, and return true if you find a distinct metadata operand.
  }
}
return false;

[oskarwirga]

(Just to double-check: is llvm.type.test being (correctly) rejected for function merging for global types (not distinct) when it's not structurally equal? (I.e., pointing at two different global types.) I'm assuming "yes" below.)

Yes, because the typeid metadata are different.

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 distinct metadata. You're never comparing metadata. You're just checking it.

I like this idea, will put up a new patch.

[oskarwirga]

It looks so much better now, thank you very much @dexonsmith!

[dexonsmith]

Nice, LGTM! I have some minor feedback inline.

[dexonsmith]

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.)

[dexonsmith]

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!

[oskarwirga]

Thanks for teaching me so much in just 1 diff!

[oskarwirga]

Thank you @nikic for catching that, growing pains of using Github over phabricator :)

[oskarwirga]

Looking good! I don't have merge access so once it gets to @nikic and @dexonsmith's standards I would appreciate a merge :)

[dexonsmith]

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...)

[dexonsmith]

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!

[nikic]

LGTM

[oskarwirga]

@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. :)