[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:53:22 PDT 2013 

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_v2.patch
Type: application/octet-stream
Size: 2261 bytes
Desc: handle_virtual_inline_step_v2.patch
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20130513/6472f583/attachment.obj>

More information about the lldb-commits mailing list