[Lldb-commits] [PATCH] D14226: Fix to solve Bug 23139 & Bug 23560

[Jason Molenda via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.
This revision is now accepted and ready to land.

Hi Abhishek, I apologize for the long delay in reviewing this one, I needed to find a solid 30-45 minute block to review the changes and the current state of the unwinder code before I really understood what is going on here.

I think this change looks good, please commit it when you have a chance.


================
Comment at: source/Plugins/Process/Utility/UnwindLLDB.cpp:320
@@ +319,3 @@
+
+    // This function is called for First Frame only.
+    if (m_frames.size() != 1)
----------------
I would do this with an assert instead of a conditional check.  This method is only called from UnwindLLDB::AddFirstFrame().  AddFirstFrame starts by ensuring that m_frames is empty.  It constructs a Cursor object for the first stack frame, pushes it to m_frames.  And then it calls UpdateUnwindPlanForFirstFrameIfInvalid().

A conditional makes it look like this is a possible valid arrangement. Instead, what we're checking here is a basic assumption of the state of the object when this method should be called.  I think it's an appropriate space to use an assert.  If the code were to ever change around this method (or this method was called from another part of the code), we'd need it to crash immediately because the code would no longer be correct.


http://reviews.llvm.org/D14226