[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:28:59 PDT 2013
On May 14, 2013, at 11:25 AM, "Malea, Daniel" <daniel.malea at intel.com> wrote:
> Thanks a bunch! The fix seems to have cleared up the remaining Linux
> buildbot failures; they're back to green now :)
Excellent! We'll see if we can fix that ;-)
JIm
>
> 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