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

[jingham at apple.com]

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