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

jingham at apple.com jingham at apple.com
Tue May 14 11:08:50 PDT 2013 

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