[DWARF] Store CFA value on DW_CFA_remember_state

[aengelke]

Summary

Previously, CFA_remember_state stored only the register locations but ignored the CFA value. This needs also to be remembered and restored for correct behavior. The problem occurs, e.g., on functions with multiple epilogues, where the CFA value after the first epilogue is becomes wrong.

[aengelke]

Ping

[MaskRay]

Looks good. The behavior matches libunwind and libgcc.

[MaskRay]

DWARF v5 says:

The DW_CFA_remember_state instruction takes no operands. The required action is to push the set of rules for every register onto an implicit stack.

I think it should mention that "the current CFA rule" is saved as well.

Filed an issue on https://dwarfstd.org/Issues.php , waiting for a link.

[aengelke]

Thanks! I have no commit access; could you commit this for me (Alexis Engelke <[e-mail]>)?

[MaskRay]

In D140338#4023802, @aengelke wrote:

Thanks! I have no commit access; could you commit this for me (Alexis Engelke <[e-mail]>)?

I will wait a bit for further feedback and then push this on your behalf (likely tomorrow) :)

[MaskRay]

In D140338#4023798, @MaskRay wrote:

DWARF v5 says:

The DW_CFA_remember_state instruction takes no operands. The required action is to push the set of rules for every register onto an implicit stack.

I think it should mention that "the current CFA rule" is saved as well.

Filed an issue on https://dwarfstd.org/Issues.php , waiting for a link.

Got a link: https://dwarfstd.org/ShowIssue.php?issue=230103.1

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

case OT_UnsignedFactDataOffset: {
     return Operand * DataAlignmentFactor;
   }
   }
   llvm_unreachable("invalid operand type");
 }
 
 Error UnwindTable::parseRows(const CFIProgram &CFIP, UnwindRow &Row,
                              const RegisterLocations *InitialLocs) {
-  std::vector<RegisterLocations> RegisterStates;
+  // State consists of CFA value and register locations.
+  std::vector<std::pair<UnwindLocation, RegisterLocations>> States;
   for (const CFIProgram::Instruction &Inst : CFIP) {
     switch (Inst.Opcode) {
     case dwarf::DW_CFA_set_loc: {
       // The DW_CFA_set_loc instruction takes a single operand that
       // represents a target address. The required action is to create a new
       // table row using the specified address as the location. All other
       // values in the new row are initially identical to the current row.
       // The new location value is always greater than the current one. If
case dwarf::DW_CFA_offset_extended_sf: {
           *RegNum, UnwindLocation::createAtCFAPlusOffset(*Offset));
       break;
     }
 
     case dwarf::DW_CFA_nop:
       break;
 
     case dwarf::DW_CFA_remember_state:
-      RegisterStates.push_back(Row.getRegisterLocations());
+      States.push_back(
+          std::make_pair(Row.getCFAValue(), Row.getRegisterLocations()));
       break;
 
     case dwarf::DW_CFA_restore_state:
-      if (RegisterStates.empty())
+      if (States.empty())
         return createStringError(errc::invalid_argument,
                                  "DW_CFA_restore_state without a matching "
                                  "previous DW_CFA_remember_state");
-      Row.getRegisterLocations() = RegisterStates.back();
-      RegisterStates.pop_back();
+      Row.getCFAValue() = States.back().first;
+      Row.getRegisterLocations() = States.back().second;
+      States.pop_back();
       break;
 
     case dwarf::DW_CFA_GNU_window_save:
       switch (CFIP.triple()) {
       case Triple::aarch64:
       case Triple::aarch64_be:
       case Triple::aarch64_32: {
         // DW_CFA_GNU_window_save is used for different things on different

llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp

dwarf::FDE TestFDE(/*IsDWARF64=*/true,
                      /*InitialLocation=*/0x1000,
                      /*AddressRange=*/0x1000,
                      /*Cie=*/&TestCIE,
                      /*LSDAAddress=*/std::nullopt,
                      /*Arch=*/Triple::x86_64);
 
   // Make a CIE that has a valid CFA definition and a single register unwind
   // rule for register that we will verify is in all of the pushed rows.
-  EXPECT_THAT_ERROR(parseCFI(TestCIE, {dwarf::DW_CFA_def_cfa, 12, 32}),
+  constexpr uint8_t CFAOff1 = 32;
+  constexpr uint8_t CFAOff2 = 16;
+  constexpr uint8_t Reg1 = 14;
+  constexpr uint8_t Reg2 = 15;
+  constexpr uint8_t Reg3 = 16;
+
+  EXPECT_THAT_ERROR(parseCFI(TestCIE, {dwarf::DW_CFA_def_cfa, 12, CFAOff1}),
                     Succeeded());
 
   // Make a FDE with DWARF call frame instruction opcodes that encodes the
   // follwing rows:
-  // 0x1000: CFA=reg12+32: Reg1=[CFA-8]
-  // 0x1004: CFA=reg12+32: Reg1=[CFA-8] Reg2=[CFA-16]
-  // 0x1008: CFA=reg12+32: Reg1=[CFA-8] Reg2=[CFA-16] Reg3=[CFA-24]
-  // 0x100C: CFA=reg12+32: Reg1=[CFA-8] Reg2=[CFA-16]
-  // 0x1010: CFA=reg12+32: Reg1=[CFA-8]
+  // 0x1000: CFA=reg12+CFAOff1: Reg1=[CFA-8]
+  // 0x1004: CFA=reg12+CFAOff1: Reg1=[CFA-8] Reg2=[CFA-16]
+  // 0x1008: CFA=reg12+CFAOff2: Reg1=[CFA-8] Reg2=[CFA-16] Reg3=[CFA-24]
+  // 0x100C: CFA=reg12+CFAOff1: Reg1=[CFA-8] Reg2=[CFA-16]
+  // 0x1010: CFA=reg12+CFAOff1: Reg1=[CFA-8]
   // This state machine will:
   //  - set Reg1 location
   //  - push a row (from DW_CFA_advance_loc)
   //  - remember the state
   //  - set Reg2 location
   //  - push a row (from DW_CFA_advance_loc)
   //  - remember the state
+  //  - set CFA offset to CFAOff2
   //  - set Reg3 location
   //  - push a row (from DW_CFA_advance_loc)
   //  - remember the state where Reg1 and Reg2 were set
   //  - push a row (from DW_CFA_advance_loc)
   //  - remember the state where only Reg1 was set
   //  - push a row (automatically at the end of instruction parsing)
   // Then we verify that all registers are correct in all generated rows.
-  constexpr uint8_t Reg1 = 14;
-  constexpr uint8_t Reg2 = 15;
-  constexpr uint8_t Reg3 = 16;
   EXPECT_THAT_ERROR(
       parseCFI(TestFDE,
                {dwarf::DW_CFA_offset | Reg1, 1, dwarf::DW_CFA_advance_loc | 4,
                 dwarf::DW_CFA_remember_state, dwarf::DW_CFA_offset | Reg2, 2,
                 dwarf::DW_CFA_advance_loc | 4, dwarf::DW_CFA_remember_state,
+                dwarf::DW_CFA_def_cfa_offset, CFAOff2,
                 dwarf::DW_CFA_offset | Reg3, 3, dwarf::DW_CFA_advance_loc | 4,
                 dwarf::DW_CFA_restore_state, dwarf::DW_CFA_advance_loc | 4,
                 dwarf::DW_CFA_restore_state}),
       Succeeded());
 
   // Create locations that we expect the UnwindRow objects to contain after
   // parsing the DWARF call frame instructions.
   dwarf::RegisterLocations VerifyLocs1;
VerifyLocs3.setRegisterLocation(
       Reg3, dwarf::UnwindLocation::createAtCFAPlusOffset(-24));
 
   // Verify we catch state machine error.
   Expected<dwarf::UnwindTable> RowsOrErr = dwarf::UnwindTable::create(&TestFDE);
   EXPECT_THAT_ERROR(RowsOrErr.takeError(), Succeeded());
   const dwarf::UnwindTable &Rows = RowsOrErr.get();
   EXPECT_EQ(Rows.size(), 5u);
   EXPECT_EQ(Rows[0].getAddress(), 0x1000u);
+  EXPECT_EQ(Rows[0].getCFAValue(),
+            dwarf::UnwindLocation::createIsRegisterPlusOffset(12, CFAOff1));
   EXPECT_EQ(Rows[0].getRegisterLocations(), VerifyLocs1);
 
   EXPECT_EQ(Rows[1].getAddress(), 0x1004u);
+  EXPECT_EQ(Rows[1].getCFAValue(),
+            dwarf::UnwindLocation::createIsRegisterPlusOffset(12, CFAOff1));
   EXPECT_EQ(Rows[1].getRegisterLocations(), VerifyLocs2);
 
   EXPECT_EQ(Rows[2].getAddress(), 0x1008u);
+  EXPECT_EQ(Rows[2].getCFAValue(),
+            dwarf::UnwindLocation::createIsRegisterPlusOffset(12, CFAOff2));
   EXPECT_EQ(Rows[2].getRegisterLocations(), VerifyLocs3);
 
   EXPECT_EQ(Rows[3].getAddress(), 0x100Cu);
+  EXPECT_EQ(Rows[3].getCFAValue(),
+            dwarf::UnwindLocation::createIsRegisterPlusOffset(12, CFAOff1));
   EXPECT_EQ(Rows[3].getRegisterLocations(), VerifyLocs2);
 
   EXPECT_EQ(Rows[4].getAddress(), 0x1010u);
+  EXPECT_EQ(Rows[4].getCFAValue(),
+            dwarf::UnwindLocation::createIsRegisterPlusOffset(12, CFAOff1));
   EXPECT_EQ(Rows[4].getRegisterLocations(), VerifyLocs1);
 }
 
 TEST(DWARFDebugFrame, UnwindTable_DW_CFA_undefined) {
   // Test that DW_CFA_undefined works as expected when parsed in the state
   // machine.
   dwarf::CIE TestCIE = createCIE(/*IsDWARF64=*/false,
                                  /*Offset=*/0x0,