[Lldb-commits] [PATCH] Improve instruction emulation based stack unwinding on ARM

[Jason Molenda]

Tamas, I apologize for taking so long to get to this patch.  I meant to do it over the weekend but didn't have the time - I need to think hard about changes in the unwinder before I feel comfortable with them and it took me a couple hours of staring at the patch and thinking things over.

I much prefer your approach in EmulateInstruction of saving the unwind register state so that we can re-establish it after an epilogue.  Mine was not very elegant and I'm sure there are more complicated functions where mine could do the wrong thing.  In practice with the armv7/arm64 code that we were supporting, it worked fine.  But your approach is better.

One problem with the patch is that we're going to lose the mid-function epilogue correctness with the ARM64 instruction emulation plugin - it doesn't tag eContextAbsoluteBranchRegister / eContextRelativeBranchImmediate instructions which is the only way the m_forward_branch_offset ivar in EmulateInstruction is set.  I think it's reasonable to accept the patch knowing that we're regressing arm64 until someone (who supports arm64, like myself or Greg) adds the instruction support for recognizing branch instructions and correctly identifying the target offset of those branches.


================
Comment at: source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp:175
@@ -161,1 +174,3 @@
 
+                        // If the current instruction is a branch forward then save the current CIF information
+                        // for the offset where we are branching.
----------------
I think you mean "current CFI information" here.  The "I" in CFI stands for information, but it's one of those annoying aspects of english where it looks weird without "information" after it. :)

================
Comment at: source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp:188
@@ -164,29 +187,3 @@
                         {
-                            reinstate_prologue_next_instruction = false;
-                            m_curr_row->SetOffset (inst->GetAddress().GetFileAddress() + inst->GetOpcode().GetByteSize() - base_addr);
-                            // Append the new row
-                            unwind_plan.AppendRow (m_curr_row);
-
-                            // Allocate a new Row for m_curr_row, copy the current state into it
-                            UnwindPlan::Row *newrow = new UnwindPlan::Row;
-                            *newrow = *m_curr_row.get();
-                            m_curr_row.reset(newrow);
-
-                            // If m_curr_insn_restored_a_register == true, we're looking at an epilogue instruction.
-                            // Set instructions_since_last_prologue_insn to a very high number so we don't append 
-                            // any of these epilogue instructions to our prologue_complete row.
-                            if (m_curr_insn_restored_a_register == false && instructions_since_last_prologue_insn < 8)
-                              instructions_since_last_prologue_insn = 0;
-                            else
-                              instructions_since_last_prologue_insn = 99;
-
-                            UnwindPlan::Row::RegisterLocation pc_regloc;
-                            UnwindPlan::Row::RegisterLocation ra_regloc;
-
-                            // While parsing the instructions of this function, if we've ever
-                            // seen the return address register (aka lr on arm) in a non-IsSame() state,
-                            // it has been saved on the stack.  If it's ever back to IsSame(), we've
-                            // executed an epilogue.
-                            if (ra_reg_num != LLDB_INVALID_REGNUM
-                                && m_curr_row->GetRegisterInfo (ra_reg_num, ra_regloc)
-                                && !ra_regloc.IsSame())
+                            // Save the modified row if we don't already have a CIF row in the currennt address
+                            if (saved_unwind_states.count(current_offset + inst->GetOpcode().GetByteSize()) == 0)
----------------
CFI.  Call Frame Information.

http://reviews.llvm.org/D10447

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/