[BOLT][NFC] Review nits

- remove unused imports
- fix clangd warnings
- rename functions
- improve comments
- reformat lib/Core/Exceptions.cpp

Author: bgergely0

bolt/include/bolt/Core/MCPlusBuilder.h

@@ -78,7 +78,7 @@ class MCPlusBuilder {
     if (*FirstAnnotationOp > OpIndex || Inst.getNumOperands() < OpIndex)
       return std::nullopt;
 
-    auto Op = Inst.begin() + OpIndex;
+    const auto *Op = Inst.begin() + OpIndex;
     const int64_t ImmValue = Op->getImm();
     return extractAnnotationIndex(ImmValue);
   }

bolt/include/bolt/Passes/InsertNegateRAStatePass.h

@@ -13,7 +13,6 @@
 #define BOLT_PASSES_INSERT_NEGATE_RA_STATE_PASS
 
 #include "bolt/Passes/BinaryPasses.h"
-#include <stack>
 
 namespace llvm {
 namespace bolt {
@@ -31,14 +30,17 @@ class InsertNegateRAState : public BinaryFunctionPass {
 private:
   /// Loops over all instructions and adds OpNegateRAState CFI
   /// after any pointer signing or authenticating instructions,
-  /// which operate on the LR, except fused ptrauth + ret instructions
-  /// (such as RETAA).
+  /// which operate on the LR, except fused pauth + ret instructions
+  /// (such as RETAA). Normal pauth and psign instructions are "special cases",
+  /// meaning they always need an OpNegateRAState CFI after them.
+  /// Fused pauth + ret instructions are not, they work as any other
+  /// instruction.
   /// Returns true, if any OpNegateRAState CFIs were added.
-  bool addNegateRAStateAfterPacOrAuth(BinaryFunction &BF);
+  bool addNegateRAStateAfterPSignOrPAuth(BinaryFunction &BF);
   /// Because states are tracked as MCAnnotations on individual instructions,
   /// newly inserted instructions do not have a state associated with them.
   /// New states are "inherited" from the last known state.
-  void fixUnknownStates(BinaryFunction &BF);
+  void inferUnknownStates(BinaryFunction &BF);
 };
 
 } // namespace bolt

bolt/lib/Core/Exceptions.cpp

@@ -648,12 +648,12 @@ bool CFIReaderWriter::fillCFIInfoFor(BinaryFunction &Function) const {
       // same id but mean different things. The latter is used in AArch64.
       if (Function.getBinaryContext().isAArch64()) {
         Function.setContainedNegateRAState();
-        // The location OpNegateRAState CFIs are needed
-        // depends on the order of BasicBlocks, which changes during
-        // optimizations. Instead of adding OpNegateRAState CFIs, an annotation
-        // is added to the instruction, to mark that the instruction modifies
-        // the RA State. The actual state for instructions are worked out in
-        // MarkRAStates based on these annotations.
+        // The location OpNegateRAState CFIs are needed depends on the order of
+        // BasicBlocks, which changes during optimizations. Instead of adding
+        // OpNegateRAState CFIs, an annotation is added to the instruction, to
+        // mark that the instruction modifies the RA State. The actual state for
+        // instructions are worked out in MarkRAStates based on these
+        // annotations.
         if (Offset != 0)
           Function.setInstModifiesRAState(DW_CFA_AARCH64_negate_ra_state,
                                           Offset);

bolt/lib/Passes/InsertNegateRAStatePass.cpp

@@ -14,10 +14,7 @@
 #include "bolt/Passes/InsertNegateRAStatePass.h"
 #include "bolt/Core/BinaryFunction.h"
 #include "bolt/Core/ParallelUtilities.h"
-#include "bolt/Utils/CommandLineOpts.h"
 #include <cstdlib>
-#include <fstream>
-#include <iterator>
 
 using namespace llvm;
 
@@ -38,10 +35,10 @@ void InsertNegateRAState::runOnFunction(BinaryFunction &BF) {
   }
 
   // If none is inserted, the function doesn't need more work.
-  if (!addNegateRAStateAfterPacOrAuth(BF))
+  if (!addNegateRAStateAfterPSignOrPAuth(BF))
     return;
 
-  fixUnknownStates(BF);
+  inferUnknownStates(BF);
 
   // Support for function splitting:
   // if two consecutive BBs with Signed state are going to end up in different
@@ -53,7 +50,7 @@ void InsertNegateRAState::runOnFunction(BinaryFunction &BF) {
     // BOLT can generate empty BBs at function splitting which are only used as
     // target labels. We should add the negate-ra-state CFI to the first
     // non-empty BB.
-    auto FirstNonEmpty =
+    auto *FirstNonEmpty =
         std::find_if(FF.begin(), FF.end(), [](BinaryBasicBlock *BB) {
           // getFirstNonPseudo returns BB.end() if it does not find any
           // Instructions.
@@ -92,7 +89,8 @@ void InsertNegateRAState::runOnFunction(BinaryFunction &BF) {
   }
 }
 
-bool InsertNegateRAState::addNegateRAStateAfterPacOrAuth(BinaryFunction &BF) {
+bool InsertNegateRAState::addNegateRAStateAfterPSignOrPAuth(
+    BinaryFunction &BF) {
   BinaryContext &BC = BF.getBinaryContext();
   bool FoundAny = false;
   for (BinaryBasicBlock &BB : BF) {
@@ -109,7 +107,7 @@ bool InsertNegateRAState::addNegateRAStateAfterPacOrAuth(BinaryFunction &BF) {
   return FoundAny;
 }
 
-void InsertNegateRAState::fixUnknownStates(BinaryFunction &BF) {
+void InsertNegateRAState::inferUnknownStates(BinaryFunction &BF) {
   BinaryContext &BC = BF.getBinaryContext();
   bool FirstIter = true;
   MCInst PrevInst;

bolt/lib/Passes/MarkRAStates.cpp

@@ -22,12 +22,7 @@
 #include "bolt/Passes/MarkRAStates.h"
 #include "bolt/Core/BinaryFunction.h"
 #include "bolt/Core/ParallelUtilities.h"
-#include "bolt/Utils/CommandLineOpts.h"
 #include <cstdlib>
-#include <fstream>
-#include <iterator>
-
-#include <iostream>
 #include <optional>
 #include <stack>
 
@@ -49,11 +44,9 @@ void MarkRAStates::runOnFunction(BinaryFunction &BF) {
       if ((BC.MIB->isPSignOnLR(Inst) ||
            (BC.MIB->isPAuthOnLR(Inst) && !BC.MIB->isPAuthAndRet(Inst))) &&
           !BC.MIB->hasNegateRAState(Inst)) {
-        // no .cfi_negate_ra_state attached to signing or authenticating instr
-        // means, that this is a function with handwritten assembly, which might
-        // not respect Clang's conventions (e.g. tailcalls are always
-        // authenticated, so functions always start with unsigned RAState when
-        // working with compiler-generated code)
+        // Not all functions have .cfi_negate_ra_state in them. But if one does,
+        // we expect psign/pauth instructions to have the hasNegateRAState
+        // annotation.
         BF.setIgnored();
         BC.outs() << "BOLT-INFO: inconsistent RAStates in function "
                   << BF.getPrintName() << "\n";