[BOLT][AArch64] Handle PAuth call instructions in isIndirectCall #133227

atrosinenko · 2025-03-27T10:20:21Z

Handle BLRA* opcodes in AArch64MCPlusBuilder::isIndirectCall, update getRegUsedAsCallDest accordingly.

atrosinenko · 2025-03-27T10:21:09Z

This PR is intended to be a follow-up for #131899.

jacobbramley

It looks sensible to me!

atrosinenko · 2025-03-27T16:37:03Z

@jacobbramley Do you suggest merging this first and then updating #131899? My initial idea was to first merge #131899 after addressing the remaining comments, then update this PR (by changing getRegUsedAsCallDest which does not exist in main yet) and mark it as ready for review. As far as I understand the semantics of "draft PR", the reviewers are only notified after the PR is marked as ready for review, thus after "undrafting" this PR, I would wait for at least another 24h in case there are any objections.

kbeyls

LGTM, thanks

Handle `BLRA*` opcodes in AArch64MCPlusBuilder::isIndirectCall, update getRegUsedAsCallDest accordingly.

atrosinenko · 2025-04-04T11:58:35Z

Updated and un-drafted this PR after #131899 was merged into main branch. Resetting the approvals as the patch was modified significantly.

llvmbot · 2025-04-04T11:59:00Z

@llvm/pr-subscribers-backend-aarch64

Author: Anatoly Trosinenko (atrosinenko)

Changes

Handle BLRA* opcodes in AArch64MCPlusBuilder::isIndirectCall, update`getRegUsedAsCallDest accordingly.

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

4 Files Affected:

(modified) bolt/include/bolt/Core/MCPlusBuilder.h (+5-5)
(modified) bolt/lib/Passes/PAuthGadgetScanner.cpp (+5-3)
(modified) bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp (+7-9)
(modified) llvm/lib/Target/AArch64/AArch64InstrInfo.h (+13)

diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index bbef65700b2a5..5b7779f255556 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -577,12 +577,12 @@ class MCPlusBuilder {
     return getNoRegister();
   }
 
-  /// Returns the register used as call destination, or no-register, if not
-  /// an indirect call. Sets IsAuthenticatedInternally if the instruction
-  /// accepts a signed pointer as its operand and authenticates it internally.
+  /// Returns the register used as the destination of an indirect branch or call
+  /// instruction. Sets IsAuthenticatedInternally if the instruction accepts
+  /// a signed pointer as its operand and authenticates it internally.
   virtual MCPhysReg
-  getRegUsedAsCallDest(const MCInst &Inst,
-                       bool &IsAuthenticatedInternally) const {
+  getRegUsedAsIndirectBranchDest(const MCInst &Inst,
+                                 bool &IsAuthenticatedInternally) const {
     llvm_unreachable("not implemented");
     return getNoRegister();
   }
diff --git a/bolt/lib/Passes/PAuthGadgetScanner.cpp b/bolt/lib/Passes/PAuthGadgetScanner.cpp
index a3b320c545734..8710eba77097d 100644
--- a/bolt/lib/Passes/PAuthGadgetScanner.cpp
+++ b/bolt/lib/Passes/PAuthGadgetScanner.cpp
@@ -457,14 +457,16 @@ static std::shared_ptr<Report>
 shouldReportCallGadget(const BinaryContext &BC, const MCInstReference &Inst,
                        const State &S) {
   static const GadgetKind CallKind("non-protected call found");
-  if (!BC.MIB->isCall(Inst) && !BC.MIB->isBranch(Inst))
+  if (!BC.MIB->isIndirectCall(Inst) && !BC.MIB->isIndirectBranch(Inst))
     return nullptr;
 
   bool IsAuthenticated = false;
-  MCPhysReg DestReg = BC.MIB->getRegUsedAsCallDest(Inst, IsAuthenticated);
-  if (IsAuthenticated || DestReg == BC.MIB->getNoRegister())
+  MCPhysReg DestReg =
+      BC.MIB->getRegUsedAsIndirectBranchDest(Inst, IsAuthenticated);
+  if (IsAuthenticated)
     return nullptr;
 
+  assert(DestReg != BC.MIB->getNoRegister());
   LLVM_DEBUG({
     traceInst(BC, "Found call inst", Inst);
     traceReg(BC, "Call destination reg", DestReg);
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index 2a648baa4d514..5ecc30b8bb107 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -10,6 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "AArch64InstrInfo.h"
 #include "AArch64MCSymbolizer.h"
 #include "MCTargetDesc/AArch64AddressingModes.h"
 #include "MCTargetDesc/AArch64FixupKinds.h"
@@ -277,15 +278,14 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
     }
   }
 
-  MCPhysReg
-  getRegUsedAsCallDest(const MCInst &Inst,
-                       bool &IsAuthenticatedInternally) const override {
-    assert(isCall(Inst) || isBranch(Inst));
-    IsAuthenticatedInternally = false;
+  MCPhysReg getRegUsedAsIndirectBranchDest(
+      const MCInst &Inst, bool &IsAuthenticatedInternally) const override {
+    assert(isIndirectCall(Inst) || isIndirectBranch(Inst));
 
     switch (Inst.getOpcode()) {
     case AArch64::BR:
     case AArch64::BLR:
+      IsAuthenticatedInternally = false;
       return Inst.getOperand(0).getReg();
     case AArch64::BRAA:
     case AArch64::BRAB:
@@ -298,9 +298,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
       IsAuthenticatedInternally = true;
       return Inst.getOperand(0).getReg();
     default:
-      if (isIndirectCall(Inst) || isIndirectBranch(Inst))
-        llvm_unreachable("Unhandled indirect branch");
-      return getNoRegister();
+      llvm_unreachable("Unhandled indirect branch or call");
     }
   }
 
@@ -662,7 +660,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
   }
 
   bool isIndirectCall(const MCInst &Inst) const override {
-    return Inst.getOpcode() == AArch64::BLR;
+    return isIndirectCallOpcode(Inst.getOpcode());
   }
 
   MCPhysReg getSpRegister(int Size) const {
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.h b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
index b3d3ec1455c8b..0ffaca9af4006 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.h
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
@@ -726,6 +726,19 @@ static inline bool isIndirectBranchOpcode(int Opc) {
   return false;
 }
 
+static inline bool isIndirectCallOpcode(unsigned Opc) {
+  switch (Opc) {
+  case AArch64::BLR:
+  case AArch64::BLRAA:
+  case AArch64::BLRAB:
+  case AArch64::BLRAAZ:
+  case AArch64::BLRABZ:
+    return true;
+  default:
+    return false;
+  }
+}
+
 static inline bool isPTrueOpcode(unsigned Opc) {
   switch (Opc) {
   case AArch64::PTRUE_B:

llvmbot · 2025-04-04T11:59:01Z

@llvm/pr-subscribers-bolt

Author: Anatoly Trosinenko (atrosinenko)

Changes

Handle BLRA* opcodes in AArch64MCPlusBuilder::isIndirectCall, update`getRegUsedAsCallDest accordingly.

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

4 Files Affected:

(modified) bolt/include/bolt/Core/MCPlusBuilder.h (+5-5)
(modified) bolt/lib/Passes/PAuthGadgetScanner.cpp (+5-3)
(modified) bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp (+7-9)
(modified) llvm/lib/Target/AArch64/AArch64InstrInfo.h (+13)

diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index bbef65700b2a5..5b7779f255556 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -577,12 +577,12 @@ class MCPlusBuilder {
     return getNoRegister();
   }
 
-  /// Returns the register used as call destination, or no-register, if not
-  /// an indirect call. Sets IsAuthenticatedInternally if the instruction
-  /// accepts a signed pointer as its operand and authenticates it internally.
+  /// Returns the register used as the destination of an indirect branch or call
+  /// instruction. Sets IsAuthenticatedInternally if the instruction accepts
+  /// a signed pointer as its operand and authenticates it internally.
   virtual MCPhysReg
-  getRegUsedAsCallDest(const MCInst &Inst,
-                       bool &IsAuthenticatedInternally) const {
+  getRegUsedAsIndirectBranchDest(const MCInst &Inst,
+                                 bool &IsAuthenticatedInternally) const {
     llvm_unreachable("not implemented");
     return getNoRegister();
   }
diff --git a/bolt/lib/Passes/PAuthGadgetScanner.cpp b/bolt/lib/Passes/PAuthGadgetScanner.cpp
index a3b320c545734..8710eba77097d 100644
--- a/bolt/lib/Passes/PAuthGadgetScanner.cpp
+++ b/bolt/lib/Passes/PAuthGadgetScanner.cpp
@@ -457,14 +457,16 @@ static std::shared_ptr<Report>
 shouldReportCallGadget(const BinaryContext &BC, const MCInstReference &Inst,
                        const State &S) {
   static const GadgetKind CallKind("non-protected call found");
-  if (!BC.MIB->isCall(Inst) && !BC.MIB->isBranch(Inst))
+  if (!BC.MIB->isIndirectCall(Inst) && !BC.MIB->isIndirectBranch(Inst))
     return nullptr;
 
   bool IsAuthenticated = false;
-  MCPhysReg DestReg = BC.MIB->getRegUsedAsCallDest(Inst, IsAuthenticated);
-  if (IsAuthenticated || DestReg == BC.MIB->getNoRegister())
+  MCPhysReg DestReg =
+      BC.MIB->getRegUsedAsIndirectBranchDest(Inst, IsAuthenticated);
+  if (IsAuthenticated)
     return nullptr;
 
+  assert(DestReg != BC.MIB->getNoRegister());
   LLVM_DEBUG({
     traceInst(BC, "Found call inst", Inst);
     traceReg(BC, "Call destination reg", DestReg);
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index 2a648baa4d514..5ecc30b8bb107 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -10,6 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "AArch64InstrInfo.h"
 #include "AArch64MCSymbolizer.h"
 #include "MCTargetDesc/AArch64AddressingModes.h"
 #include "MCTargetDesc/AArch64FixupKinds.h"
@@ -277,15 +278,14 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
     }
   }
 
-  MCPhysReg
-  getRegUsedAsCallDest(const MCInst &Inst,
-                       bool &IsAuthenticatedInternally) const override {
-    assert(isCall(Inst) || isBranch(Inst));
-    IsAuthenticatedInternally = false;
+  MCPhysReg getRegUsedAsIndirectBranchDest(
+      const MCInst &Inst, bool &IsAuthenticatedInternally) const override {
+    assert(isIndirectCall(Inst) || isIndirectBranch(Inst));
 
     switch (Inst.getOpcode()) {
     case AArch64::BR:
     case AArch64::BLR:
+      IsAuthenticatedInternally = false;
       return Inst.getOperand(0).getReg();
     case AArch64::BRAA:
     case AArch64::BRAB:
@@ -298,9 +298,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
       IsAuthenticatedInternally = true;
       return Inst.getOperand(0).getReg();
     default:
-      if (isIndirectCall(Inst) || isIndirectBranch(Inst))
-        llvm_unreachable("Unhandled indirect branch");
-      return getNoRegister();
+      llvm_unreachable("Unhandled indirect branch or call");
     }
   }
 
@@ -662,7 +660,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
   }
 
   bool isIndirectCall(const MCInst &Inst) const override {
-    return Inst.getOpcode() == AArch64::BLR;
+    return isIndirectCallOpcode(Inst.getOpcode());
   }
 
   MCPhysReg getSpRegister(int Size) const {
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.h b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
index b3d3ec1455c8b..0ffaca9af4006 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.h
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
@@ -726,6 +726,19 @@ static inline bool isIndirectBranchOpcode(int Opc) {
   return false;
 }
 
+static inline bool isIndirectCallOpcode(unsigned Opc) {
+  switch (Opc) {
+  case AArch64::BLR:
+  case AArch64::BLRAA:
+  case AArch64::BLRAB:
+  case AArch64::BLRAAZ:
+  case AArch64::BLRABZ:
+    return true;
+  default:
+    return false;
+  }
+}
+
 static inline bool isPTrueOpcode(unsigned Opc) {
   switch (Opc) {
   case AArch64::PTRUE_B:

jacobbramley · 2025-04-04T12:55:18Z

@jacobbramley Do you suggest merging this first and then updating #131899?

I think that ordering makes sense, and is probably what I'd do, but I'm far from an expert in GitHub workflows (and I'm not a regular LLVM contributor so I don't know the local conventions well).

jacobbramley

LGTM, but I'm not sure if my approval is sufficient here.

atrosinenko · 2025-04-04T13:01:42Z

@jacobbramley Do you suggest merging this first and then updating #131899?

I think that ordering makes sense, and is probably what I'd do, but I'm far from an expert in GitHub workflows (and I'm not a regular LLVM contributor so I don't know the local conventions well).

Probably some outdated notification was sent only after the PR was marked as ready for review. Actually, #131899 is already merged - that is the reason for updating this PR.

kbeyls

LGTM!

llvm-ci · 2025-04-08T10:31:38Z

LLVM Buildbot has detected a new failure on builder clang-cmake-x86_64-avx512-win running on avx512-intel64-win while building bolt,llvm at step 4 "cmake stage 1".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/81/builds/6095

Here is the relevant piece of the build log for the reference

Step 4 (cmake stage 1) failure: 'cmake -G ...' (failure)
'cmake' is not recognized as an internal or external command,
operable program or batch file.

atrosinenko requested review from asl, jacobbramley and kbeyls March 27, 2025 10:20

atrosinenko mentioned this pull request Mar 27, 2025

[BOLT] Gadget scanner: detect non-protected indirect calls #131899

Merged

jacobbramley approved these changes Mar 27, 2025

View reviewed changes

kbeyls approved these changes Apr 3, 2025

View reviewed changes

[BOLT][AArch64] Handle PAuth call instructions in isIndirectCall

ff463e2

Handle `BLRA*` opcodes in AArch64MCPlusBuilder::isIndirectCall, update getRegUsedAsCallDest accordingly.

atrosinenko force-pushed the bolt-aarch64-fix-isindirectcall branch from 71b8787 to ff463e2 Compare April 4, 2025 11:53

atrosinenko marked this pull request as ready for review April 4, 2025 11:58

atrosinenko requested review from aaupov, maksfb, rafaelauler, ayermolo, dcci and yota9 as code owners April 4, 2025 11:58

llvmbot added backend:AArch64 BOLT labels Apr 4, 2025

atrosinenko requested review from jacobbramley and kbeyls April 4, 2025 11:58

jacobbramley approved these changes Apr 4, 2025

View reviewed changes

kbeyls approved these changes Apr 4, 2025

View reviewed changes

atrosinenko merged commit 8521bd2 into llvm:main Apr 8, 2025
15 checks passed

atrosinenko deleted the bolt-aarch64-fix-isindirectcall branch April 8, 2025 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[BOLT][AArch64] Handle PAuth call instructions in isIndirectCall #133227

[BOLT][AArch64] Handle PAuth call instructions in isIndirectCall #133227

Uh oh!

atrosinenko commented Mar 27, 2025 •

edited

Loading

Uh oh!

atrosinenko commented Mar 27, 2025

Uh oh!

jacobbramley left a comment

Uh oh!

atrosinenko commented Mar 27, 2025

Uh oh!

kbeyls left a comment

Uh oh!

atrosinenko commented Apr 4, 2025

Uh oh!

llvmbot commented Apr 4, 2025

Uh oh!

llvmbot commented Apr 4, 2025

Uh oh!

jacobbramley commented Apr 4, 2025

Uh oh!

jacobbramley left a comment

Uh oh!

atrosinenko commented Apr 4, 2025

Uh oh!

kbeyls left a comment

Uh oh!

Uh oh!

llvm-ci commented Apr 8, 2025

Uh oh!

Uh oh!

[BOLT][AArch64] Handle PAuth call instructions in isIndirectCall #133227

[BOLT][AArch64] Handle PAuth call instructions in isIndirectCall #133227

Uh oh!

Conversation

atrosinenko commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atrosinenko commented Mar 27, 2025

Uh oh!

jacobbramley left a comment

Choose a reason for hiding this comment

Uh oh!

atrosinenko commented Mar 27, 2025

Uh oh!

kbeyls left a comment

Choose a reason for hiding this comment

Uh oh!

atrosinenko commented Apr 4, 2025

Uh oh!

llvmbot commented Apr 4, 2025

Uh oh!

llvmbot commented Apr 4, 2025

Uh oh!

jacobbramley commented Apr 4, 2025

Uh oh!

jacobbramley left a comment

Choose a reason for hiding this comment

Uh oh!

atrosinenko commented Apr 4, 2025

Uh oh!

kbeyls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvm-ci commented Apr 8, 2025

Uh oh!

Uh oh!

atrosinenko commented Mar 27, 2025 •

edited

Loading