[X86] Fix fentry handling in X86IndirectBranchTracking.cpp

[joaomoreira]

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

[craig.topper]

LGTM

[joaomoreira]

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

[craig.topper]

In D111108#3174689, @joaomoreira wrote:

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?

[pengfei]

In D111108#3174721, @craig.topper wrote:

In D111108#3174689, @joaomoreira wrote:

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]

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

[pengfei]

In D111108#3175787, @DavidSpickett wrote:

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 !

[joaomoreira]

FWIIW, looks correct to me too. Tks @DavidSpickett

llvm/lib/Target/X86/X86IndirectBranchTracking.cpp

#endif
 
   for (auto &MBB : MF) {
     // Find all basic blocks that their address was taken (for example
     // in the case of indirect jump) and add ENDBR instruction.
     if (MBB.hasAddressTaken())
       Changed |= addENDBR(MBB, MBB.begin());
 
     for (MachineBasicBlock::iterator I = MBB.begin(); I != MBB.end(); ++I) {
-      if (I->isCall() && IsCallReturnTwice(I->getOperand(0)))
+      if (I->isCall() && I->getNumOperands() > 0 &&
+          IsCallReturnTwice(I->getOperand(0))) {
         Changed |= addENDBR(MBB, std::next(I));
       }
+    }
 
     // Exception handle may indirectly jump to catch pad, So we should add
     // ENDBR before catch pad instructions. For SjLj exception model, it will
     // create a new BB(new landingpad) indirectly jump to the old landingpad.
     if (TM->Options.ExceptionModel == ExceptionHandling::SjLj) {
       for (MachineBasicBlock::iterator I = MBB.begin(); I != MBB.end(); ++I) {
         // New Landingpad BB without EHLabel.
         if (MBB.isEHPad()) {

llvm/test/CodeGen/X86/fentry-ibt.ll

+; RUN: llc %s -o - -verify-machineinstrs | FileCheck %s
+
+define void @test1() #0 {
+entry:
+  ret void
+
+; CHECK-LABEL: @test1
+; CHECK: endbr64
+; CHECK: callq __fentry__
+; CHECK-NOT: mcount
+; CHECK: retq
+}
+
+!llvm.module.flags = !{!0}
+
+attributes #0 = { "fentry-call"="true" }
+!0 = !{i32 4, !"cf-protection-branch", i32 1}