[BOLT] New CFI handling policy.

Summary:
The new interface for handling Call Frame Information:

  * CFI state at any point in a function (in CFG state) is defined by
    CFI state at basic block entry and CFI instructions inside the
    block. The state is independent of basic blocks layout order
    (this is implied by CFG state but wasn't always true in the past).
  * Use BinaryBasicBlock::getCFIStateAtInstr(const MCInst *Inst) to
    get CFI state at any given instruction in the program.
  * No need to call fixCFIState() after any given pass. fixCFIState()
    is called only once during function finalization, and any function
    transformations after that point are prohibited.
  * When introducing new basic blocks, make sure CFI state at entry
    is set correctly and matches CFI instructions in the basic block
    (if any).
  * When splitting basic blocks, use getCFIStateAtInstr() to get
    a state at the split point, and set the new basic block's CFI
    state to this value.

Introduce CFG_Finalized state to indicate that no further optimizations
are allowed on the function. This state is reached after we have synced
CFI instructions and updated EH info.

Rename "-print-after-fixup" option to "-print-finalized".

This diffs fixes CFI for cases when we split conditional tail calls,
and for indirect call promotion optimization.

(cherry picked from FBD4629307)

bolt/BinaryBasicBlock.cpp

@@ -81,7 +81,7 @@ bool BinaryBasicBlock::validateSuccessorInvariants() {
   }
   return true;
 }
-  
+
 BinaryBasicBlock *BinaryBasicBlock::getSuccessor(const MCSymbol *Label) const {
   if (!Label && succ_size() == 1)
     return *succ_begin();
@@ -103,6 +103,76 @@ BinaryBasicBlock *BinaryBasicBlock::getLandingPad(const MCSymbol *Label) const {
   return nullptr;
 }
 
+int32_t BinaryBasicBlock::getCFIStateAtInstr(const MCInst *Instr) const {
+  assert(getFunction()->getState() == BinaryFunction::State::CFG &&
+         "can only calculate CFI state when function is in active CFG state");
+
+  const auto &FDEProgram = getFunction()->getFDEProgram();
+
+  // Find the last CFI preceding Instr in this basic block.
+  const MCInst *LastCFI = nullptr;
+  bool InstrSeen = (Instr == nullptr);
+  for (auto RII = Instructions.rbegin(), E = Instructions.rend();
+       RII != E; ++RII) {
+    if (!InstrSeen) {
+      InstrSeen = (&*RII == Instr);
+      continue;
+    }
+    if (Function->getBinaryContext().MIA->isCFI(*RII)) {
+      LastCFI = &*RII;
+      break;
+    }
+  }
+
+  assert(InstrSeen && "instruction expected in basic block");
+
+  // CFI state is the same as at basic block entry point.
+  if (!LastCFI)
+    return getCFIState();
+
+  // Fold all RememberState/RestoreState sequences, such as for:
+  //
+  //   [ CFI #(K-1) ]
+  //   RememberState (#K)
+  //     ....
+  //   RestoreState
+  //   RememberState
+  //     ....
+  //   RestoreState
+  //   [ GNU_args_size ]
+  //   RememberState
+  //     ....
+  //   RestoreState   <- LastCFI
+  //
+  // we return K - the most efficient state to (re-)generate.
+  int64_t State = LastCFI->getOperand(0).getImm();
+  while (State >= 0 &&
+         FDEProgram[State].getOperation() == MCCFIInstruction::OpRestoreState) {
+    int32_t Depth = 1;
+    --State;
+    assert(State >= 0 && "first CFI cannot be RestoreState");
+    while (Depth && State >= 0) {
+      const auto &CFIInstr = FDEProgram[State];
+      if (CFIInstr.getOperation() == MCCFIInstruction::OpRestoreState) {
+        ++Depth;
+      } else if (CFIInstr.getOperation() == MCCFIInstruction::OpRememberState) {
+        --Depth;
+      }
+      --State;
+    }
+    assert(Depth == 0 && "unbalanced RememberState/RestoreState stack");
+
+    // Skip any GNU_args_size.
+    while (State >= 0 &&
+           FDEProgram[State].getOperation() == MCCFIInstruction::OpGnuArgsSize){
+      --State;
+    }
+  }
+
+  assert((State + 1 >= 0) && "miscalculated CFI state");
+  return State + 1;
+}
+
 void BinaryBasicBlock::addSuccessor(BinaryBasicBlock *Succ,
                                     uint64_t Count,
                                     uint64_t MispredictedCount) {

bolt/BinaryBasicBlock.h

@@ -95,6 +95,9 @@ private:
   /// Number of pseudo instructions in this block.
   uint32_t NumPseudos{0};
 
+  /// CFI state at the entry to this basic block.
+  int32_t CFIState{-1};
+
   /// True if this basic block is (potentially) an external entry point into
   /// the function.
   bool IsEntryPoint{false};
@@ -434,6 +437,31 @@ public:
     return RII == Instructions.rend() ? nullptr : &*RII;
   }
 
+  /// Set CFI state at entry to this basic block.
+  void setCFIState(int32_t NewCFIState) {
+    assert((CFIState == -1 || NewCFIState == CFIState) &&
+           "unexpected change of CFI state for basic block");
+    CFIState = NewCFIState;
+  }
+
+  /// Return CFI state (expected) at entry of this basic block.
+  int32_t getCFIState() const {
+    assert(CFIState >= 0 && "unknown CFI state");
+    return CFIState;
+  }
+
+  /// Calculate and return CFI state right before instruction \p Instr in
+  /// this basic block. If \p Instr is nullptr then return the state at
+  /// the end of the basic block.
+  int32_t getCFIStateAtInstr(const MCInst *Instr) const;
+
+  /// Calculate and return CFI state after execution of this basic block.
+  /// The state depends on CFI state at entry and CFI instructions inside the
+  /// basic block.
+  int32_t getCFIStateAtExit() const {
+    return getCFIStateAtInstr(nullptr);
+  }
+
   /// Set minimum alignment for the basic block.
   void setAlignment(uint64_t Align) {
     Alignment = Align;

bolt/BinaryFunction.cpp

@@ -203,7 +203,7 @@ BinaryFunction::getBasicBlockContainingOffset(uint64_t Offset) {
 
 size_t
 BinaryFunction::getBasicBlockOriginalSize(const BinaryBasicBlock *BB) const {
-  if (CurrentState != State::CFG)
+  if (!hasCFG())
     return 0;
 
   auto Index = getIndex(BB);
@@ -276,8 +276,6 @@ std::pair<unsigned, uint64_t> BinaryFunction::eraseInvalidBBs() {
   if (Count > 0) {
     updateBBIndices(0);
     recomputeLandingPads(0, BasicBlocks.size());
-    BBCFIState = annotateCFIState();
-    fixCFIState();
   }
 
   return std::make_pair(Count, Bytes);
@@ -331,7 +329,7 @@ void BinaryFunction::print(raw_ostream &OS, std::string Annotation,
      << "\n  IsSplit     : "   << IsSplit
      << "\n  BB Count    : "   << BasicBlocksLayout.size();
 
-  if (CurrentState == State::CFG) {
+  if (hasCFG()) {
     OS << "\n  Hash        : "   << Twine::utohexstr(hash());
   }
   if (FrameInstructions.size()) {
@@ -400,8 +398,8 @@ void BinaryFunction::print(raw_ostream &OS, std::string Annotation,
     if (hasValidProfile()) {
       OS << "  Exec Count : " << BBExecCount << "\n";
     }
-    if (!BBCFIState.empty()) {
+    if (BB->getCFIState() >= 0) {
-      OS << "  CFI State : " << BBCFIState[getIndex(BB)] << '\n';
+      OS << "  CFI State : " << BB->getCFIState() << '\n';
     }
     if (!BB->pred_empty()) {
       OS << "  Predecessors: ";
@@ -461,6 +459,13 @@ void BinaryFunction::print(raw_ostream &OS, std::string Annotation,
       OS << '\n';
     }
 
+    // In CFG_Finalized state we can miscalculate CFI state at exit.
+    if (CurrentState == State::CFG) {
+      const auto CFIStateAtExit = BB->getCFIStateAtExit();
+      if (CFIStateAtExit >= 0)
+        OS << "  CFI State: " << CFIStateAtExit << '\n';
+    }
+
     OS << '\n';
   }
 
@@ -1768,8 +1773,8 @@ bool BinaryFunction::buildCFG() {
   else
     clearProfile();
 
-  // Update CFI information for each BB
+  // Assign CFI information to each BB entry.
-  BBCFIState = annotateCFIState();
+  annotateCFIState();
 
   // Convert conditional tail call branches to conditional branches that jump
   // to a tail call.
@@ -1791,10 +1796,6 @@ bool BinaryFunction::buildCFG() {
     setSimple(false);
   }
 
-  // Fix the possibly corrupted CFI state. CFI state may have been corrupted
-  // because of the CFG modifications while removing conditional tail calls.
-  fixCFIState();
-
   // Clean-up memory taken by instructions and labels.
   //
   // NB: don't clear Labels list as we may need them if we mark the function
@@ -2146,7 +2147,7 @@ void BinaryFunction::removeConditionalTailCalls() {
       TailCallBB = BasicBlocks[InsertIdx];
 
       // Add the correct CFI state for the new block.
-      BBCFIState.insert(BBCFIState.begin() + InsertIdx, TCInfo.CFIStateBefore);
+      TailCallBB->setCFIState(TCInfo.CFIStateBefore);
     } else {
       // Forward jump: we will create a new basic block at the end of the
       // function containing the unconditional tail call and change the target
@@ -2158,15 +2159,15 @@ void BinaryFunction::removeConditionalTailCalls() {
       // the end of the code as a result of __builtin_unreachable().
       const BinaryBasicBlock *LastBB = BasicBlocks.back();
       uint64_t NewBlockOffset =
-        LastBB->getOffset() + BC.computeCodeSize(LastBB->begin(), LastBB->end()) + 1;
+        LastBB->getOffset()
+          + BC.computeCodeSize(LastBB->begin(), LastBB->end()) + 1;
       TailCallBB = addBasicBlock(NewBlockOffset, TCLabel);
       TailCallBB->addInstruction(TailCallInst);
 
       // Add the correct CFI state for the new block. It has to be inserted in
       // the one before last position (the last position holds the CFI state
       // after the last block).
-      BBCFIState.insert(BBCFIState.begin() + BBCFIState.size() - 1,
+      TailCallBB->setCFIState(TCInfo.CFIStateBefore);
-                        TCInfo.CFIStateBefore);
 
       // Replace the target of the conditional tail call with the label of the
       // new basic block.
@@ -2201,64 +2202,62 @@ uint64_t BinaryFunction::getFunctionScore() {
   return FunctionScore;
 }
 
-BinaryFunction::CFIStateVector
+void BinaryFunction::annotateCFIState() {
-BinaryFunction::annotateCFIState(const MCInst *Stop) {
+  assert(CurrentState == State::Disassembled && "unexpected function state");
   assert(!BasicBlocks.empty() && "basic block list should not be empty");
 
-  uint32_t State = 0;
+  // This is an index of the last processed CFI in FDE CFI program.
-  uint32_t HighestState = 0;
+  int32_t State = 0;
-  std::stack<uint32_t> StateStack;
-  CFIStateVector CFIState;
 
-  for (auto CI = BasicBlocks.begin(), CE = BasicBlocks.end(); CI != CE; ++CI) {
+  // This is an index of RememberState CFI reflecting effective state right
-    BinaryBasicBlock *CurBB = *CI;
+  // after execution of RestoreState CFI.
-    // Annotate this BB entry
+  //
-    CFIState.emplace_back(State);
+  // It differs from State iff the CFI at (State-1)
+  // was RestoreState (modulo GNU_args_size CFIs, which are ignored).
+  //
+  // This allows us to generate shorter replay sequences when producing new
+  // CFI programs.
+  int32_t EffectiveState = 0;
+
+  // For tracking RememberState/RestoreState sequences.
+  std::stack<int32_t> StateStack;
+
+  for (auto *BB : BasicBlocks) {
+    BB->setCFIState(EffectiveState);
 
     // While building the CFG, we want to save the CFI state before a tail call
-    // instruction, so that we can correctly remove condtional tail calls
+    // instruction, so that we can correctly remove conditional tail calls.
-    auto TCI = TailCallTerminatedBlocks.find(CurBB);
+    auto TCI = TailCallTerminatedBlocks.find(BB);
     bool SaveState = TCI != TailCallTerminatedBlocks.end();
 
-    // Advance state
+    uint32_t Idx = 0; // instruction index in a current basic block
-    uint32_t Idx = 0;
+    for (const auto &Instr : *BB) {
-    for (const auto &Instr : *CurBB) {
+      ++Idx;
-      auto *CFI = getCFIFor(Instr);
+      if (SaveState && Idx == TCI->second.Index) {
-      if (CFI == nullptr) {
+        TCI->second.CFIStateBefore = EffectiveState;
-        if (SaveState && Idx == TCI->second.Index)
+        SaveState = false;
-          TCI->second.CFIStateBefore = State;
-        ++Idx;
-        if (&Instr == Stop) {
-          CFIState.emplace_back(State);
-          return CFIState;
-        }
-        continue;
       }
-      ++HighestState;
+
+      const auto *CFI = getCFIFor(Instr);
+      if (!CFI)
+        continue;
+
+      ++State;
+
       if (CFI->getOperation() == MCCFIInstruction::OpRememberState) {
-        StateStack.push(State);
+        StateStack.push(EffectiveState);
       } else if (CFI->getOperation() == MCCFIInstruction::OpRestoreState) {
-        assert(!StateStack.empty() && "Corrupt CFI stack");
+        assert(!StateStack.empty() && "corrupt CFI stack");
-        State = StateStack.top();
+        EffectiveState = StateStack.top();
         StateStack.pop();
       } else if (CFI->getOperation() != MCCFIInstruction::OpGnuArgsSize) {
-        State = HighestState;
+        // OpGnuArgsSize CFIs do not affect the CFI state.
-      }
+        EffectiveState = State;
-      assert(State <= FrameInstructions.size());
-      ++Idx;
-      if (&Instr == Stop) {
-        CFIState.emplace_back(State);
-        return CFIState;
       }
     }
   }
 
-  // Store the state after the last BB
+  assert(StateStack.empty() && "corrupt CFI stack");
-  CFIState.emplace_back(State);
-
-  assert(StateStack.empty() && "Corrupt CFI stack");
-
-  return CFIState;
 }
 
 bool BinaryFunction::fixCFIState() {
@@ -2268,74 +2267,72 @@ bool BinaryFunction::fixCFIState() {
                << ": ");
 
   auto replayCFIInstrs =
-      [this](uint32_t FromState, uint32_t ToState, BinaryBasicBlock *InBB,
+      [this](int32_t FromState, int32_t ToState, BinaryBasicBlock *InBB,
-             BinaryBasicBlock::iterator InsertIt) -> bool {
+         BinaryBasicBlock::iterator InsertIt) -> bool {
-        if (FromState == ToState)
+    if (FromState == ToState)
-          return true;
+      return true;
-        assert(FromState < ToState);
+    assert(FromState < ToState && "can only replay CFIs forward");
 
-        std::vector<uint32_t> NewCFIs;
+    std::vector<uint32_t> NewCFIs;
-        uint32_t NestedLevel = 0;
+    uint32_t NestedLevel = 0;
-        for (uint32_t CurState = FromState; CurState < ToState; ++CurState) {
+    for (auto CurState = FromState; CurState < ToState; ++CurState) {
-          assert(CurState < FrameInstructions.size());
+      MCCFIInstruction *Instr = &FrameInstructions[CurState];
-          MCCFIInstruction *Instr = &FrameInstructions[CurState];
+      if (Instr->getOperation() == MCCFIInstruction::OpRememberState)
-          if (Instr->getOperation() == MCCFIInstruction::OpRememberState)
+        ++NestedLevel;
-            ++NestedLevel;
+      if (!NestedLevel)
-          if (!NestedLevel)
+        NewCFIs.push_back(CurState);
-            NewCFIs.push_back(CurState);
+      if (Instr->getOperation() == MCCFIInstruction::OpRestoreState)
-          if (Instr->getOperation() == MCCFIInstruction::OpRestoreState)
+        --NestedLevel;
-            --NestedLevel;
+    }
-        }
 
-        // TODO: If in replaying the CFI instructions to reach this state we
+    // TODO: If in replaying the CFI instructions to reach this state we
-        // have state stack instructions, we could still work out the logic
+    // have state stack instructions, we could still work out the logic
-        // to extract only the necessary instructions to reach this state
+    // to extract only the necessary instructions to reach this state
-        // without using the state stack. Not sure if it is worth the effort
+    // without using the state stack. Not sure if it is worth the effort
-        // because this happens rarely.
+    // because this happens rarely.
-        if (NestedLevel != 0) {
+    if (NestedLevel != 0) {
-          if (opts::Verbosity >= 1) {
+      errs() << "BOLT-WARNING: CFI rewriter detected nested CFI state"
-            errs() << "BOLT-WARNING: CFI rewriter detected nested CFI state"
+             << " while replaying CFI instructions for BB "
-                   << " while replaying CFI instructions for BB "
+             << InBB->getName() << " in function " << *this << '\n';
-                   << InBB->getName() << " in function " << *this << '\n';
+      return false;
-          }
+    }
-          return false;
-        }
 
-        for (auto CFI : NewCFIs) {
+    for (auto CFI : NewCFIs) {
-          // Ignore GNU_args_size instructions.
+      // Ignore GNU_args_size instructions.
-          if (FrameInstructions[CFI].getOperation() !=
+      if (FrameInstructions[CFI].getOperation() !=
-              MCCFIInstruction::OpGnuArgsSize) {
+          MCCFIInstruction::OpGnuArgsSize) {
-            InsertIt = addCFIPseudo(InBB, InsertIt, CFI);
+        InsertIt = addCFIPseudo(InBB, InsertIt, CFI);
-            ++InsertIt;
+        ++InsertIt;
-          }
+      }
-        }
+    }
 
-        return true;
+    return true;
-      };
+  };
 
-  uint32_t State = 0;
+  int32_t State = 0;
   auto *FDEStartBB = BasicBlocksLayout[0];
-  for (uint32_t I = 0, E = BasicBlocksLayout.size(); I != E; ++I) {
+  bool SeenCold = false;
-    auto *BB = BasicBlocksLayout[I];
+  for (auto *BB : BasicBlocksLayout) {
-    uint32_t BBIndex = getIndex(BB);
+    const auto CFIStateAtExit = BB->getCFIStateAtExit();
 
     // Hot-cold border: check if this is the first BB to be allocated in a cold
-    // region (a different FDE). If yes, we need to reset the CFI state and
+    // region (with a different FDE). If yes, we need to reset the CFI state and
-    // the FDEStartBB that is used to insert remember_state CFIs (t12863876).
+    // the FDEStartBB that is used to insert remember_state CFIs.
-    if (I != 0 && BB->isCold() != BasicBlocksLayout[I - 1]->isCold()) {
+    if (!SeenCold && BB->isCold()) {
       State = 0;
       FDEStartBB = BB;
+      SeenCold = true;
     }
 
     // We need to recover the correct state if it doesn't match expected
     // state at BB entry point.
-    if (BBCFIState[BBIndex] < State) {
+    if (BB->getCFIState() < State) {
       // In this case, State is currently higher than what this BB expect it
       // to be. To solve this, we need to insert a CFI instruction to remember
       // the old state at function entry, then another CFI instruction to
       // restore it at the entry of this BB and replay CFI instructions to
       // reach the desired state.
-      uint32_t OldState = BBCFIState[BBIndex];
+      int32_t OldState = BB->getCFIState();
       // Remember state at function entry point (our reference state).
       auto InsertIt = FDEStartBB->begin();
       while (InsertIt != FDEStartBB->end() && BC.MIA->isCFI(*InsertIt))
@@ -2375,25 +2372,22 @@ bool BinaryFunction::fixCFIState() {
       }
 
       if (StackOffset != 0) {
-        if (opts::Verbosity >= 1) {
+        errs() << "BOLT-WARNING: not possible to remember/recover state"
-          errs() << "BOLT-WARNING: not possible to remember/recover state"
+               << " without corrupting CFI state stack in function "
-                 << " without corrupting CFI state stack in function "
+               << *this << " @ " << BB->getName() << "\n";
-                 << *this << " @ " << BB->getName() << "\n";
-        }
         return false;
       }
-    } else if (BBCFIState[BBIndex] > State) {
+    } else if (BB->getCFIState() > State) {
-      // If BBCFIState[BBIndex] > State, it means we are behind in the
+      // If BB's CFI state is greater than State, it means we are behind in the
       // state. Just emit all instructions to reach this state at the
       // beginning of this BB. If this sequence of instructions involve
       // remember state or restore state, bail out.
-      if (!replayCFIInstrs(State, BBCFIState[BBIndex], BB, BB->begin()))
+      if (!replayCFIInstrs(State, BB->getCFIState(), BB, BB->begin()))
         return false;
     }
 
-    State = BBCFIState[BBIndex + 1];
+    State = CFIStateAtExit;
-    DEBUG(dbgs() << Sep << State);
+    DEBUG(dbgs() << Sep << State; Sep = ", ");
-    DEBUG(Sep = ", ");
   }
   DEBUG(dbgs() << "\n");
   return true;
@@ -2543,9 +2537,6 @@ void BinaryFunction::emitBody(MCStreamer &Streamer, bool EmitColdPart) {
       // Emit GNU_args_size CFIs as necessary.
       if (usesGnuArgsSize() && BC.MIA->isInvoke(Instr)) {
         auto NewGnuArgsSize = BC.MIA->getGnuArgsSize(Instr);
-        if (NewGnuArgsSize < 0) {
-          errs() << "XXX: in function " << *this << '\n';
-        }
         assert(NewGnuArgsSize >= 0 && "expected non-negative GNU_args_size");
         if (NewGnuArgsSize != CurrentGnuArgsSize) {
           CurrentGnuArgsSize = NewGnuArgsSize;
@@ -2688,7 +2679,7 @@ void BinaryFunction::dumpGraph(raw_ostream& OS) const {
                  BB->getOffset(),
                  getIndex(BB),
                  Layout,
-                 BBCFIState[getIndex(BB)]);
+                 BB->getCFIState());
     OS << format("\"%s\" [shape=box]\n", BB->getName().data());
     if (opts::DotToolTipCode) {
       std::string Str;
@@ -3095,7 +3086,7 @@ __attribute__((noinline)) BinaryFunction::BasicBlockOrderType BinaryFunction::df
 bool BinaryFunction::isIdenticalWith(const BinaryFunction &OtherBF,
                                      bool IgnoreSymbols,
                                      bool UseDFS) const {
-  assert(CurrentState == State::CFG && OtherBF.CurrentState == State::CFG);
+  assert(hasCFG() && OtherBF.hasCFG() && "both functions should have CFG");
 
   // Compare the two functions, one basic block at a time.
   // Currently we require two identical basic blocks to have identical
@@ -3261,7 +3252,7 @@ bool BinaryFunction::equalJumpTables(const JumpTable *JumpTableA,
 }
 
 std::size_t BinaryFunction::hash(bool Recompute, bool UseDFS) const {
-  assert(CurrentState == State::CFG);
+  assert(hasCFG() && "function is expected to have CFG");
 
   if (!Recompute)
     return Hash;
@@ -3343,13 +3334,11 @@ void BinaryFunction::updateBBIndices(const unsigned StartIndex) {
 void BinaryFunction::updateCFIState(BinaryBasicBlock *Start,
                                     const unsigned NumNewBlocks) {
   assert(TailCallTerminatedBlocks.empty());
-  auto PartialCFIState = annotateCFIState(&(*Start->rbegin()));
+  const auto CFIState = Start->getCFIStateAtExit();
-  const auto StartIndex = getIndex(Start);
+  const auto StartIndex = getIndex(Start) + 1;
-  BBCFIState.insert(BBCFIState.begin() + StartIndex + 1,
+  for (unsigned I = 0; I < NumNewBlocks; ++I) {
-                    NumNewBlocks,
+    BasicBlocks[StartIndex + I]->setCFIState(CFIState);
-                    PartialCFIState.back());
+  }
-  assert(BBCFIState.size() == BasicBlocks.size() + 1);
-  fixCFIState();
 }
 
 void BinaryFunction::updateLayout(BinaryBasicBlock* Start,

bolt/BinaryFunction.h

@@ -159,10 +159,11 @@ enum JumpTableSupportLevel : char {
 class BinaryFunction : public AddressRangesOwner {
 public:
   enum class State : char {
-    Empty = 0,        /// Function body is empty
+    Empty = 0,        /// Function body is empty.
-    Disassembled,     /// Function have been disassembled
+    Disassembled,     /// Function have been disassembled.
-    CFG,              /// Control flow graph have been built
+    CFG,              /// Control flow graph have been built.
-    Assembled,        /// Function has been assembled in memory
+    CFG_Finalized,    /// CFG is finalized. No optimizations allowed.
+    Assembled,        /// Function has been assembled in memory.
   };
 
   /// Settings for splitting function bodies into hot/cold partitions.
@@ -336,6 +337,11 @@ private:
   /// Update the indices of all the basic blocks starting at StartIndex.
   void updateBBIndices(const unsigned StartIndex);
 
+  /// Annotate each basic block entry with its current CFI state. This is
+  /// run right after the construction of CFG while basic blocks are in their
+  /// original order.
+  void annotateCFIState();
+
   /// Helper function that compares an instruction of this function to the
   /// given instruction of the given function. The functions should have
   /// identical CFG.
@@ -450,6 +456,28 @@ private:
   using CFIInstrMapType = std::vector<MCCFIInstruction>;
   using cfi_iterator       = CFIInstrMapType::iterator;
   using const_cfi_iterator = CFIInstrMapType::const_iterator;
+
+  /// We don't decode Call Frame Info encoded in DWARF program state
+  /// machine. Instead we define a "CFI State" - a frame information that
+  /// is a result of executing FDE CFI program up to a given point. The
+  /// program consists of opaque Call Frame Instructions:
+  ///
+  ///   CFI #0
+  ///   CFI #1
+  ///   ....
+  ///   CFI #N
+  ///
+  /// When we refer to "CFI State K" - it corresponds to a row in an abstract
+  /// Call Frame Info table. This row is reached right before executing CFI #K.
+  ///
+  /// At any point of execution in a function we are in any one of (N + 2)
+  /// states described in the original FDE program. We can't have more states
+  /// without intelligent processing of CFIs.
+  ///
+  /// When the final layout of basic blocks is known, and we finalize CFG,
+  /// we modify the original program to make sure the same state could be
+  /// reached even when basic blocks containing CFI instructions are executed
+  /// in a different order.
   CFIInstrMapType FrameInstructions;
 
   /// Exception handling ranges.
@@ -615,13 +643,6 @@ private:
   };
   std::vector<BasicBlockOffset> BasicBlockOffsets;
 
-  // At each basic block entry we attach a CFI state to detect if reordering
-  // corrupts the CFI state for a block. The CFI state is simply the index in
-  // FrameInstructions for the CFI responsible for creating this state.
-  // This vector is indexed by BB index.
-  using CFIStateVector = std::vector<uint32_t>;
-  CFIStateVector BBCFIState;
-
   /// Symbol in the output.
   ///
   /// NB: function can have multiple symbols associated with it. We will emit
@@ -895,10 +916,18 @@ public:
     return Names;
   }
 
+  /// Return a state the function is in (see BinaryFunction::State definition
+  /// for description).
   State getState() const {
     return CurrentState;
   }
 
+  /// Return true if function has a control flow graph available.
+  bool hasCFG() const {
+    return getState() == State::CFG ||
+           getState() == State::CFG_Finalized;
+  }
+
   /// Return containing file section.
   SectionRef getSection() const {
     return Section;
@@ -1511,14 +1540,9 @@ public:
   /// and size.
   uint64_t getFunctionScore();
 
-  /// Annotate each basic block entry with its current CFI state. This is used
+  const CFIInstrMapType &getFDEProgram() const {
-  /// to detect when reordering changes the CFI state seen by a basic block and
+    return FrameInstructions;
-  /// fix this.
+  }
-  /// The CFI state is simply the index in FrameInstructions for the
-  /// MCCFIInstruction object responsible for this state.
-  /// If Stop is not null, the annotation will exit early once the scan finishes
-  /// with the Stop instruction.
-  CFIStateVector annotateCFIState(const MCInst *Stop = nullptr);
 
   /// After reordering, this function checks the state of CFI and fixes it if it
   /// is corrupted. If it is unable to fix it, it returns false.
@@ -1545,6 +1569,11 @@ public:
   /// When we reverse the branch condition, the CFG is updated accordingly.
   void fixBranches();
 
+  /// Mark function as finalized. No further optimizations are permitted.
+  void setFinalized() {
+    CurrentState = State::CFG_Finalized;
+  }
+
   /// Split function in two: a part with warm or hot BBs and a part with never
   /// executed BBs. The cold part is moved to a new BinaryFunction.
   void splitFunction();
@@ -1712,6 +1741,7 @@ inline raw_ostream &operator<<(raw_ostream &OS,
   case BinaryFunction::State::Empty:        OS << "empty";  break;
   case BinaryFunction::State::Disassembled: OS << "disassembled";  break;
   case BinaryFunction::State::CFG:          OS << "CFG constructed";  break;
+  case BinaryFunction::State::CFG_Finalized:OS << "CFG finalized";  break;
   case BinaryFunction::State::Assembled:    OS << "assembled";  break;
   }
 

bolt/BinaryPassManager.cpp

@@ -95,8 +95,8 @@ PrintAfterBranchFixup("print-after-branch-fixup",
                       cl::Hidden);
 
 static cl::opt<bool>
-PrintAfterFixup("print-after-fixup",
+PrintFinalized("print-finalized",
-                cl::desc("print function after fixup"),
+                cl::desc("print function after CFG is finalized"),
                 cl::Hidden);
 
 static cl::opt<bool>
@@ -140,7 +140,7 @@ PrintICP("print-icp",
          cl::desc("print functions after indirect call promotion"),
          cl::ZeroOrMore,
          cl::Hidden);
-  
+
 static cl::opt<bool>
 PrintInline("print-inline",
             cl::desc("print functions after inlining optimization"),
@@ -283,7 +283,7 @@ void BinaryFunctionPassManager::runAllPasses(
     llvm::make_unique<EliminateUnreachableBlocks>(PrintUCE),
     opts::EliminateUnreachable);
 
-  Manager.registerPass(llvm::make_unique<FixupFunctions>(PrintAfterFixup));
+  Manager.registerPass(llvm::make_unique<FinalizeFunctions>(PrintFinalized));
 
   Manager.registerPass(
     llvm::make_unique<InstructionLowering>(PrintAfterLowering));

bolt/Exceptions.cpp

@@ -302,7 +302,7 @@ void BinaryFunction::updateEHRanges() {
   if (getSize() == 0)
     return;
 
-  assert(CurrentState == State::CFG && "unexpected state");
+  assert(CurrentState == State::CFG_Finalized && "unexpected state");
 
   // Build call sites table.
   struct EHInfo {

bolt/Passes/BinaryPasses.cpp

@@ -378,7 +378,7 @@ void FixupBranches::runOnFunctions(
   }
 }
 
-void FixupFunctions::runOnFunctions(
+void FinalizeFunctions::runOnFunctions(
   BinaryContext &BC,
   std::map<uint64_t, BinaryFunction> &BFs,
   std::set<uint64_t> &
@@ -394,17 +394,15 @@ void FixupFunctions::runOnFunctions(
     if (shouldOptimize(Function) && !Function.fixCFIState()) {
       if (opts::Relocs) {
         errs() << "BOLT-ERROR: unable to fix CFI state for function "
-               << Function << ". Aborting.\n";
+               << Function << ". Exiting.\n";
-        abort();
+        exit(1);
-      }
-      if (opts::Verbosity >= 1) {
-        errs() << "BOLT-WARNING: unable to fix CFI state for function "
-               << Function << ". Skipping.\n";
       }
       Function.setSimple(false);
       continue;
     }
 
+    Function.setFinalized();
+
     // Update exception handling information.
     Function.updateEHRanges();
   }
@@ -592,7 +590,7 @@ void Peepholes::shortenInstructions(BinaryContext &BC,
 void debugDump(BinaryFunction *BF) {
   BF->dump();
 }
-  
+
 // This peephole fixes jump instructions that jump to another basic
 // block with a single jump instruction, e.g.
 //
@@ -1396,7 +1394,7 @@ IndirectCallPromotion::printCallsiteInfo(const BinaryBasicBlock *BB,
     BC.printInstruction(dbgs(), Inst, Targets[0].From.Offset, nullptr, true);
   });
 }
-  
+
 void IndirectCallPromotion::runOnFunctions(
   BinaryContext &BC,
   std::map<uint64_t, BinaryFunction> &BFs,

bolt/Passes/BinaryPasses.h

@@ -139,13 +139,13 @@ class FixupBranches : public BinaryFunctionPass {
 
 /// Fix the CFI state and exception handling information after all other
 /// passes have completed.
-class FixupFunctions : public BinaryFunctionPass {
+class FinalizeFunctions : public BinaryFunctionPass {
  public:
-  explicit FixupFunctions(const cl::opt<bool> &PrintPass)
+  explicit FinalizeFunctions(const cl::opt<bool> &PrintPass)
     : BinaryFunctionPass(PrintPass) { }
 
   const char *getName() const override {
-    return "fixup-functions";
+    return "finalize-functions";
   }
   void runOnFunctions(BinaryContext &BC,
                       std::map<uint64_t, BinaryFunction> &BFs,

bolt/RewriteInstance.cpp

@@ -2751,7 +2751,6 @@ void RewriteInstance::patchELFSectionHeaderTable(ELFObjectFile<ELFT> *File) {
 
   // New section header string table goes last.
 
-
   // Fix ELF header.
   auto NewEhdr = *Obj->getHeader();
   NewEhdr.e_entry = EntryPoint;