[IndirectThunks] Make generated MF structure as expected by all instruction selectors.

[kristof.beyls]

Summary

This also enables running the AArch64 SLSHardening pass with GlobalISel,
so add a test for that.

[arsenm]

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

2496 ↗

(On Diff #269244)

Why was this violated? Seems like it should be straightforward to fix? Can you add a pure IR test that just runs the IRTranslator?

[kristof.beyls]

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

2496 ↗

(On Diff #269244)

The violation comes from thunks produced by the ThunkInserter. They are naked functions, not sure if that could trigger this issue.
I'll look into if it's possible to have a pure IR test trigger this.

[kristof.beyls]

This new revision fixes the root cause of GlobalISel asserting on an IndirectThunk: the IndirectThunk creation code was producing a MachineFunction with a slightly different structure than what gets produced from a naked function implemented in C code and processed by clang.
This fixes this by letting the IndirectThunk generator produce a MachineFunction structure that is the same as what gets produced by clang for an empty naked function written in C.

[zbrid]

LGTM

Could you explain how not adding the MBB entry block is different from removing extra MBB later? Curious on the details about the differences here since the change makes sense functionally and I don't see any issues, but I can't understand why adding the extra MBB entry block at the point we create the machine function and then deleting extra blocks results in something different than the automatically created entry block. Is it some attributes or configuration in the basic block or something?

Also slightly unrelated question: Thanks for moving the IndirectThunk code to be of use to more Targets earlier in the commit stack! Along those lines, I was wondering if you had additional thoughts recently on adding retpolines for AArch64 since you are working near the x86 retpoline code and there was an llvm-dev thread from a team member of mine a week or two ago?

llvm/include/llvm/CodeGen/IndirectThunks.h

68

nit: update this comment to only mention MachineFunctions since MBBs don't need to be created.

[zbrid]

This comment was removed by zbrid.

[kristof.beyls]

In D81403#2097148, @zbrid wrote:

LGTM

Could you explain how not adding the MBB entry block is different from removing extra MBB later? Curious on the details about the differences here since the change makes sense functionally and I don't see any issues, but I can't understand why adding the extra MBB entry block at the point we create the machine function and then deleting extra blocks results in something different than the automatically created entry block. Is it some attributes or configuration in the basic block or something?

This was triggering the following assert in GlobalISel:
https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp#L2495

The X86 target does not use globalisel by default, but the AArch64 target does use it by default at -O0. So now that IndirectThunks is used by AArch64 also, that has become a blocker.
Globalisel runs between the place where the MBB entry block was added and the block was removed later.
Apart from this assert blocking the compilation flow, I wouldn't expect code generation to be different.

[zbrid]

Thanks for the explanation!

[kristof.beyls]

llvm/include/llvm/CodeGen/IndirectThunks.h

68

Thanks, I fixed that in a follow-up commit.

llvm/include/llvm/CodeGen/IndirectThunks.h

void ThunkInserter<Derived>::createThunkFunction(MachineModuleInfo &MMI,
   F->addAttributes(llvm::AttributeList::FunctionIndex, B);
 
   // Populate our function a bit so that we can verify.
   BasicBlock *Entry = BasicBlock::Create(Ctx, "entry", F);
   IRBuilder<> Builder(Entry);
 
   Builder.CreateRetVoid();
 
   // MachineFunctions/MachineBasicBlocks aren't created automatically for the
   // IR-level constructs we already made. Create them and insert them into the
   // module.
   MachineFunction &MF = MMI.getOrCreateMachineFunction(*F);
-  MachineBasicBlock *EntryMBB = MF.CreateMachineBasicBlock(Entry);
+  // A MachineBasicBlock must not be created for the Entry block; code
+  // generation from an empty naked function in C source code also does not
+  // generate one.  At least GlobalISel asserts if this invariant isn't
+  // respected.
 
-  // Insert EntryMBB into MF. It's not in the module until we do this.
-  MF.insert(MF.end(), EntryMBB);
   // Set MF properties. We never use vregs...
   MF.getProperties().set(MachineFunctionProperties::Property::NoVRegs);
 }
 
 template <typename Derived>
 bool ThunkInserter<Derived>::run(MachineModuleInfo &MMI, MachineFunction &MF) {
   // If MF is not a thunk, check to see if we need to insert a thunk.
   if (!MF.getName().startswith(getDerived().getThunkPrefix())) {

llvm/lib/Target/AArch64/AArch64SLSHardening.cpp

void SLSBLRThunkInserter::populateThunk(MachineFunction &MF) {
   assert(MF.getName().startswith(getThunkPrefix()));
   auto ThunkIt = llvm::find_if(
       SLSBLRThunks, [&MF](auto T) { return T.Name == MF.getName(); });
   assert(ThunkIt != std::end(SLSBLRThunks));
   Register ThunkReg = ThunkIt->Reg;
 
   const TargetInstrInfo *TII =
       MF.getSubtarget<AArch64Subtarget>().getInstrInfo();
-  // Grab the entry MBB and erase any other blocks. O0 codegen appears to
-  // generate two bbs for the entry block.
+  assert (MF.size() == 1);
   MachineBasicBlock *Entry = &MF.front();
   Entry->clear();
-  while (MF.size() > 1)
-    MF.erase(std::next(MF.begin()));
 
   //  These thunks need to consist of the following instructions:
   //  __llvm_slsblr_thunk_xN:
   //      BR xN
   //      barrierInsts
   Entry->addLiveIn(ThunkReg);
   BuildMI(Entry, DebugLoc(), TII->get(AArch64::BR)).addReg(ThunkReg);
   // Make sure the thunks do not make use of the SB extension in case there is

llvm/lib/Target/X86/X86IndirectThunks.cpp

struct LVIThunkInserter : ThunkInserter<LVIThunkInserter> {
   const char *getThunkPrefix() { return LVIThunkNamePrefix; }
   bool mayUseThunk(const MachineFunction &MF) {
     return MF.getSubtarget<X86Subtarget>().useLVIControlFlowIntegrity();
   }
   void insertThunks(MachineModuleInfo &MMI) {
     createThunkFunction(MMI, R11LVIThunkName);
   }
   void populateThunk(MachineFunction &MF) {
-    // Grab the entry MBB and erase any other blocks. O0 codegen appears to
-    // generate two bbs for the entry block.
+    assert (MF.size() == 1);
     MachineBasicBlock *Entry = &MF.front();
     Entry->clear();
-    while (MF.size() > 1)
-      MF.erase(std::next(MF.begin()));
 
     // This code mitigates LVI by replacing each indirect call/jump with a
     // direct call/jump to a thunk that looks like:
     // ```
     // lfence
     // jmpq *%r11
     // ```
     // This ensures that if the value in register %r11 was loaded from memory,
else if (MF.getName() == EDXRetpolineName)
       ThunkReg = X86::EDX;
     else if (MF.getName() == EDIRetpolineName)
       ThunkReg = X86::EDI;
     else
       llvm_unreachable("Invalid thunk name on x86-32!");
   }
 
   const TargetInstrInfo *TII = MF.getSubtarget<X86Subtarget>().getInstrInfo();
-  // Grab the entry MBB and erase any other blocks. O0 codegen appears to
-  // generate two bbs for the entry block.
+  assert (MF.size() == 1);
   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);

llvm/test/CodeGen/AArch64/speculation-hardening-sls.ll

 ; RUN: llc -mattr=harden-sls-retbr,harden-sls-blr -verify-machineinstrs -mtriple=aarch64-none-linux-gnu < %s | FileCheck %s --check-prefixes=CHECK,HARDEN,ISBDSB,ISBDSBDAGISEL
 ; RUN: llc -mattr=harden-sls-retbr,harden-sls-blr -mattr=+sb -verify-machineinstrs -mtriple=aarch64-none-linux-gnu < %s | FileCheck %s --check-prefixes=CHECK,HARDEN,SB,SBDAGISEL
 ; RUN: llc -verify-machineinstrs -mtriple=aarch64-none-linux-gnu < %s | FileCheck %s --check-prefixes=CHECK,NOHARDEN
-
+; RUN: llc -global-isel -global-isel-abort=0 -mattr=harden-sls-retbr,harden-sls-blr -verify-machineinstrs -mtriple=aarch64-none-linux-gnu < %s | FileCheck %s --check-prefixes=CHECK,HARDEN,ISBDSB
+; RUN: llc -global-isel -global-isel-abort=0 -mattr=harden-sls-retbr,harden-sls-blr -mattr=+sb -verify-machineinstrs -mtriple=aarch64-none-linux-gnu < %s | FileCheck %s --check-prefixes=CHECK,HARDEN,SB
 
 ; Function Attrs: norecurse nounwind readnone
 define dso_local i32 @double_return(i32 %a, i32 %b) local_unnamed_addr {
 entry:
   %cmp = icmp sgt i32 %a, 0
   br i1 %cmp, label %if.then, label %if.else
 
 if.then:                                          ; preds = %entry