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

Malea, Daniel daniel.malea at intel.com
Tue May 14 11:25:28 PDT 2013 

Thanks a bunch! The fix seems to have cleared up the remaining Linux
buildbot failures; they're back to green now :)

Cheers,
Dan

On 2013-05-14 2:08 PM, "jingham at apple.com" <jingham at apple.com> wrote:

>What you did seems fine.  There is a whole class of stuff you want to ask
>the "current thread plan" when figuring out whether you ShouldStop.  You
>just have to know what you're doing when you do that.
>
>Jim
>
>
>
>On May 14, 2013, at 8:21 AM, "Malea, Daniel" <daniel.malea at intel.com>
>wrote:
>
>> 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