[Lldb-commits] [PATCH] UnwindPlan::Row refactor -- add support for CFA set by a DWARF expression

[Jason Molenda]

This looks fine to me.  I don't have a problem with the CFAValue::IncOffset() method but it would be more in keeping with the typical lldb style if this class still had a SetOffset() method.  It's more a consistency thing than one being better than the other - if I see GetOffset(), I'm going to assume there is also a SetOffset().  I'm fine with keeping IncOffset() - in some code sequences it will be more natural than SetOffset.  And you've made the code a little less clear in places where that is the natural way to express what is being done.  Like in UnwindAssembly-x86.cpp we went from

  row->SetCFAOffset (current_sp_bytes_offset_from_cfa);

to

  row->GetCFAValue().SetIsRegisterPlusOffset(m_lldb_sp_regnum,
          current_sp_bytes_offset_from_cfa);

which obscures what is really being done IMO.  I'd prefer the places where you changed SetOffset to SetIsRegisterPlusOffset remain SetOffset.


================
Comment at: include/lldb/Symbol/UnwindPlan.h:244
@@ +243,3 @@
+
+            enum RestoreType
+                {
----------------
Very minor comment but I don't know if RestoreType is the best nomenclature in this case.  I don't have a better suggestion right now.  In an unwind scenario, I think of "restore" as meaning "retrieving the value of a register that was saved by a callee".  Maybe I'll think up something better later.  CFACalculateType would be closer, but that sounds horrible.

================
Comment at: include/lldb/Symbol/UnwindPlan.h:316
@@ +315,3 @@
+                m_type = isDWARFExpression;
+                m_value.expr.opcodes = opcodes;
+                m_value.expr.length = len;
----------------
I was worried about the ownership of the opcode bytes in SetIsDWARFExpression but I guess the assumption is that this will come from the DWARF for the Module and since the UnwindPlans are also owned by the Module, they'll have the same lifetime.  This should be fine.

http://reviews.llvm.org/D7755

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