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

[Pavel Labath]

The reason I avoided the SetOffset() function was that carried the assumption that the only way to set the CFA value was reg+offset, which I think was true at some point (and I think some of the code still assumes that, like the Row::Dump part which I removed), but it wasn't true even before this patch --- it could be set as [reg], in which case the offset was ignored. If we allow setting the offset separately, then the question is what should be done when the current value type is not "isRegisterPlusOffset":

- set the type to "isRegisterPlusOffset", but leave the register undefined?
- set the type to "isRegisterPlusOffset", and set the register to some default value?
- ignore the SetOffset() command?
- return an error?

IncOffset is slightly better I think because it is more obvious that if you want to increment something, it has to be valid in the first place. I agree that the SetIsRegisterPlusOffset makes things a bit cumbersome, but actually most of those places could be naturally changed to use IncOffset instead. And we have a precedent for now having a SetOffset function: Row::RegisterLocation provides a GetOffset method, but is only settable via SetAtCFAPlusOffset and the like.

What do you think?


================
Comment at: include/lldb/Symbol/UnwindPlan.h:244
@@ +243,3 @@
+
+            enum RestoreType
+                {
----------------
jasonmolenda wrote:
> 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.
How about just ValueType, given that the class is called CFAValue , and the getter is GetValueType()?

================
Comment at: include/lldb/Symbol/UnwindPlan.h:316
@@ +315,3 @@
+                m_type = isDWARFExpression;
+                m_value.expr.opcodes = opcodes;
+                m_value.expr.length = len;
----------------
jasonmolenda wrote:
> 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.
I modeled this class after Row::RegisterLocation, where they do the same thing, basically. So I think it should be ok...

http://reviews.llvm.org/D7755

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