[BOLT] Review nits (NFC)

bgergely0 · bgergely0 · commit 3fb8a1e0edf9 · 2025-05-23T14:45:24.000+02:00
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
@@ -179,6 +179,15 @@ template <typename R> static bool emptyRange(const R &Range) {
   return Range.begin() == Range.end();
 }
 
+static void checkFlagsForPacRet() {
+  if (!(opts::BinaryAnalysisMode || opts::HeatmapMode || opts::AllowPacret)) {
+    llvm_unreachable(
+        "BOLT-ERROR: support for binaries using pac-ret hardening (e.g. as "
+        "produced by '-mbranch-protection=pac-ret') is experimental\n"
+        "BOLT-ERROR: set --allow-experimental-pacret to allow processing");
+  }
+}
+
 /// Gets debug line information for the instruction located at the given
 /// address in the original binary. The SMLoc's pointer is used
 /// to point to this information, which is represented by a
@@ -2786,13 +2795,7 @@ struct CFISnapshot {
       llvm_unreachable("unsupported CFI opcode");
       break;
     case MCCFIInstruction::OpNegateRAState:
-      if (!(opts::BinaryAnalysisMode || opts::HeatmapMode ||
-            opts::AllowPacret)) {
-        llvm_unreachable(
-            "BOLT-ERROR: support for binaries using pac-ret hardening (e.g. as "
-            "produced by '-mbranch-protection=pac-ret') is experimental\n"
-            "BOLT-ERROR: set --allow-experimental-pacret to allow processing");
-      }
+      checkFlagsForPacRet();
       break;
     case MCCFIInstruction::OpRememberState:
     case MCCFIInstruction::OpRestoreState:
@@ -2934,13 +2937,7 @@ struct CFISnapshotDiff : public CFISnapshot {
       llvm_unreachable("unsupported CFI opcode");
       return false;
     case MCCFIInstruction::OpNegateRAState:
-      if (!(opts::BinaryAnalysisMode || opts::HeatmapMode ||
-            opts::AllowPacret)) {
-        llvm_unreachable(
-            "BOLT-ERROR: support for binaries using pac-ret hardening (e.g. as "
-            "produced by '-mbranch-protection=pac-ret') is experimental\n"
-            "BOLT-ERROR: set --allow-experimental-pacret to allow processing");
-      }
+      checkFlagsForPacRet();
       break;
     case MCCFIInstruction::OpRememberState:
     case MCCFIInstruction::OpRestoreState:
@@ -3093,13 +3090,7 @@ BinaryFunction::unwindCFIState(int32_t FromState, int32_t ToState,
       llvm_unreachable("unsupported CFI opcode");
       break;
     case MCCFIInstruction::OpNegateRAState:
-      if (!(opts::BinaryAnalysisMode || opts::HeatmapMode ||
-            opts::AllowPacret)) {
-        llvm_unreachable(
-            "BOLT-ERROR: support for binaries using pac-ret hardening (e.g. as "
-            "produced by '-mbranch-protection=pac-ret') is experimental\n"
-            "BOLT-ERROR: set --allow-experimental-pacret to allow processing");
-      }
+      checkFlagsForPacRet();
       break;
     case MCCFIInstruction::OpGnuArgsSize:
       // do not affect CFI state
diff --git a/bolt/lib/Rewrite/BinaryPassManager.cpp b/bolt/lib/Rewrite/BinaryPassManager.cpp
@@ -275,11 +275,6 @@ static cl::opt<bool> ShortenInstructions("shorten-instructions",
                                          cl::desc("shorten instructions"),
                                          cl::init(true),
                                          cl::cat(BoltOptCategory));
-
-cl::opt<bool> AllowPacret(
-    "allow-experimental-pacret",
-    cl::desc("Enable processing binaries with pac-ret (experimental)"),
-    cl::cat(BoltOptCategory));
 } // namespace opts
 
 namespace llvm {
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -283,17 +283,12 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
 
   bool isPSignOnLR(const MCInst &Inst) const override {
     ErrorOr<MCPhysReg> SignReg = getSignedReg(Inst);
-    if (SignReg && *SignReg != getNoRegister() && *SignReg == AArch64::LR)
-      return true;
-
-    return false;
+    return SignReg && *SignReg != getNoRegister() && *SignReg == AArch64::LR;
   }
 
   bool isPAuthOnLR(const MCInst &Inst) const override {
     ErrorOr<MCPhysReg> AutReg = getAuthenticatedReg(Inst);
-    if (AutReg && *AutReg != getNoRegister() && *AutReg == AArch64::LR)
-      return true;
-    return false;
+    return AutReg && *AutReg != getNoRegister() && *AutReg == AArch64::LR;
   }
 
   bool isPAuthAndRet(const MCInst &Inst) const override {
diff --git a/bolt/lib/Utils/CommandLineOpts.cpp b/bolt/lib/Utils/CommandLineOpts.cpp
@@ -228,6 +228,11 @@ cl::opt<unsigned>
               cl::init(0), cl::ZeroOrMore, cl::cat(BoltCategory),
               cl::sub(cl::SubCommand::getAll()));
 
+cl::opt<bool> AllowPacret(
+    "allow-experimental-pacret",
+    cl::desc("Enable processing binaries with pac-ret (experimental)"),
+    cl::init(false), cl::cat(BoltOptCategory));
+
 bool processAllFunctions() {
   if (opts::AggregateOnly)
     return false;
diff --git a/bolt/test/AArch64/negate-ra-state-incorrect.s b/bolt/test/AArch64/negate-ra-state-incorrect.s
@@ -1,6 +1,7 @@
 # RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o
 # RUN: %clang %cflags  %t.o -o %t.exe -Wl,-q
 # RUN: llvm-bolt %t.exe -o %t.exe.bolt | FileCheck %s
+
 # check that the output is listing foo as incorrect
 # CHECK: BOLT-INFO: inconsistent RAStates in function foo
 
@@ -9,10 +10,10 @@
 # RUN: not grep "<foo>:" %t.exe.dump
 
 
-# Why is this test incorrect?
-#   There is an extra .cfi_negate_ra_state in line ...
-#   Because of this, we will get to the autiasp (hint #29)
-#   in a (seemingly) unsigned state. That is incorrect.
+# How is this test incorrect?
+# There is an extra .cfi_negate_ra_state in foo.
+# Because of this, we will get to the autiasp (hint #29)
+# in a (seemingly) unsigned state. That is incorrect.
   .text
   .globl  foo
   .p2align        2
diff --git a/bolt/test/AArch64/negate-ra-state.s b/bolt/test/AArch64/negate-ra-state.s
@@ -2,7 +2,7 @@
 # RUN: %clang %cflags  %t.o -o %t.exe -Wl,-q
 
 # RUN: llvm-objdump %t.exe -d > %t.exe.dump
-# RUN: llvm-objdump --dwarf=frames %t.exe -D > %t.exe.dump-dwarf
+# RUN: llvm-objdump --dwarf=frames %t.exe > %t.exe.dump-dwarf
 # RUN: match-dwarf %t.exe.dump %t.exe.dump-dwarf foo > %t.match-dwarf.txt
 
 # RUN: llvm-bolt %t.exe -o %t.exe.bolt
diff --git a/bolt/test/match_dwarf.py b/bolt/test/match_dwarf.py
@@ -39,10 +39,9 @@ def print(self):
 
     def parse_negate_offsets(self):
         """
-        Create a list of locations/offsets of the negate_ra_state
-        CFIs in the dwarf entry.
-        To find offsets for each, we match the DW_CFA_advance_loc entries,
-        and sum up their values.
+        Create a list of locations/offsets of the negate_ra_state CFIs in the
+        dwarf entry. To find offsets for each, we match the DW_CFA_advance_loc
+        entries, and sum up their values.
         """
         negate_offsets = []
         loc = 0
@@ -65,10 +64,10 @@ def __eq__(self, other):
 
 def extract_function_addresses(objdump):
     """
-    Parse and return address-to-name dictionary from objdump file
+    Parse and return address-to-name dictionary from objdump file.
     Function names in the objdump look like this:
         000123abc <foo>:
-    We want to create a dict from the addr (000123abc), to the name (foo).
+    We create a dict from the addr (000123abc), to the name (foo).
     """
     addr_name_dict = dict()
     re_function = re.compile(r"^([0-9a-fA-F]+)\s<(.*)>:$")
@@ -91,10 +90,10 @@ def match_dwarf_to_name(dwarfdump, addr_name_dict):
     The matched lines look like this:
     000123 000456 000789 FDE cie=000000  pc=0123abc...0456def
     We do not have the function name for this, only the PC range it applies to.
-    We need to find the pc=0123abc (the start address), and find the matching name from
+    We match the pc=0123abc (the start address), and find the matching name from
     the addr_name_dict.
-    The result NameDwarfPair will hold the lines this header applied to, and instead of
-    the header with the addresses, it will just have the function name.
+    The resultint NameDwarfPair will hold the lines this header applied to, and
+    instead of the header with the addresses, it will just have the function name.
     """
     re_address_line = re.compile(r".*pc=([0-9a-fA-F]+)\.\.\.([0-9a-fA-F]+)")
     with open(dwarfdump, "r") as dw:
