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

[Justin Hibbits]

>>! In D6182#5, @jasonmolenda wrote:
> 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. :)

I have both powerpc64 and powerpc32 unwinding with the default unwindplan now (to be committed separately).  Past experience has taught me infrastructure first and separate when reviewing.

================
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;
----------------
jasonmolenda wrote:
> I don't see cfa_regnum being used in this code block.
Oops, missed this in my cleanup.  In my first iteration I was passing discrete information, but changed to passing the row itself instead.

================
Comment at: source/Plugins/Process/Utility/RegisterContextLLDB.cpp:1426
@@ -1424,3 +1425,3 @@
         {
             m_cfa = cfa_regval + active_row->GetCFAOffset ();
         }
----------------
jasonmolenda wrote:
> 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.
I debated doing this and went with the path of least code change, but wanted your opinion.  Trivial for me to change it.

================
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();
----------------
jasonmolenda wrote:
> 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.
Gotcha.  This started out as a debug printf for me that I changed to log and didn't update.

================
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;
----------------
jasonmolenda wrote:
> 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? 
This was part of the patch you sent me.  I ripped out a chunk that didn't compile, and moved it down to UnwindPlan::Row::operator==(), and was left with this.  I'll revert this since it makes no sense to keep it as changed.

http://reviews.llvm.org/D6182