Page MenuHomePhabricator

[X86] Fix fentry handling in X86IndirectBranchTracking.cpp
ClosedPublic

Authored by joaomoreira on Oct 4 2021, 6:57 PM.

Details

Summary

When compiling with indirect branch tracking and fentry (-fcf-protection=branch -mfentry -pg) the X86IndirectBranchTrackingPass will attempt to place endbr in basic blocks, checking for Calls/IsCallReturnTwice. For calling the function IsCallReturnTwice(), the pass attempts to retrieve the first operand of the respective machine instruction. Since FENTRY_CALL is considered a call, and it does not have any argument, the condition inside the pass will attempt to call IsCallReturnTwice on the machine instruction, but since it does not have operands, it will lead into a crash.

Kudos to Alyssa Milburn for helping in the issue triage. The diff brings a test, but to reproduce the problem, follow the steps below.

echo "int main() {};" > repro.c
clang repro.c -fcf-protection=branch -mfentry -pg

Diff Detail

Event Timeline

joaomoreira created this revision.Oct 4 2021, 6:57 PM
joaomoreira requested review of this revision.Oct 4 2021, 6:57 PM
joaomoreira edited the summary of this revision. (Show Details)Oct 4 2021, 6:59 PM
This revision is now accepted and ready to land.Oct 4 2021, 7:13 PM
joaomoreira added a comment.EditedDec 6 2021, 2:13 PM

It seems this was not merged yet. Is there anything else needed?

It seems this was not merged yet. Is there anything else needed?

I probably just forgot you don't have commit access. @pengfei do you mind committing this?

This revision was automatically updated to reflect the committed changes.

It seems this was not merged yet. Is there anything else needed?

I probably just forgot you don't have commit access. @pengfei do you mind committing this?

Done~

DavidSpickett added a subscriber: DavidSpickett.EditedDec 7 2021, 2:06 AM

I added the x86 triple from the other existing test to this new one since on any other Arch it was choosing native arch and failing.

https://lab.llvm.org/buildbot/#/builders/179/builds/2046

It passed with that but feel free to fix what I did if you intended something else.

https://reviews.llvm.org/rGdb490ad3852c

I added the x86 triple from the other existing test to this new one since on any other Arch it was choosing native arch and failing.

https://lab.llvm.org/buildbot/#/builders/179/builds/2046

It passed with that but feel free to fix what I did if you intended something else.

https://reviews.llvm.org/rGdb490ad3852c

I believe it's the right fix. Thanks @DavidSpickett !

FWIIW, looks correct to me too. Tks @DavidSpickett