[BOLT][BTI] Add MCPlusBuilder::addBTItoBBStart

[bgergely0]

This function contains most of the logic for BTI:

• it takes the BasicBlock and the instruction used to jump to it.
• then it checks if the first non-pseudo instruction is a sufficient
  landing pad for the used call.
• if not, it generates the correct BTI instruction.

Also introduce the isBTIVariantCoveringCall helper to simplify the logic.

[bgergely0]

• [BOLT][BTI] Add MCPlusBuilder::addBTItoBBStart #167329 👈 (View in Graphite)
• [BOLT][BTI] Add MCPlusBuilder::updateBTIVariant #167308
• [BOLT][BTI] Add MCPlusBuilder::isBTILandingPad #167306
• [BOLT][BTI] Add MCPlusBuilder::createBTI #167305
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Gergely Bálint (bgergely0)

Changes

This function contains most of the logic for BTI:

• it takes the BasicBlock and the instruction used to jump to it.
• then it checks if the first non-pseudo instruction is a sufficient
  landing pad for the used call.
• if not, it generates the correct BTI instruction.

Also introduce the isBTIVariantCoveringCall helper to simplify the logic.

---

Full diff: https://github.com/llvm/llvm-project/pull/167329.diff

3 Files Affected:

• (modified) bolt/include/bolt/Core/MCPlusBuilder.h (+13)
• (modified) bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp (+75)
• (modified) bolt/unittests/Core/MCPlusBuilder.cpp (+105)

diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index 660c1c64b06cf..4eaf444c320bf 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -1890,6 +1890,19 @@ class MCPlusBuilder {
     llvm_unreachable("not implemented");
   }
 
+  /// Checks if the indirect call / jump is accepted by the landing pad at the
+  /// start of the target BasicBlock.
+  virtual bool isBTIVariantCoveringCall(MCInst &Call, MCInst &Pad) const {
+    llvm_unreachable("not implemented");
+    return false;
+  }
+
+  /// Adds a BTI landing pad to the start of the BB, that matches the indirect
+  /// call/jump inst.
+  virtual void addBTItoBBStart(BinaryBasicBlock &BB, MCInst &Call) const {
+    llvm_unreachable("not implemented");
+  }
+
   /// Store \p Target absolute address to \p RegName
   virtual InstructionListType materializeAddress(const MCSymbol *Target,
                                                  MCContext *Ctx,
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index cb0a9cc0c12db..a5c88e113f726 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -2738,6 +2738,81 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
     Inst.addOperand(MCOperand::createImm(HintNum));
   }
 
+  bool isBTIVariantCoveringCall(MCInst &Call, MCInst &Pad) const override {
+    assert((isIndirectCall(Call) || isIndirectBranch(Call)) &&
+           "Not an indirect call or branch.");
+
+    // A BLR can be accepted by a BTI c.
+    if (isIndirectCall(Call))
+      return isBTILandingPad(Pad, true, false) ||
+             isBTILandingPad(Pad, true, true);
+
+    // A BR can be accepted by a BTI j or BTI c (and BTI jc) IF the operand is
+    // x16 or x17. If the operand is not x16 or x17, it can be accepted by a BTI
+    // j or BTI jc (and not BTI c).
+    if (isIndirectBranch(Call)) {
+      assert(Call.getNumOperands() == 1 &&
+             "Indirect branch needs to have 1 operand.");
+      assert(Call.getOperand(0).isReg() &&
+             "Indirect branch does not have a register operand.");
+      MCPhysReg Reg = Call.getOperand(0).getReg();
+      if (Reg == AArch64::X16 || Reg == AArch64::X17)
+        return isBTILandingPad(Pad, true, false) ||
+               isBTILandingPad(Pad, false, true) ||
+               isBTILandingPad(Pad, true, true);
+      return isBTILandingPad(Pad, false, true) ||
+             isBTILandingPad(Pad, true, true);
+    }
+    return false;
+  }
+
+  void addBTItoBBStart(BinaryBasicBlock &BB, MCInst &Call) const override {
+    auto II = BB.getFirstNonPseudo();
+    if (II != BB.end()) {
+      if (isBTIVariantCoveringCall(Call, *II))
+        return;
+      // A BLR can be accepted by a BTI c.
+      if (isIndirectCall(Call)) {
+        // if we have a BTI j at the start, extend it to a BTI jc,
+        // otherwise insert a new BTI c.
+        if (isBTILandingPad(*II, false, true)) {
+          updateBTIVariant(*II, true, true);
+        } else {
+          MCInst BTIInst;
+          createBTI(BTIInst, true, false);
+          BB.insertInstruction(II, BTIInst);
+        }
+      }
+
+      // A BR can be accepted by a BTI j or BTI c (and BTI jc) IF the operand is
+      // x16 or x17. If the operand is not x16 or x17, it can be accepted by a
+      // BTI j or BTI jc (and not BTI c).
+      if (isIndirectBranch(Call)) {
+        assert(Call.getNumOperands() == 1 &&
+               "Indirect branch needs to have 1 operand.");
+        assert(Call.getOperand(0).isReg() &&
+               "Indirect branch does not have a register operand.");
+        MCPhysReg Reg = Call.getOperand(0).getReg();
+        if (Reg == AArch64::X16 || Reg == AArch64::X17) {
+          // Add a new BTI c
+          MCInst BTIInst;
+          createBTI(BTIInst, true, false);
+          BB.insertInstruction(II, BTIInst);
+        } else {
+          // If BB starts with a BTI c, extend it to BTI jc,
+          // otherwise insert a new BTI j.
+          if (isBTILandingPad(*II, true, false)) {
+            updateBTIVariant(*II, true, true);
+          } else {
+            MCInst BTIInst;
+            createBTI(BTIInst, false, true);
+            BB.insertInstruction(II, BTIInst);
+          }
+        }
+      }
+    }
+  }
+
   InstructionListType materializeAddress(const MCSymbol *Target, MCContext *Ctx,
                                          MCPhysReg RegName,
                                          int64_t Addend = 0) const override {
diff --git a/bolt/unittests/Core/MCPlusBuilder.cpp b/bolt/unittests/Core/MCPlusBuilder.cpp
index 02ecb87b4a5e3..e08ae09e76027 100644
--- a/bolt/unittests/Core/MCPlusBuilder.cpp
+++ b/bolt/unittests/Core/MCPlusBuilder.cpp
@@ -196,6 +196,111 @@ TEST_P(MCPlusBuilderTester, AArch64_BTI) {
   ASSERT_TRUE(BC->MIB->isImplicitBTIC(*II));
 }
 
+TEST_P(MCPlusBuilderTester, AArch64_addBTItoBBStart_0) {
+  if (GetParam() != Triple::aarch64)
+    GTEST_SKIP();
+  BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true);
+  std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock();
+  MCInst Inst = MCInstBuilder(AArch64::RET).addReg(AArch64::LR);
+  BB->addInstruction(Inst);
+  // BR x16 needs BTI c or BTI j. We prefer adding a BTI c.
+  MCInst CallInst = MCInstBuilder(AArch64::BR).addReg(AArch64::X16);
+  BC->MIB->addBTItoBBStart(*BB, CallInst);
+  auto II = BB->begin();
+  ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, false));
+}
+
+TEST_P(MCPlusBuilderTester, AArch64_addBTItoBBStart_1) {
+  if (GetParam() != Triple::aarch64)
+    GTEST_SKIP();
+  BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true);
+  std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock();
+  MCInst BTIc;
+  BC->MIB->createBTI(BTIc, true, false);
+  BB->addInstruction(BTIc);
+  // BR x16 needs BTI c or BTI j. We have a BTI c, no change is needed.
+  MCInst CallInst = MCInstBuilder(AArch64::BR).addReg(AArch64::X16);
+  BC->MIB->addBTItoBBStart(*BB, CallInst);
+  auto II = BB->begin();
+  ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, false));
+}
+
+TEST_P(MCPlusBuilderTester, AArch64_addBTItoBBStart_2) {
+  if (GetParam() != Triple::aarch64)
+    GTEST_SKIP();
+  BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true);
+  std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock();
+  MCInst BTIc;
+  BC->MIB->createBTI(BTIc, true, false);
+  BB->addInstruction(BTIc);
+  // BR x5 needs BTI j
+  // we have BTI c -> extend it to BTI jc.
+  MCInst CallInst = MCInstBuilder(AArch64::BR).addReg(AArch64::X5);
+  BC->MIB->addBTItoBBStart(*BB, CallInst);
+  auto II = BB->begin();
+  ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, true));
+}
+
+TEST_P(MCPlusBuilderTester, AArch64_addBTItoBBStart_3) {
+  if (GetParam() != Triple::aarch64)
+    GTEST_SKIP();
+  BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true);
+  std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock();
+  MCInst Inst = MCInstBuilder(AArch64::RET).addReg(AArch64::LR);
+  BB->addInstruction(Inst);
+  // BR x5 needs BTI j
+  MCInst CallInst = MCInstBuilder(AArch64::BR).addReg(AArch64::X5);
+  BC->MIB->addBTItoBBStart(*BB, CallInst);
+  auto II = BB->begin();
+  ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, false, true));
+}
+
+TEST_P(MCPlusBuilderTester, AArch64_addBTItoBBStart_4) {
+  if (GetParam() != Triple::aarch64)
+    GTEST_SKIP();
+  BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true);
+  std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock();
+  MCInst Inst = MCInstBuilder(AArch64::RET).addReg(AArch64::LR);
+  BB->addInstruction(Inst);
+  // BLR needs BTI c, regardless of the register used.
+  MCInst CallInst = MCInstBuilder(AArch64::BLR).addReg(AArch64::X5);
+  BC->MIB->addBTItoBBStart(*BB, CallInst);
+  auto II = BB->begin();
+  ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, false));
+}
+
+TEST_P(MCPlusBuilderTester, AArch64_addBTItoBBStart_5) {
+  if (GetParam() != Triple::aarch64)
+    GTEST_SKIP();
+  BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true);
+  std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock();
+  MCInst BTIj;
+  BC->MIB->createBTI(BTIj, false, true);
+  BB->addInstruction(BTIj);
+  // BLR needs BTI c, regardless of the register used.
+  // We have a BTI j -> extend it to BTI jc.
+  MCInst CallInst = MCInstBuilder(AArch64::BLR).addReg(AArch64::X5);
+  BC->MIB->addBTItoBBStart(*BB, CallInst);
+  auto II = BB->begin();
+  ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, true));
+}
+
+TEST_P(MCPlusBuilderTester, AArch64_addBTItoBBStart_6) {
+  if (GetParam() != Triple::aarch64)
+    GTEST_SKIP();
+  BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true);
+  std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock();
+  MCInst Paciasp =
+      MCInstBuilder(AArch64::PACIASP).addReg(AArch64::LR).addReg(AArch64::SP);
+  BB->addInstruction(Paciasp);
+  // PACI(AB)SP are implicit BTI c, no change needed.
+  MCInst CallInst = MCInstBuilder(AArch64::BR).addReg(AArch64::X17);
+  BC->MIB->addBTItoBBStart(*BB, CallInst);
+  auto II = BB->begin();
+  ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, false));
+  ASSERT_TRUE(BC->MIB->isPSignOnLR(*II));
+}
+
 TEST_P(MCPlusBuilderTester, AArch64_CmpJNE) {
   if (GetParam() != Triple::aarch64)
     GTEST_SKIP();

[peterwaller-arm]

Nit: should it be addBTIToBBStart?

If we're entertaining a rename, how about insert rather than add - since normally I think of putting something at the beginning of a list as 'insertion' rather than 'addition'. insertBTI would be a clear verb for a function name (and 'ToBBStart would be a bit redundant since presumably we won't expect to insert a BTI anywhere else?)

[bgergely0]

insert is probably better.

(and 'ToBBStart would be a bit redundant since presumably we won't expect to insert a BTI anywhere else?)

agree.
The naming should also imply that we don't necessarily insert a new instruction, only if the instruction already at the BBStart is not a BTI (or not compatible with a BTI).
WDYT?

[peterwaller-arm]

ensureBTITarget?
makeBTITarget?

On the other hand insertBTI I think should be reasonably clear. If it begins with if (alreadyBTITarget) return; that shouldn't be too surprising to a developer given the purpose of BTI. At least I can't think of a good reason to need to double them up.

[bgergely0]

I think I have previously encountered empty BasicBlocks - that was around function splitting I believe. Not sure if empty BasicBlocks should be covered here. Currently, we silently skip them - that does not seem ideal.