[Lldb-commits] [PATCH] Add an alternative CFA type.

[Jason Molenda]

Looks good, some very minor comments inlined, fine to commit after those are tweaked.  Have you tried exercising the code?  Either with an architecture default unwindplan in your ABI or with your eh_frame parser?  I'd like to know that it actually works for you. :)

================
Comment at: source/Plugins/Process/Utility/RegisterContextLLDB.cpp:1422
@@ -1421,2 +1421,3 @@
         m_full_unwind_plan_sp = m_fallback_unwind_plan_sp;
+        uint32_t cfa_regnum = active_row->GetCFARegister();
         addr_t cfa_regval = LLDB_INVALID_ADDRESS;
----------------
I don't see cfa_regnum being used in this code block.

================
Comment at: source/Plugins/Process/Utility/RegisterContextLLDB.cpp:1426
@@ -1424,3 +1425,3 @@
         {
             m_cfa = cfa_regval + active_row->GetCFAOffset ();
         }
----------------
I think ReadCFAValueForRow should add active_row->GetCFAOffset to the cfa register value itself (and the same thing up at line 575 for the previous call to ReadCFAValueForRow.  I bet you can drop the cfa_regval variable entirely and just have m_cfa.  This will break some UnwindLogMsg calls -- but I think ReadCFAValueForRow() should UnwindLogMsg the CFA register value and offset if it's a reg+offset CFA.

================
Comment at: source/Plugins/Process/Utility/RegisterContextLLDB.cpp:1461
@@ +1460,3 @@
+            value = reg_value.GetAsUInt64();
+            UnwindLogMsg("dereferenced address: %p yields: %lx\n", tmp, value);
+            return error.Success();
----------------
When printing an addr_t, you should use PRIx64.  e.g. "dereferenced address: 0x%" PRIx64 " yields: 0x%" PRIx64, tmp, value);".  also, no \n at the end of UnwindLogMsgs.

================
Comment at: source/Symbol/UnwindPlan.cpp:23
@@ -22,2 +22,3 @@
 UnwindPlan::Row::RegisterLocation::operator == (const UnwindPlan::Row::RegisterLocation& rhs) const
 {
+    bool is_match = true;
----------------
I don't have any problem with the changes to UnwindPlan::Row::RegisterLocation::operator == - but what motivated this change?  It seems like it's functionally the same.  Am I missing something?

http://reviews.llvm.org/D6182