Review fixes

Author: bgergely0

bolt/lib/Passes/InsertNegateRAStatePass.cpp

@@ -131,6 +131,12 @@ InsertNegateRAState::getFirstKnownRAState(BinaryContext &BC,
   return std::nullopt;
 }
 
+bool InsertNegateRAState::isUnknownBlock(BinaryContext &BC,
+                                         BinaryBasicBlock &BB) {
+  std::optional<bool> FirstRAState = getFirstKnownRAState(BC, BB);
+  return !FirstRAState.has_value();
+}
+
 void InsertNegateRAState::fillUnknownStateInBB(BinaryContext &BC,
                                                BinaryBasicBlock &BB) {
 
@@ -174,18 +180,6 @@ void InsertNegateRAState::fillUnknownStateInBB(BinaryContext &BC,
   }
 }
 
-bool InsertNegateRAState::isUnknownBlock(BinaryContext &BC,
-                                         BinaryBasicBlock &BB) {
-  for (const MCInst &Inst : BB) {
-    if (BC.MIB->isCFI(Inst))
-      continue;
-    std::optional<bool> RAState = BC.MIB->getRAState(Inst);
-    if (RAState.has_value())
-      return false;
-  }
-  return true;
-}
-
 void InsertNegateRAState::markUnknownBlock(BinaryContext &BC,
                                            BinaryBasicBlock &BB, bool State) {
   // If we call this when an Instruction has either kRASigned or kRAUnsigned
@@ -205,7 +199,13 @@ void InsertNegateRAState::fillUnknownStubs(BinaryFunction &BF) {
   MCInst PrevInst;
   for (FunctionFragment &FF : BF.getLayout().fragments()) {
     for (BinaryBasicBlock *BB : FF) {
-      if (!FirstIter && isUnknownBlock(BC, *BB)) {
+      if (FirstIter) {
+        FirstIter = false;
+        if (isUnknownBlock(BC, *BB))
+          // If the first BasicBlock is unknown, the function's entry RAState
+          // should be used.
+          markUnknownBlock(BC, *BB, BF.getInitialRAState());
+      } else if (isUnknownBlock(BC, *BB)) {
         // As explained in issue #160989, the unwind info is incorrect for
         // stubs. Indicating the correct RAState without the rest of the unwind
         // info being correct is not useful. Instead, we copy the RAState from
@@ -224,13 +224,6 @@ void InsertNegateRAState::fillUnknownStubs(BinaryFunction &BF) {
           PrevRAState = false;
         markUnknownBlock(BC, *BB, *PrevRAState);
       }
-      if (FirstIter) {
-        FirstIter = false;
-        if (isUnknownBlock(BC, *BB))
-          // If the first BasicBlock is unknown, the function's entry RAState
-          // should be used.
-          markUnknownBlock(BC, *BB, BF.getInitialRAState());
-      }
       // This function iterates on BasicBlocks, so the PrevInst has to be
       // updated to the last instruction of the current BasicBlock. If the
       // BasicBlock is empty, or only has PseudoInstructions, PrevInst will not

bolt/unittests/Passes/CMakeLists.txt

@@ -2,7 +2,7 @@ set(LLVM_LINK_COMPONENTS
   DebugInfoDWARF
   Object
   MC
-  ${LLVM_TARGETS_TO_BUILD}
+  ${BOLT_TARGETS_TO_BUILD}
   )
 
 add_bolt_unittest(PassTests

bolt/unittests/Passes/InsertNegateRAState.cpp

@@ -216,9 +216,19 @@ TEST_P(PassTester, fillUnknownStateInBBTest) {
   Error E = PassManager->runPasses();
   EXPECT_FALSE(E);
 
+  BF->dump();
+
   auto CFILoc = findCFIOffsets(*BF);
   EXPECT_EQ(CFILoc.size(), 1u);
   EXPECT_EQ(CFILoc[0], 4);
+  // Check that the pass set Unknown and Unknown1 to signed.
+  // begin() is the CFI, begin() + 1 is Unknown, begin() + 2 is Unknown1.
+  std::optional<bool> RAState = BC->MIB->getRAState(*(BB2->begin() + 1));
+  EXPECT_TRUE(RAState.has_value());
+  EXPECT_TRUE(*RAState);
+  std::optional<bool> RAState1 = BC->MIB->getRAState(*(BB2->begin() + 2));
+  EXPECT_TRUE(RAState1.has_value());
+  EXPECT_TRUE(*RAState1);
 }
 
 TEST_P(PassTester, fillUnknownStubs) {
@@ -314,9 +324,9 @@ TEST_P(PassTester, fillUnknownStubsEmpty) {
 
   // Check that BOLT added an RAState to BB2.
   std::optional<bool> RAState = BC->MIB->getRAState(*(BB2->begin()));
-  ASSERT_TRUE(RAState.has_value());
+  EXPECT_TRUE(RAState.has_value());
   // BB2 should be set to BF.initialRAState (false).
-  ASSERT_FALSE(*RAState);
+  EXPECT_FALSE(*RAState);
 }
 
 #ifdef AARCH64_AVAILABLE