Fix bug in UnwindAssemblyInstEmulation with fp-using codegen and mid-function epilogues

[jasonmolenda]

Summary

I found this on an AAarch64 target, where we use a frame pointer register on Darwin. UnwindAssemblyInstEmulation iterates over the instructions building up an UnwindPlan row by row. When UnwindAssemblyInstEmulation sees a branch instruction, it "forwards" the unwind state to the target offset; if we have a mid-function epilogue that restores the spilled registers and returns, when we hit the branch target, the saved unwind state is reinstated.

The bug comes in that UnwindAssemblyInstEmulation maintains both the "current row" object and it tracks the register that the Canonical Frame Address is defined in terms of, and whether this register is the frame pointer or not. After the prologue we have unwind state defining the CFA as $fp+16; then during a mid-function epilogue we restore the registers and switch to the CFA as $sp based. At this point the UnwindAssemblyInstEmulation ivars (CFA register, is-it-fp-or-not) are updated to recognize the switch to $sp. After the epilogue, we restore the current-row but we leave the other ivars to their epilogue setting.

The failure then comes in if the instructions after the mid-function epilogue modify $sp, UnwindAssemblyInstEmulation changes the current row's CFA offset value to track that change, because it thinks the CFA is defined in terms of the stack pointer. But the current row is still defining it in terms of $fp. So everything fails.

It's a sneaky bug because it's easy to miss if lldb silently "falls back" to the architectural default unwind plan when it can't find the caller. So we're missing spilled registers from this frame. It takes some specific circumstances to hit it -- modifying $sp after that mid-function epilogue.

I know not a lot of people have touched this code in ages (including me!), but let's put it up for review in case anyone has questions.

[clayborg]

If you test worked, then there is something wrong with this test? See inline comment for copy and paste error

lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp

140

Is there a copy and paste error here?:

s/ra_reg_info/sp_reg_info/

[jasonmolenda]

In D78077#1980116, @clayborg wrote:

If you test worked, then there is something wrong with this test? See inline comment for copy and paste error

Thanks for catching that. The test definitely fails without the patch, and works with. lldb is using that sp_reg_info to re-set the m_fp_is_cfa ivar, which my test clearly does not exercise if it is wrong.

[jasonmolenda]

Update to address mistake Greg identified; also remove two unused variables that were in this method before my changes.

lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp

if (disasm_sp) {
         UnwindPlan::Row *newrow = new UnwindPlan::Row;
         if (last_row.get())
           *newrow = *last_row.get();
         m_curr_row.reset(newrow);
 
         // Add the initial state to the save list with offset 0.
         saved_unwind_states.insert({0, {last_row, m_register_values}});
 
-        // cache the pc register number (in whatever register numbering this
-        // UnwindPlan uses) for quick reference during instruction parsing.
-        RegisterInfo pc_reg_info;
-        m_inst_emulator_up->GetRegisterInfo(
-            eRegisterKindGeneric, LLDB_REGNUM_GENERIC_PC, pc_reg_info);
-
-        // cache the return address register number (in whatever register
+        // cache the stack pointer register number (in whatever register
         // numbering this UnwindPlan uses) for quick reference during
         // instruction parsing.
-        RegisterInfo ra_reg_info;
+        RegisterInfo sp_reg_info;
         m_inst_emulator_up->GetRegisterInfo(
-            eRegisterKindGeneric, LLDB_REGNUM_GENERIC_RA, ra_reg_info);
+            eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP, sp_reg_info);
 
         // The architecture dependent condition code of the last processed
         // instruction.
         EmulateInstruction::InstructionCondition last_condition =
             EmulateInstruction::UnconditionalCondition;
         lldb::addr_t condition_block_start_offset = 0;
 
         for (size_t idx = 0; idx < num_instructions; ++idx) {
           m_curr_row_modified = false;
           m_forward_branch_offset = 0;
 
           inst = inst_list.GetInstructionAtIndex(idx).get();
           if (inst) {
             lldb::addr_t current_offset =
                 inst->GetAddress().GetFileAddress() - base_addr;
             auto it = saved_unwind_states.upper_bound(current_offset);
             assert(it != saved_unwind_states.begin() &&
                    "Unwind row for the function entry missing");
             --it; // Move it to the row corresponding to the current offset
 
             // If the offset of m_curr_row don't match with the offset we see
             // in saved_unwind_states then we have to update m_curr_row and
             // m_register_values based on the saved values. It is happening
             // after we processed an epilogue and a return to caller
             // instruction.
             if (it->second.first->GetOffset() != m_curr_row->GetOffset()) {
               UnwindPlan::Row *newrow = new UnwindPlan::Row;
               *newrow = *it->second.first;
               m_curr_row.reset(newrow);
               m_register_values = it->second.second;
+              // re-set the CFA register ivars to match the
+              // new m_curr_row.
+              if (sp_reg_info.name &&
+                  m_curr_row->GetCFAValue().IsRegisterPlusOffset()) {
+                uint32_t row_cfa_regnum =
+                    m_curr_row->GetCFAValue().GetRegisterNumber();
+                lldb::RegisterKind row_kind =
+                    m_unwind_plan_ptr->GetRegisterKind();
+                // set m_cfa_reg_info to the row's CFA reg.
+                m_inst_emulator_up->GetRegisterInfo(row_kind, row_cfa_regnum,
+                                                    m_cfa_reg_info);
+                // set m_fp_is_cfa.
+                if (sp_reg_info.kinds[row_kind] == row_cfa_regnum)
+                  m_fp_is_cfa = false;
+                else
+                  m_fp_is_cfa = true;
+              }
             }
 
             m_inst_emulator_up->SetInstruction(inst->GetOpcode(),
                                                inst->GetAddress(), nullptr);
 
             if (last_condition !=
                 m_inst_emulator_up->GetInstructionCondition()) {
               if (m_inst_emulator_up->GetInstructionCondition() !=
if (disasm_sp) {
               if (last_condition !=
                   EmulateInstruction::UnconditionalCondition) {
                 const auto &saved_state =
                     saved_unwind_states.at(condition_block_start_offset);
                 m_curr_row =
                     std::make_shared<UnwindPlan::Row>(*saved_state.first);
                 m_curr_row->SetOffset(current_offset);
                 m_register_values = saved_state.second;
+                // re-set the CFA register ivars to match the
+                // new m_curr_row.
+                if (sp_reg_info.name &&
+                    m_curr_row->GetCFAValue().IsRegisterPlusOffset()) {
+                  uint32_t row_cfa_regnum =
+                      m_curr_row->GetCFAValue().GetRegisterNumber();
+                  lldb::RegisterKind row_kind =
+                      m_unwind_plan_ptr->GetRegisterKind();
+                  // set m_cfa_reg_info to the row's CFA reg.
+                  m_inst_emulator_up->GetRegisterInfo(row_kind, row_cfa_regnum,
+                                                      m_cfa_reg_info);
+                  // set m_fp_is_cfa.
+                  if (sp_reg_info.kinds[row_kind] == row_cfa_regnum)
+                    m_fp_is_cfa = false;
+                  else
+                    m_fp_is_cfa = true;
+                }
                 bool replace_existing =
                     true; // The last instruction might already
                           // created a row for this offset and
                           // we want to overwrite it.
                 unwind_plan.InsertRow(
                     std::make_shared<UnwindPlan::Row>(*m_curr_row),
                     replace_existing);
               }

lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp

TEST_F(TestArm64InstEmulation, TestRegisterDoubleSpills) {
   }
   if (row_sp->GetRegisterInfo(gpr_x27_arm64, regloc)) {
     EXPECT_TRUE(regloc.IsSame());
   }
   if (row_sp->GetRegisterInfo(gpr_x28_arm64, regloc)) {
     EXPECT_TRUE(regloc.IsSame());
   }
 }
+
+TEST_F(TestArm64InstEmulation, TestCFARegisterTrackedAcrossJumps) {
+  ArchSpec arch("arm64-apple-ios10");
+  std::unique_ptr<UnwindAssemblyInstEmulation> engine(
+      static_cast<UnwindAssemblyInstEmulation *>(
+          UnwindAssemblyInstEmulation::CreateInstance(arch)));
+  ASSERT_NE(nullptr, engine);
+
+  UnwindPlan::RowSP row_sp;
+  AddressRange sample_range;
+  UnwindPlan unwind_plan(eRegisterKindLLDB);
+  UnwindPlan::Row::RegisterLocation regloc;
+
+  uint8_t data[] = {
+      // prologue
+      0xf4, 0x4f, 0xbe, 0xa9, //  0: 0xa9be4ff4 stp x20, x19, [sp, #-0x20]!
+      0xfd, 0x7b, 0x01, 0xa9, //  4: 0xa9017bfd stp x29, x30, [sp, #0x10]
+      0xfd, 0x43, 0x00, 0x91, //  8: 0x910043fd add x29, sp, #0x10
+      0xff, 0x43, 0x00, 0xd1, // 12: 0xd10043ff sub sp, sp, #0x10
+      // conditional branch over a mid-function epilogue
+      0xeb, 0x00, 0x00, 0x54, // 16: 0x540000eb b.lt <+44>
+      // mid-function epilogue
+      0x1f, 0x20, 0x03, 0xd5, // 20: 0xd503201f   nop
+      0xe0, 0x03, 0x13, 0xaa, // 24: 0xaa1303e0   mov    x0, x19
+      0xbf, 0x43, 0x00, 0xd1, // 28: 0xd10043bf   sub    sp, x29, #0x10
+      0xfd, 0x7b, 0x41, 0xa9, // 32: 0xa9417bfd   ldp    x29, x30, [sp, #0x10]
+      0xf4, 0x4f, 0xc2, 0xa8, // 36: 0xa8c24ff4   ldp    x20, x19, [sp], #0x20
+      0xc0, 0x03, 0x5f, 0xd6, // 40: 0xd65f03c0   ret
+      // unwind state restored, we're using a frame pointer, let's change the
+      // stack pointer and see no change in how the CFA is computed
+      0x1f, 0x20, 0x03, 0xd5, // 44: 0xd503201f   nop
+      0xff, 0x43, 0x00, 0xd1, // 48: 0xd10043ff   sub    sp, sp, #0x10
+      0x1f, 0x20, 0x03, 0xd5, // 52: 0xd503201f   nop
+      // final epilogue
+      0xe0, 0x03, 0x13, 0xaa, // 56: 0xaa1303e0   mov    x0, x19
+      0xbf, 0x43, 0x00, 0xd1, // 60: 0xd10043bf   sub    sp, x29, #0x10
+      0xfd, 0x7b, 0x41, 0xa9, // 64: 0xa9417bfd   ldp    x29, x30, [sp, #0x10]
+      0xf4, 0x4f, 0xc2, 0xa8, // 68: 0xa8c24ff4   ldp    x20, x19, [sp], #0x20
+      0xc0, 0x03, 0x5f, 0xd6, // 72: 0xd65f03c0   ret
+
+      0x1f, 0x20, 0x03, 0xd5, // 52: 0xd503201f   nop
+  };
+
+  // UnwindPlan we expect:
+  // row[0]:    0: CFA=sp +0 =>
+  // row[1]:    4: CFA=sp+32 => x19=[CFA-24] x20=[CFA-32]
+  // row[2]:    8: CFA=sp+32 => x19=[CFA-24] x20=[CFA-32] fp=[CFA-16] lr=[CFA-8]
+  // row[3]:   12: CFA=fp+16 => x19=[CFA-24] x20=[CFA-32] fp=[CFA-16] lr=[CFA-8]
+  // row[4]:   32: CFA=sp+32 => x19=[CFA-24] x20=[CFA-32] fp=[CFA-16] lr=[CFA-8]
+  // row[5]:   36: CFA=sp+32 => x19=[CFA-24] x20=[CFA-32] fp= <same> lr= <same>
+  // row[6]:   40: CFA=sp +0 => x19= <same> x20= <same> fp= <same> lr= <same> 
+  // row[7]:   44: CFA=fp+16 => x19=[CFA-24] x20=[CFA-32] fp=[CFA-16] lr=[CFA-8] 
+  // row[8]:   64: CFA=sp+32 => x19=[CFA-24] x20=[CFA-32] fp=[CFA-16] lr=[CFA-8] 
+  // row[9]:   68: CFA=sp+32 => x19=[CFA-24] x20=[CFA-32] fp= <same> lr= <same> 
+  // row[10]:  72: CFA=sp +0 => x19= <same> x20= <same> fp= <same> lr= <same> 
+
+  // The specific bug we're looking for is this incorrect CFA definition, 
+  // where the InstEmulation is using the $sp value mixed in with $fp, 
+  // it looks like this:
+  //
+  // row[7]:   44: CFA=fp+16 => x19=[CFA-24] x20=[CFA-32] fp=[CFA-16] lr=[CFA-8]
+  // row[8]:   52: CFA=fp+64 => x19=[CFA-24] x20=[CFA-32] fp=[CFA-16] lr=[CFA-8]
+  // row[9]:   68: CFA=fp+64 => x19=[CFA-24] x20=[CFA-32] fp= <same> lr= <same>
+ 
+  sample_range = AddressRange(0x1000, sizeof(data));
+
+  EXPECT_TRUE(engine->GetNonCallSiteUnwindPlanFromAssembly(
+      sample_range, data, sizeof(data), unwind_plan));
+
+  // Confirm CFA at mid-func epilogue 'ret' is $sp+0
+  row_sp = unwind_plan.GetRowForFunctionOffset(40);
+  EXPECT_EQ(40ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == gpr_sp_arm64);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+  EXPECT_EQ(0, row_sp->GetCFAValue().GetOffset());
+
+  // After the 'ret', confirm we're back to the correct CFA of $fp+16
+  row_sp = unwind_plan.GetRowForFunctionOffset(44);
+  EXPECT_EQ(44ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == gpr_fp_arm64);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+  EXPECT_EQ(16, row_sp->GetCFAValue().GetOffset());
+
+  // Confirm that we have no additional UnwindPlan rows before the 
+  // real epilogue -- we still get the Row at offset 44.
+  row_sp = unwind_plan.GetRowForFunctionOffset(60);
+  EXPECT_EQ(44ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == gpr_fp_arm64);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+  EXPECT_EQ(16, row_sp->GetCFAValue().GetOffset());
+
+  // And in the epilogue, confirm that we start by switching back to 
+  // defining the CFA in terms of $sp.
+  row_sp = unwind_plan.GetRowForFunctionOffset(64);
+  EXPECT_EQ(64ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == gpr_sp_arm64);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+  EXPECT_EQ(32, row_sp->GetCFAValue().GetOffset());
+}
+