>>! 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
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
