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/
_______________________________________________
lldb-commits mailing list
lldb-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
