[Spectre] Fix MIR verifier errors in retpoline thunks

[rnk]

Summary

The main challenge here is that X86InstrInfo::AnalyzeBranch doesn't
understand the way we're using a CALL instruction as a branch, so we
can't list the CallTarget MBB as a successor of the entry block. If we
don't list it as a successor, then the AsmPrinter doesn't print a label
for the MBB.

Fix the issue by inserting our own label at the beginning of the call
target block. We can rely on the AsmPrinter to always emit it, even
though the block appears to be unreachable, but address-taken.

Fixes PR38391.

[efriedma]

The main challenge here is that X86InstrInfo::AnalyzeBranch doesn't understand the way we're using a CALL instruction as a branch

This seems like something which could be fixed directly by adding a pseudo-instruction to represent the "call". For example, on Thumb1, we have tBfar. But maybe it's not worth fixing if the only place it would be used is inside retpoline thunks.

[rnk]

In D53653#1274634, @efriedma wrote:

The main challenge here is that X86InstrInfo::AnalyzeBranch doesn't understand the way we're using a CALL instruction as a branch

This seems like something which could be fixed directly by adding a pseudo-instruction to represent the "call". For example, on Thumb1, we have tBfar. But maybe it's not worth fixing if the only place it would be used is inside retpoline thunks.

I agree, it's not worth a pseudo. We're generating this MIR as late as possible, and honestly if we could generate MC instead, I would. The semantics of this are only explicable in raw assembly.

[rnk]

I'm going to land this so we can close out the bug and enable the MIR verifier for x86. Feel free to add comments post commit.

llvm/lib/Target/X86/X86RetpolineThunks.cpp

private:
   bool Is64Bit;
   const X86Subtarget *STI;
   const X86InstrInfo *TII;
 
   bool InsertedThunks;
 
   void createThunkFunction(Module &M, StringRef Name);
   void insertRegReturnAddrClobber(MachineBasicBlock &MBB, unsigned Reg);
-  void populateThunk(MachineFunction &MF, Optional<unsigned> Reg = None);
+  void populateThunk(MachineFunction &MF, unsigned Reg);
 };
 
 } // end anonymous namespace
 
 FunctionPass *llvm::createX86RetpolineThunksPass() {
   return new X86RetpolineThunks();
 }
 
void X86RetpolineThunks::insertRegReturnAddrClobber(MachineBasicBlock &MBB,
                                                     unsigned Reg) {
   const unsigned MovOpc = Is64Bit ? X86::MOV64mr : X86::MOV32mr;
   const unsigned SPReg = Is64Bit ? X86::RSP : X86::ESP;
   addRegOffset(BuildMI(&MBB, DebugLoc(), TII->get(MovOpc)), SPReg, false, 0)
       .addReg(Reg);
 }
 
 void X86RetpolineThunks::populateThunk(MachineFunction &MF,
-                                       Optional<unsigned> Reg) {
+                                       unsigned Reg) {
   // Set MF properties. We never use vregs...
   MF.getProperties().set(MachineFunctionProperties::Property::NoVRegs);
 
+  // Grab the entry MBB and erase any other blocks. O0 codegen appears to
+  // generate two bbs for the entry block.
   MachineBasicBlock *Entry = &MF.front();
   Entry->clear();
+  while (MF.size() > 1)
+    MF.erase(std::next(MF.begin()));
 
   MachineBasicBlock *CaptureSpec = MF.CreateMachineBasicBlock(Entry->getBasicBlock());
   MachineBasicBlock *CallTarget = MF.CreateMachineBasicBlock(Entry->getBasicBlock());
+  MCSymbol *TargetSym = MF.getContext().createTempSymbol();
   MF.push_back(CaptureSpec);
   MF.push_back(CallTarget);
 
   const unsigned CallOpc = Is64Bit ? X86::CALL64pcrel32 : X86::CALLpcrel32;
   const unsigned RetOpc = Is64Bit ? X86::RETQ : X86::RETL;
 
-  BuildMI(Entry, DebugLoc(), TII->get(CallOpc)).addMBB(CallTarget);
-  Entry->addSuccessor(CallTarget);
+  Entry->addLiveIn(Reg);
+  BuildMI(Entry, DebugLoc(), TII->get(CallOpc)).addSym(TargetSym);
+
+  // The MIR verifier thinks that the CALL in the entry block will fall through
+  // to CaptureSpec, so mark it as the successor. Technically, CaptureTarget is
+  // the successor, but the MIR verifier doesn't know how to cope with that.
   Entry->addSuccessor(CaptureSpec);
-  CallTarget->setHasAddressTaken();
 
   // In the capture loop for speculation, we want to stop the processor from
   // speculating as fast as possible. On Intel processors, the PAUSE instruction
   // will block speculation without consuming any execution resources. On AMD
   // processors, the PAUSE instruction is (essentially) a nop, so we also use an
   // LFENCE instruction which they have advised will stop speculation as well
   // with minimal resource utilization. We still end the capture with a jump to
   // form an infinite loop to fully guarantee that no matter what implementation
   // of the x86 ISA, speculating this code path never escapes.
   BuildMI(CaptureSpec, DebugLoc(), TII->get(X86::PAUSE));
   BuildMI(CaptureSpec, DebugLoc(), TII->get(X86::LFENCE));
   BuildMI(CaptureSpec, DebugLoc(), TII->get(X86::JMP_1)).addMBB(CaptureSpec);
   CaptureSpec->setHasAddressTaken();
   CaptureSpec->addSuccessor(CaptureSpec);
 
+  CallTarget->addLiveIn(Reg);
+  CallTarget->setHasAddressTaken();
   CallTarget->setAlignment(4);
-  insertRegReturnAddrClobber(*CallTarget, *Reg);
+  insertRegReturnAddrClobber(*CallTarget, Reg);
+  CallTarget->back().setPreInstrSymbol(MF, TargetSym);
   BuildMI(CallTarget, DebugLoc(), TII->get(RetOpc));
 }

llvm/test/CodeGen/X86/retpoline.ll

-; RUN: llc -mtriple=x86_64-unknown < %s | FileCheck %s --implicit-check-not="jmp.*\*" --implicit-check-not="call.*\*" --check-prefix=X64
-; RUN: llc -mtriple=x86_64-unknown -O0 < %s | FileCheck %s --implicit-check-not="jmp.*\*" --implicit-check-not="call.*\*" --check-prefix=X64FAST
+; RUN: llc -verify-machineinstrs -mtriple=x86_64-unknown < %s | FileCheck %s --implicit-check-not="jmp.*\*" --implicit-check-not="call.*\*" --check-prefix=X64
+; RUN: llc -verify-machineinstrs -mtriple=x86_64-unknown -O0 < %s | FileCheck %s --implicit-check-not="jmp.*\*" --implicit-check-not="call.*\*" --check-prefix=X64FAST
 
-; RUN: llc -mtriple=i686-unknown < %s | FileCheck %s --implicit-check-not="jmp.*\*" --implicit-check-not="call.*\*" --check-prefix=X86
-; RUN: llc -mtriple=i686-unknown -O0 < %s | FileCheck %s --implicit-check-not="jmp.*\*" --implicit-check-not="call.*\*" --check-prefix=X86FAST
+; RUN: llc -verify-machineinstrs -mtriple=i686-unknown < %s | FileCheck %s --implicit-check-not="jmp.*\*" --implicit-check-not="call.*\*" --check-prefix=X86
+; RUN: llc -verify-machineinstrs -mtriple=i686-unknown -O0 < %s | FileCheck %s --implicit-check-not="jmp.*\*" --implicit-check-not="call.*\*" --check-prefix=X86FAST
 
 declare void @bar(i32)
 
 ; Test a simple indirect call and tail call.
 define void @icall_reg(void (i32)* %fp, i32 %x) #0 {
 entry:
   tail call void @bar(i32 %x)
   tail call void %fp(i32 %x)

 ; X64-NEXT:          callq   [[CALL_TARGET:.*]]
 ; X64-NEXT:  [[CAPTURE_SPEC:.*]]:                    # Block address taken
 ; X64-NEXT:                                          # %entry
 ; X64-NEXT:                                          # =>This Inner Loop Header: Depth=1
 ; X64-NEXT:          pause
 ; X64-NEXT:          lfence
 ; X64-NEXT:          jmp     [[CAPTURE_SPEC]]
 ; X64-NEXT:          .p2align        4, 0x90
-; X64-NEXT:  [[CALL_TARGET]]:                        # Block address taken
+; X64-NEXT:  {{.*}}                                  # Block address taken
 ; X64-NEXT:                                          # %entry
+; X64-NEXT:  [[CALL_TARGET]]:
 ; X64-NEXT:          movq    %r11, (%rsp)
 ; X64-NEXT:          retq
 ;
 ; X86-LABEL:         .section        .text.__llvm_retpoline_eax,{{.*}},__llvm_retpoline_eax,comdat
 ; X86-NEXT:          .hidden __llvm_retpoline_eax
 ; X86-NEXT:          .weak   __llvm_retpoline_eax
 ; X86:       __llvm_retpoline_eax:
 ; X86-NEXT:  # {{.*}}                                # %entry
 ; X86-NEXT:          calll   [[CALL_TARGET:.*]]
 ; X86-NEXT:  [[CAPTURE_SPEC:.*]]:                    # Block address taken
 ; X86-NEXT:                                          # %entry
 ; X86-NEXT:                                          # =>This Inner Loop Header: Depth=1
 ; X86-NEXT:          pause
 ; X86-NEXT:          lfence
 ; X86-NEXT:          jmp     [[CAPTURE_SPEC]]
 ; X86-NEXT:          .p2align        4, 0x90
-; X86-NEXT:  [[CALL_TARGET]]:                        # Block address taken
+; X86-NEXT:  {{.*}}                                  # Block address taken
 ; X86-NEXT:                                          # %entry
+; X86-NEXT:  [[CALL_TARGET]]:
 ; X86-NEXT:          movl    %eax, (%esp)
 ; X86-NEXT:          retl
 ;
 ; X86-LABEL:         .section        .text.__llvm_retpoline_ecx,{{.*}},__llvm_retpoline_ecx,comdat
 ; X86-NEXT:          .hidden __llvm_retpoline_ecx
 ; X86-NEXT:          .weak   __llvm_retpoline_ecx
 ; X86:       __llvm_retpoline_ecx:
 ; X86-NEXT:  # {{.*}}                                # %entry
 ; X86-NEXT:          calll   [[CALL_TARGET:.*]]
 ; X86-NEXT:  [[CAPTURE_SPEC:.*]]:                    # Block address taken
 ; X86-NEXT:                                          # %entry
 ; X86-NEXT:                                          # =>This Inner Loop Header: Depth=1
 ; X86-NEXT:          pause
 ; X86-NEXT:          lfence
 ; X86-NEXT:          jmp     [[CAPTURE_SPEC]]
 ; X86-NEXT:          .p2align        4, 0x90
-; X86-NEXT:  [[CALL_TARGET]]:                        # Block address taken
+; X86-NEXT:  {{.*}}                                  # Block address taken
 ; X86-NEXT:                                          # %entry
+; X86-NEXT:  [[CALL_TARGET]]:
 ; X86-NEXT:          movl    %ecx, (%esp)
 ; X86-NEXT:          retl
 ;
 ; X86-LABEL:         .section        .text.__llvm_retpoline_edx,{{.*}},__llvm_retpoline_edx,comdat
 ; X86-NEXT:          .hidden __llvm_retpoline_edx
 ; X86-NEXT:          .weak   __llvm_retpoline_edx
 ; X86:       __llvm_retpoline_edx:
 ; X86-NEXT:  # {{.*}}                                # %entry
 ; X86-NEXT:          calll   [[CALL_TARGET:.*]]
 ; X86-NEXT:  [[CAPTURE_SPEC:.*]]:                    # Block address taken
 ; X86-NEXT:                                          # %entry
 ; X86-NEXT:                                          # =>This Inner Loop Header: Depth=1
 ; X86-NEXT:          pause
 ; X86-NEXT:          lfence
 ; X86-NEXT:          jmp     [[CAPTURE_SPEC]]
 ; X86-NEXT:          .p2align        4, 0x90
-; X86-NEXT:  [[CALL_TARGET]]:                        # Block address taken
+; X86-NEXT:  {{.*}}                                  # Block address taken
 ; X86-NEXT:                                          # %entry
+; X86-NEXT:  [[CALL_TARGET]]:
 ; X86-NEXT:          movl    %edx, (%esp)
 ; X86-NEXT:          retl
 ;
 ; X86-LABEL:         .section        .text.__llvm_retpoline_edi,{{.*}},__llvm_retpoline_edi,comdat
 ; X86-NEXT:          .hidden __llvm_retpoline_edi
 ; X86-NEXT:          .weak   __llvm_retpoline_edi
 ; X86:       __llvm_retpoline_edi:
 ; X86-NEXT:  # {{.*}}                                # %entry
 ; X86-NEXT:          calll   [[CALL_TARGET:.*]]
 ; X86-NEXT:  [[CAPTURE_SPEC:.*]]:                    # Block address taken
 ; X86-NEXT:                                          # %entry
 ; X86-NEXT:                                          # =>This Inner Loop Header: Depth=1
 ; X86-NEXT:          pause
 ; X86-NEXT:          lfence
 ; X86-NEXT:          jmp     [[CAPTURE_SPEC]]
 ; X86-NEXT:          .p2align        4, 0x90
-; X86-NEXT:  [[CALL_TARGET]]:                        # Block address taken
+; X86-NEXT:  {{.*}}                                  # Block address taken
 ; X86-NEXT:                                          # %entry
+; X86-NEXT:  [[CALL_TARGET]]:
 ; X86-NEXT:          movl    %edi, (%esp)
 ; X86-NEXT:          retl
 
 
 attributes #0 = { "target-features"="+retpoline-indirect-calls" }
 attributes #1 = { "target-features"="+retpoline-indirect-calls,+retpoline-indirect-branches" }
 attributes #2 = { nonlazybind }