[Lldb-commits] [lldb][PATCH] Clear thread unwinder when stack is reset

[Jason Molenda]

Hi Andy, sorry again for putting this one off.

I think your patch is fine.  I traced through the ownership of all the relevant objects.  There is the slight oddity that an UnwindLLDB has a vector of RegisterContextLLDB shared pointers, and the RegisterContextLLDB objects have a pointer back to the UnwindLLDB that owns them.  They need the back pointer to avoid using recursion when we try to retrieve register values / do a stack walk - the UnwindLLDB object can do an iterative lookup over the vector of SP's to find a saved value.

Then off on the side, the Thread's StackFrameList's StackFrame object has a shared pointer to the RegisterContextLLDB to provide register values.

So my concern was with UnwindLLDB clearing its vector of RegisterContextLLDB shared pointers but the old stack frame list still having references to those RegisterContextLLDB's.  If anyone asked one of the old RCLLDB's to retrieve a register value, they would ask their owning UnwindLLDB to find the saved value -- but now we're looking at an entirely different stack.

But thinking about it for a while, I don't see this being a problem in real world use.

I'll commit your patch later today.

J


On Nov 12, 2012, at 2:25 PM, "Kaylor, Andrew" <andrew.kaylor at intel.com> wrote:

> Ping (with refreshed patch).
> 
> We've been testing with this patch (as attached) and it seems to work without introducing other problems.
> 
> The question of whether it's best in the limited case (as attached) or in a more general location (ClearStackFrames) is still open and unaddressed.
> 
> -Andy
> 
> -----Original Message-----
> From: lldb-commits-bounces at cs.uiuc.edu [mailto:lldb-commits-bounces at cs.uiuc.edu] On Behalf Of Kaylor, Andrew
> Sent: Tuesday, October 02, 2012 9:27 AM
> To: Jason Molenda
> Cc: lldb-commits at cs.uiuc.edu
> Subject: Re: [Lldb-commits] [lldb][PATCH] Clear thread unwinder when stack is reset
> 
> Hi Jason,
> 
> Thanks for your looking at this.
> 
> This is indeed complicated code!  I was a bit perplexed as I worked through this by the number of different places that stack frames and register contexts were being tracked.  I'll admit that my own understanding of it all is vague at best.
> 
> In the version of the patch I submitted that calls Unwind::Clear from Thread::ResetFrameZeroRegisters that call would happen immediately following a call to Thread::ClearStackFrames, and of course my other candidate for clearing was within ClearStackFrames itself, so unless I'm misunderstanding something (most certainly possible) there shouldn't be any problems with regard to saved frames.
> 
> In the particular case I was working with, a call to InferiorCallMmap had hijacked a thread in the inferior for a call to mmap and it needed to put things back the way they were when that call was done.  A private breakpoint at the mmap return address had caused the Thread::Unwind object to store an essentially bogus stack, and so when Process::RunThreadPlan was trying to restore the selected frame after its work was done, it couldn't find the frame it was looking for.  (BTW, the code at this point is a being worrisome in that it does nothing if it can't find the old selected frame.  It seems like it might be best to raise an error, or at least log something, there.)  The consequence was that subsequent lldb commands were confused as to what the current frame was.  For instance, 'expression argc' failed because it didn't recognize that 'argc' was a parameter to the current function.
> 
> In this case, at least, clearing the unwinder's internal state sometime after the ThreadPlanCallFunction has cleaned up the actual frame is necessary.  I'm reasonably confident that the correct place to do this is from within the ThreadPlanCallFunction::DoTakedown call to m_thread.RestoreThreadStateFromCheckpoint.  I'm less confident about the details of precisely how it should happen, so I appreciate your input in that regard.
> 
> -Andy
> 
> 
> -----Original Message-----
> From: Jason Molenda [mailto:jmolenda at apple.com] 
> Sent: Monday, October 01, 2012 11:39 PM
> To: Kaylor, Andrew
> Cc: lldb-commits at cs.uiuc.edu
> Subject: Re: [Lldb-commits] [lldb][PATCH] Clear thread unwinder when stack is reset
> 
> Thanks for submitting the patches Andy.  This part of the unwinder is a little fragile and hard to see the dependencies correctly - I'm not yet sure what I think here.
> 
> Normally a thread has an Unwind object (usually an UnwindLLDB object).  The UnwindLLDB object provides the Thread with RegisterContexts up the stack -- it provides register values for all the stack frames on a thread.  So if you want to get rip on frame 4, the Unwind will give you a RegisterContext for frame 4 that provides the value, assuming it is retrievable (and it will usually do this by seeing where frame 3 stored the value, going down the stack frames until it finds someone who saved it.)
> 
> The problem is this:  The Thread has an Unwind object in the m_unwinder_ap, and the Unwind object has an array of RegisterContexts that it is responsible for.  But IIRC the StackFrames also have pointers into these RegisterContexts.  If we're throwing away all known StackFrames for a Thread, it should be safe to reset the Unwind object.  But if we're *restoring* stack frames that were saved aside earlier, I worry that they may have pointers to RegisterContexts that have since been freed or something.
> 
> (and to further complicate things, I think the RegisterContextLLBB's refer to their "owning" UnwindLLDB object to retrieve values).
> 
> This is probably all done with refcounted shared pointers and will "just work" but it's a complicated enough interdependency that I need to look at it a bit more closely before I'm confident about what the right answer is here.  I haven't looked at this part of the code in probably a year or so.
> 
> J
> 
> 
> On Oct 1, 2012, at 1:42 PM, "Kaylor, Andrew" <andrew.kaylor at intel.com> wrote:
> 
>> In debugging a problem with expression evaluation after an inferior function call, I tracked it back to the Thread object's m_unwinder_ap member not being cleared when the Thread state was restored from a checkpoint.  I was able to fix the problem by inserting a call to Unwind::Clear in the Thread::ResetFrameZeroRegisters function.  I thought that it might make sense to clear the unwinder every time the thread's stack was cleared (i.e. from Thread::ClearStackFrames), but I wasn't sure how that is used and whether it always indicates an actual stack change that the unwinder needs to know about.
>> 
>> The problem in question showed up in the BasicExprCommandsTestCase.test_many_expr_commands test on x86_64 Linux.
>> 
>> I'm attaching two patches, one with each implementation, though only one is needed.  Can someone offer some guidance as to which approach is preferred?
>> 
>> Thanks,
>> Andy
>> 
>> <unwinder-reset.patch><reset-frame-zero-unwind.patch>_______________________________________________
>> lldb-commits mailing list
>> lldb-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
> 
> 
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
> <reset-frame-zero-unwind.patch>_______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits