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

Malea, Daniel daniel.malea at intel.com
Tue May 14 08:21:35 PDT 2013 

Thanks Jim, IMHO it makes sense to query the current thread plan in order
to decide if the stop reason should be preserved, since atm only a thread
plan knows about these virtual steps. If anyone has a different opinion,
I'm happy to copy the isVirtualStep flag into process as well.

Committed in r181795.

Cheers,
Dan

On 2013-05-13 7:21 PM, "jingham at apple.com" <jingham at apple.com> wrote:

>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>
>

More information about the lldb-commits mailing list