[Lldb-commits] [PATCH]: Fix inline stepping test case (and possibly improve remote step performance)

Malea, Daniel daniel.malea at intel.com
Mon May 13 15:56:39 PDT 2013 

Oops, the one I attached last was incomplete. Here's the complete patch..

On 2013-05-13 6:53 PM, "Malea, Daniel" <daniel.malea at intel.com> wrote:

>Here's a patch that implements the last solution I proposed. It still
>fixes the problem with TestInlineStepping, and I think it's a little more
>general than my initial approach, and should be extendable to handle the
>case you mentioned with continue.
>
>Cheers,
>Dan
>
>On 2013-05-13 6:38 PM, "Malea, Daniel" <daniel.malea at intel.com> wrote:
>
>>So it looks like ThreadPlanStepInRange already has an m_virtual_step
>>member that keeps track of this pretend-stepping sitaution already. I was
>>hesitant to expose this internal flag because I couldn't think of what
>>use/meaning that member would have for other thread plans, but based on
>>the step-out use case you mentioned, maybe it should be exposed?
>>
>>Perhaps something like:
>>
>>ThreadPlan::IsVirtualStep() { return false; }
>>
>>With an override in ThreadPlanStepInRange that returns m_virtual_step.
>>Once we handle the step-out case you mentioned ThreadPlanStepOut (and any
>>other plans which need to do some virtual steps) can also override that
>>function?
>>
>>What do you think?
>>
>>Just to confirm, there's no existing test case for the use-case you
>>mentioned, right?
>>
>>Cheers,
>>Dan
>>
>>On 2013-05-13 6:24 PM, "jingham at apple.com" <jingham at apple.com> wrote:
>>
>>>I don't think we handle it yet, but we also should do the same thing on
>>>the way out if you are stepping out of a bunch of inlined functions
>>>whose
>>>returns all have the same PC.  Just something to keep in mind.
>>>
>>>And to complete the picture, the other major bit we don't do yet is for
>>>instance if you have:
>>>
>>>0x0000    ContainingFunction
>>>0x0004    InlinedFunction1
>>>0x0004    InlinedFunction2
>>>0x0004    InlinedFunction3
>>>
>>>and somebody does:
>>>
>>>(lldb) break set -n InlinedFunction2
>>>
>>>and then runs, we don't properly set the "inlined stack depth" to 1 when
>>>we hit the breakpoint.  So this area still needs a little bit of work.
>>>
>>>Jim
>>>
>>>On May 13, 2013, at 3:19 PM, jingham at apple.com wrote:
>>>
>>>> The general idea seems fine.  The thread requests a "virtual step" in
>>>>DoWillResume, maybe you can just keep track of that?  That's more
>>>>declarative, and if we come up with some other reason to "pretend to
>>>>step without stepping" it would get that too.  OTOH, you'd have to
>>>>manage setting it in the right place, and remember to clear it in
>>>>WillResume, so maybe that is a pain?
>>>> 
>>>> Otherwise, if you look in StackFrameList.cpp, you'll see a couple of
>>>>functions that deal with the current inlined depth
>>>>(GetCurrentInlinedDepth, IncrementCurrentInlinedDepth.)  I am pretty
>>>>sure (haven't looked at this code in a while) that you should just be
>>>>able to ask whether the current inlined depth is > 0.  When the PC
>>>>moves
>>>>away from the stack of same-pc inlined functions, the InlinedDepth goes
>>>>back to 0 till you get to another similar set.  But just take a look at
>>>>that code and it should be clear.
>>>> 
>>>> Jim
>>>> 
>>>> 
>>>> On May 13, 2013, at 3:07 PM, "Malea, Daniel" <daniel.malea at intel.com>
>>>>wrote:
>>>> 
>>>>> Ah, I see; I didn't think of that case. Thanks for catching it!
>>>>> 
>>>>> Do you agree with the basic approach of the patch though? If so,
>>>>>what's
>>>>> the best way to check for a 'virtual inline step'. Would it be to
>>>>>check
>>>>> that frame 0 and frame 1 have the same PC, or is there something else
>>>>>that
>>>>> would be better?
>>>>> 
>>>>> 
>>>>> Dan
>>>>> 
>>>>> On 2013-05-13 6:04 PM, "jingham at apple.com" <jingham at apple.com> wrote:
>>>>> 
>>>>>> Daniel,
>>>>>> 
>>>>>> I think your test in IsVirtualInlineStep is too broad.
>>>>>> 
>>>>>> The virtual inlined step is the one where you have several stack
>>>>>>frames
>>>>>> with the SAME PC, and you are logically at a higher one of these,
>>>>>>and
>>>>>> then the user says "step in".  That means just decrement the virtual
>>>>>> frame counter, so it looks like the next frame in the "contiguous
>>>>>>inlined
>>>>>> frame stack" is the current one.  But you don't have to run the
>>>>>>target to
>>>>>> do this.
>>>>>> 
>>>>>> So a step is not a "virtual inlined step" just because the stack
>>>>>>frame at
>>>>>> index 0 is inlined.  After all, you could be in the middle of an
>>>>>>inlined
>>>>>> function, and do a step that is just an ordinary straight-up step,
>>>>>>over
>>>>>> some code with no debug info, etc.  In the course of doing that, you
>>>>>> could hit a breakpoint in inlined code somewhere deeper on the
>>>>>>stack,
>>>>>>and
>>>>>> if I read it aright, your patch would preserve the stop info from
>>>>>>the
>>>>>> step in, which would be incorrect.
>>>>>> 
>>>>>> Jim
>>>>>> 
>>>>>> On May 13, 2013, at 2:18 PM, "Malea, Daniel"
>>>>>><daniel.malea at intel.com>
>>>>>> wrote:
>>>>>> 
>>>>>>> Hi all,
>>>>>>> 
>>>>>>> Please review the attached patch that fixes the TestInlineStep case
>>>>>>>on
>>>>>>> Linux which was broken after r181501. The fix involves tweaking
>>>>>>> Thread::GetPrivateStopReason() to not reset the m_stop_info_sp in
>>>>>>>the
>>>>>>> case of a "virtual" inline step-in (that is, a step that does not
>>>>>>>cause
>>>>>>> the inferior process to resume, but rather just pretends to step
>>>>>>>into an
>>>>>>> inlined function call).
>>>>>>> 
>>>>>>> I also removed a double-initialization and check for a Process at
>>>>>>>the
>>>>>>> start of the function. In addition, with my change, I expect that
>>>>>>>when
>>>>>>> remotely stepping into an inlined function call, LLDB will avoid
>>>>>>>the
>>>>>>> needless call into ThreadGDBRemote::CalculateStopReason, which if I
>>>>>>>read
>>>>>>> it correctly, causes some packets to be sent over the wire.
>>>>>>> 
>>>>>>> 
>>>>>>> An alternative way to implement what I'm doing here would be to
>>>>>>> introduce a new stop reason as per the comment in
>>>>>>> ThreadPlanStepInRange.cpp:458 but I expect that will require a lot
>>>>>>>more
>>>>>>> changes to accomplish essentially the same thing.
>>>>>>> 
>>>>>>> Reviews are welcome,
>>>>>>> Dan
>>>>>>> 
>>>>>>> 
>>>>>>><handle_virtual_inline_step.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
>>>
>>
>>
>>_______________________________________________
>>lldb-commits mailing list
>>lldb-commits at cs.uiuc.edu
>>http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: handle_virtual_inline_step_v3.patch
Type: application/octet-stream
Size: 3175 bytes
Desc: handle_virtual_inline_step_v3.patch
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20130513/659ef5a6/attachment.obj>

More information about the lldb-commits mailing list