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

[jingham at apple.com]

Thanks for working on this...

You have to be a little careful about using "GetCurrentPlan" for this sort of thing, as it makes the answer you get to the question dependent on when in the handling of the stop event you ask it.  For instance, if the current plan is step in, then the completion of the virtual step will finish its job.  So in ShouldStop, that plan will be moved from the "plan stack" to the "completed plan stack", making the plan above it on the plan stack be the "current plan".  That means that if you ask what GetCurrentPlan thinks about the reason for the stop before ShouldStop is done, you'll get one answer, and if you ask it after you will get another.  

Of course there are parts of the plan stack handling logic where it is appropriate to ask the current plan.  So if it makes sense that this question will always get asked before the Thread->ShouldStop logic get striggered (which is when plans get reaped from the current plan stack) then it's okay to ask the plans.  If so, you should document that the call requires this ordering.  Otherwise, it might be a better idea to stash this bit of info away in the thread right when you stop, and ask the thread the question.

Jim


On May 13, 2013, at 3:56 PM, "Malea, Daniel" <daniel.malea at intel.com> wrote:

> 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
>> 
> 
> <handle_virtual_inline_step_v3.patch>