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

[Malea, Daniel]

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
>