Store only the last row in the state

Amirhossein Pashaeehir · Amirhossein Pashaeehir · commit 8a87530111b3 · 2025-07-01T21:01:18.000Z
diff --git a/llvm/include/llvm/DWARFCFIChecker/DWARFCFIAnalysis.h b/llvm/include/llvm/DWARFCFIChecker/DWARFCFIAnalysis.h
@@ -74,13 +74,13 @@ class DWARFCFIAnalysis {
 
 private:
   void checkRegDiff(const MCInst &Inst, DWARFRegNum Reg,
-                    const dwarf::UnwindRow *PrevRow,
-                    const dwarf::UnwindRow *NextRow,
+                    const dwarf::UnwindRow &PrevRow,
+                    const dwarf::UnwindRow &NextRow,
                     const SmallSet<DWARFRegNum, 4> &Reads,
                     const SmallSet<DWARFRegNum, 4> &Writes);
 
-  void checkCFADiff(const MCInst &Inst, const dwarf::UnwindRow *PrevRow,
-                    const dwarf::UnwindRow *NextRow,
+  void checkCFADiff(const MCInst &Inst, const dwarf::UnwindRow &PrevRow,
+                    const dwarf::UnwindRow &NextRow,
                     const SmallSet<DWARFRegNum, 4> &Reads,
                     const SmallSet<DWARFRegNum, 4> &Writes);
 
diff --git a/llvm/include/llvm/DWARFCFIChecker/DWARFCFIState.h b/llvm/include/llvm/DWARFCFIChecker/DWARFCFIState.h
@@ -23,23 +23,24 @@ namespace llvm {
 
 using DWARFRegNum = uint32_t;
 
-/// This class is used to maintain a CFI state and history, referred to as an
-/// unwinding table, during CFI analysis. The table is private, meaning the only
-/// way to modify it is to append a new row by updating it with a CFI directive,
-/// and the only way to read from it is to read the last row (i.e., current CFI
-/// state) from the table. The fetched row is constant and should not be
-/// modified or deleted.
+/// This class is used to maintain a CFI state, referred to as an unwinding row,
+/// during CFI analysis. The only way to modify the state is by updating it with
+/// a CFI directive.
 class DWARFCFIState {
 public:
-  DWARFCFIState(MCContext *Context) : Context(Context) {};
-  ~DWARFCFIState();
+  DWARFCFIState(MCContext *Context) : Context(Context), IsInitiated(false) {};
 
-  std::optional<const dwarf::UnwindRow *> getCurrentUnwindRow() const;
+  std::optional<dwarf::UnwindRow> getCurrentUnwindRow() const;
+  /// This method updates the state by applying \p Directive to the current
+  /// state. If the directive is not supported by the checker or any error
+  /// happens while applying the CFI directive, a warning or error is reported
+  /// to the user, and the directive is ignored, leaving the state unchanged.
   void update(const MCCFIInstruction &Directive);
 
 private:
+  dwarf::UnwindRow Row;
   MCContext *Context;
-  std::vector<dwarf::UnwindRow *> Table;
+  bool IsInitiated;
 
   dwarf::CFIProgram convert(MCCFIInstruction Directive);
 };
diff --git a/llvm/lib/DWARFCFIChecker/DWARFCFIAnalysis.cpp b/llvm/lib/DWARFCFIChecker/DWARFCFIAnalysis.cpp
@@ -44,8 +44,8 @@ struct CFARegOffsetInfo {
 };
 
 static std::optional<CFARegOffsetInfo>
-getCFARegOffsetInfo(const dwarf::UnwindRow *UnwindRow) {
-  auto CFALocation = UnwindRow->getCFAValue();
+getCFARegOffsetInfo(const dwarf::UnwindRow &UnwindRow) {
+  auto CFALocation = UnwindRow.getCFAValue();
   if (CFALocation.getLocation() !=
       dwarf::UnwindLocation::Location::RegPlusOffset) {
     return std::nullopt;
@@ -55,8 +55,8 @@ getCFARegOffsetInfo(const dwarf::UnwindRow *UnwindRow) {
 }
 
 static SmallSet<DWARFRegNum, 4>
-getUnwindRuleRegSet(const dwarf::UnwindRow *UnwindRow, DWARFRegNum Reg) {
-  auto MaybeLoc = UnwindRow->getRegisterLocations().getRegisterLocation(Reg);
+getUnwindRuleRegSet(const dwarf::UnwindRow &UnwindRow, DWARFRegNum Reg) {
+  auto MaybeLoc = UnwindRow.getRegisterLocations().getRegisterLocation(Reg);
   assert(MaybeLoc && "The register should be included in the unwinding row");
   auto Loc = *MaybeLoc;
 
@@ -131,7 +131,7 @@ void DWARFCFIAnalysis::update(const MCInst &Inst,
   auto MaybePrevRow = State.getCurrentUnwindRow();
   assert(MaybePrevRow && "The analysis should have initialized the "
                          "history with at least one row by now");
-  const dwarf::UnwindRow *PrevRow = MaybePrevRow.value();
+  auto PrevRow = *MaybePrevRow;
 
   for (auto &&Directive : Directives)
     State.update(Directive);
@@ -158,7 +158,7 @@ void DWARFCFIAnalysis::update(const MCInst &Inst,
 
   auto MaybeNextRow = State.getCurrentUnwindRow();
   assert(MaybeNextRow && "Prev row existed, so should the current row.");
-  const dwarf::UnwindRow *NextRow = *MaybeNextRow;
+  auto NextRow = *MaybeNextRow;
 
   checkCFADiff(Inst, PrevRow, NextRow, Reads, Writes);
 
@@ -170,12 +170,12 @@ void DWARFCFIAnalysis::update(const MCInst &Inst,
 }
 
 void DWARFCFIAnalysis::checkRegDiff(const MCInst &Inst, DWARFRegNum Reg,
-                                    const dwarf::UnwindRow *PrevRow,
-                                    const dwarf::UnwindRow *NextRow,
+                                    const dwarf::UnwindRow &PrevRow,
+                                    const dwarf::UnwindRow &NextRow,
                                     const SmallSet<DWARFRegNum, 4> &Reads,
                                     const SmallSet<DWARFRegNum, 4> &Writes) {
-  auto MaybePrevLoc = PrevRow->getRegisterLocations().getRegisterLocation(Reg);
-  auto MaybeNextLoc = NextRow->getRegisterLocations().getRegisterLocation(Reg);
+  auto MaybePrevLoc = PrevRow.getRegisterLocations().getRegisterLocation(Reg);
+  auto MaybeNextLoc = NextRow.getRegisterLocations().getRegisterLocation(Reg);
 
   // All the tracked registers are added during initiation. So if a register is
   // not added, should stay the same during execution and vice versa.
@@ -256,8 +256,8 @@ void DWARFCFIAnalysis::checkRegDiff(const MCInst &Inst, DWARFRegNum Reg,
 }
 
 void DWARFCFIAnalysis::checkCFADiff(const MCInst &Inst,
-                                    const dwarf::UnwindRow *PrevRow,
-                                    const dwarf::UnwindRow *NextRow,
+                                    const dwarf::UnwindRow &PrevRow,
+                                    const dwarf::UnwindRow &NextRow,
                                     const SmallSet<DWARFRegNum, 4> &Reads,
                                     const SmallSet<DWARFRegNum, 4> &Writes) {
 
diff --git a/llvm/lib/DWARFCFIChecker/DWARFCFIState.cpp b/llvm/lib/DWARFCFIChecker/DWARFCFIState.cpp
@@ -18,38 +18,26 @@
 
 using namespace llvm;
 
-DWARFCFIState::~DWARFCFIState() {
-  for (auto &&Row : Table)
-    delete Row;
-}
-
-std::optional<const dwarf::UnwindRow *>
-DWARFCFIState::getCurrentUnwindRow() const {
-  if (!Table.size())
+std::optional<dwarf::UnwindRow> DWARFCFIState::getCurrentUnwindRow() const {
+  if (!IsInitiated)
     return std::nullopt;
-
-  return *(--Table.end());
+  return Row;
 }
 
 void DWARFCFIState::update(const MCCFIInstruction &Directive) {
   auto CFIP = convert(Directive);
 
-  auto MaybeCurrentRow = getCurrentUnwindRow();
-
-  // This is a copy of the last row of the table (or a new empty row), its value
-  // will be updated by `parseRows`.
-  dwarf::UnwindRow *NewRow = nullptr;
-  if (MaybeCurrentRow)
-    NewRow = new dwarf::UnwindRow(*(MaybeCurrentRow.value()));
-  else
-    NewRow = new dwarf::UnwindRow();
+  // This is a copy of the current row, its value will be updated by
+  // `parseRows`.
+  dwarf::UnwindRow NewRow = Row;
 
   // `parseRows` updates the current row by applying the `CFIProgram` to it.
-  // During this process, it may create multiple rows that should be placed in
-  // the unwinding table, preceding the newly updated row and following the
-  // previous rows. These middle rows are stored in `PrecedingRows`.
+  // During this process, it may create multiple rows preceding the newly
+  // updated row and following the previous rows. These middle rows are stored
+  // in `PrecedingRows`. For now, there is no need to store these rows in the
+  // state, so they are ignored in the end.
   dwarf::UnwindTable::RowContainer PrecedingRows;
-  if (Error Err = parseRows(CFIP, *NewRow, nullptr).moveInto(PrecedingRows)) {
+  if (Error Err = parseRows(CFIP, NewRow, nullptr).takeError()) {
     Context->reportError(
         Directive.getLoc(),
         formatv("could not parse this CFI directive due to: {0}",
@@ -59,9 +47,8 @@ void DWARFCFIState::update(const MCCFIInstruction &Directive) {
     return;
   }
 
-  for (auto &&PrecedingRow : PrecedingRows)
-    Table.push_back(new dwarf::UnwindRow(PrecedingRow));
-  Table.push_back(NewRow);
+  Row = NewRow;
+  IsInitiated = true;
 }
 
 dwarf::CFIProgram DWARFCFIState::convert(MCCFIInstruction Directive) {
@@ -101,20 +88,18 @@ dwarf::CFIProgram DWARFCFIState::convert(MCCFIInstruction Directive) {
     break;
   case MCCFIInstruction::OpRelOffset:
     assert(
-        MaybeCurrentRow &&
+        IsInitiated &&
         "Cannot define relative offset to a non-existing CFA unwinding rule");
 
     CFIP.addInstruction(dwarf::DW_CFA_offset, Directive.getRegister(),
-                        Directive.getOffset() -
-                            (*MaybeCurrentRow)->getCFAValue().getOffset());
+                        Directive.getOffset() - Row.getCFAValue().getOffset());
     break;
   case MCCFIInstruction::OpAdjustCfaOffset:
-    assert(MaybeCurrentRow &&
+    assert(IsInitiated &&
            "Cannot adjust CFA offset of a non-existing CFA unwinding rule");
 
     CFIP.addInstruction(dwarf::DW_CFA_def_cfa_offset,
-                        Directive.getOffset() +
-                            (*MaybeCurrentRow)->getCFAValue().getOffset());
+                        Directive.getOffset() + Row.getCFAValue().getOffset());
     break;
   case MCCFIInstruction::OpEscape:
     // TODO: DWARFExpressions are not supported yet, ignoring expression here.
