Ah, sorry for the delay in responding here; this test slipped by. Here's an updated patch -- essentially the same as the last one except without relying on GetStackFrameCount. Instead, ThreadPlanStepOverRange::ShouldStop now loops until it gets a null stack-frame or a case where a new ThreadPlanStepOverRange must be queued in order to step out of an inlined call.
This addresses 1 (of the now 3) test failures on the Linux GCC buildbot. Cheers, dan On 2013-09-09 7:26 PM, "[email protected]" <[email protected]> wrote: >Daniel, > >We try never to call "GetStackFrameCount" in the ShouldStop methods of >any thread plans. This can slow down stepping noticeably when you are >far down on the stack. > >If the problem is that there is confusion when you are stepping through >nested inlines, then it should be possible to look up the stack till you >hit the first non-inlined function. If that doesn't work we should come >up with some other heuristic when to give up the comparison and just stop. > >Jim > > >On Sep 9, 2013, at 3:33 PM, Malea, Daniel <[email protected]> wrote: > >> Hi Jim, >> >> Thanks again for the feedback. Here's another attempt at fixing the >> problem -- when you have a moment, could you take a look? >> >> This patch modifies ThreadPlanStepOut::ShouldStop to check every frame >> instead of just the first frame when deciding whether to queue another >> step-out thread plan. This appears to be required in cases (such as in >> TestInlineStepping) where there are multiple levels of functions being >> inlined. >> >> Also, I factored out the logic that compares SymbolContexts into a >> separate (private) helper function to keep ShouldStop() a little bit >> smaller. >> >> Testing: >> >> - verified this fixes TestInlineStepping (on Linux with Clang trunk >> and GCC 4.7) >> - no other regressions detected in the test suite >> >> >> >> Cheers, >> Dan >> >> >> On 2013-09-06 5:55 PM, "[email protected]" <[email protected]> wrote: >> >>> Ah, okay. >>> >>> The reason the code was originally written the way it was was to >>> distinguish the case where we have debug information for the two >>>frames, >>> and the case where we don't. If you have a Compile Unit, then you have >>> debug information. Then the code in that branch dealt with the debug >>> information case. The other part of the if was if you have symbols, we >>> would compare the symbols. If there aren't any symbols at all, then we >>> don't know what to do and we just stop. >>> >>> The test case is showing us that there is some case of "I have debug >>> information, and the two contexts SHOULD be considered equivalent" >>>which >>> isn't being correctly handled by the section of the code that is >>>dealing >>> with "when you have debug information." Your fix is to move that case >>>to >>> the logic which handles the "I don't have debug information, only >>> symbols" case. So your patch fixes the incorrect logic in the debug >>>case >>> by accident, rather than getting the "have debug info" logic correct. >>> That doesn't seem like the right fix to me. >>> >>> I'm in the middle of something right now, so if you can figure out what >>> branch of the "have debug info part" of this is wrong, that would be >>> lovely. Otherwise I'll try to figure this out on Monday. >>> >>> Jim >>> >>> >>> On Sep 6, 2013, at 2:25 PM, Malea, Daniel <[email protected]> >>>wrote: >>> >>>> In the original (nested) case, if an inner if-statement fails, the >>>>outer >>>> else-if is never evaluated, whereas in my version, it is. >>>> >>>> On 2013-09-06 5:20 PM, "[email protected]" <[email protected]> wrote: >>>> >>>>> I must be dense today, but I can't see any logical difference between >>>>> the >>>>> version in your patch and the one that was there before. >>>>> >>>>> Jim >>>>> >>>>> On Sep 6, 2013, at 2:03 PM, Malea, Daniel <[email protected]> >>>>> wrote: >>>>> >>>>>> Hi Ed/Jim, >>>>>> >>>>>> After this fix, I'm noticing some buildbot failures in >>>>>> TestInlineStepping.py, which I reproduced locally: >>>>>> >>>>>> Process 25548 stopped >>>>>> * thread #1: tid = 25548, 0x0000000000400721 a.out`main(argc=1, >>>>>> argv=0x00007fffd73b7b38) + 33 at calling.cpp:106, name = 'a.out', >>>>>>stop >>>>>> reason = step over >>>>>> frame #0: 0x0000000000400721 a.out`main(argc=1, >>>>>> argv=0x00007fffd73b7b38) >>>>>> + 33 at calling.cpp:106 >>>>>> 103 >>>>>> 104 inline_value = 0; // Stop here and step over to set up >>>>>> stepping over. >>>>>> 105 >>>>>> -> 106 inline_trivial_1 (); // At inline_trivial_1 called >>>>>>from >>>>>> main. >>>>>> 107 >>>>>> 108 caller_trivial_1(); // At first call of >>>>>>caller_trivial_1 >>>>>> in >>>>>> main. >>>>>> 109 >>>>>> (lldb) thread step-over >>>>>> Process 25548 stopped >>>>>> * thread #1: tid = 25548, 0x0000000000400728 a.out`main [inlined] >>>>>> inline_trivial_2() + 7 at calling.cpp:96, name = 'a.out', stop >>>>>>reason >>>>>> = >>>>>> step over >>>>>> frame #0: 0x0000000000400728 a.out`main [inlined] >>>>>>inline_trivial_2() + >>>>>> 7 >>>>>> at calling.cpp:96 >>>>>> 93 void >>>>>> 94 inline_trivial_2 () >>>>>> 95 { >>>>>> -> 96 inline_value += 1; // In inline_trivial_2. >>>>>> 97 called_by_inline_trivial (); // At caller_by_inline_trivial >>>>>> in >>>>>> inline_trivial_2. >>>>>> 98 } >>>>>> 99 >>>>>> (lldb) >>>>>> >>>>>> >>>>>> >>>>>> It looks like the call to step-over the call on line 106 results in >>>>>> LLDB >>>>>> stopping two levels of inlined functions inside the call to >>>>>> inline_trivial_1(), rather than the line after the call to >>>>>> inline_trivial_1() as the user would expect.. >>>>>> >>>>>> Debugging the broken 'thread step-out' through the location in >>>>>> ThreadPlanStepOverRange.cpp that was modified in 190149, I noticed >>>>>> that >>>>>> while m_addr_context and older_context had equal comp_unit, function >>>>>> and >>>>>> symbol members, the block members were not equal. Due to the way the >>>>>> if-statement is nested, older_ctx_is_equivalent remains false and >>>>>> leads >>>>>> to >>>>>> the buggy behaviour. I have a potential fix (please see attached >>>>>> patch) >>>>>> -- >>>>>> Jim when you have a moment, can you comment on the validity of the >>>>>> fix? >>>>>> I >>>>>> am not seeing any other regressions with my patch applied (with >>>>>>either >>>>>> clang trunk or gcc 4.7). >>>>>> >>>>>> >>>>>> Thanks, >>>>>> Dan >>>>>> >>>>>> On 2013-09-06 8:43 AM, "Ed Maste" <[email protected]> wrote: >>>>>> >>>>>>> Author: emaste >>>>>>> Date: Fri Sep 6 07:43:14 2013 >>>>>>> New Revision: 190149 >>>>>>> >>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=190149&view=rev >>>>>>> Log: >>>>>>> Correct logic error found by inspection. >>>>>>> >>>>>>> From Jim's post on the lldb-dev mailing list: >>>>>>> >>>>>>> This code is there as a backstop for when the unwinder drops a >>>>>>>frame >>>>>>> at >>>>>>> the beginning of new function/trampoline or whatever. >>>>>>> >>>>>>> In the (older_ctx_is_equivalent == false) case we will see if we >>>>>>>are >>>>>>> at >>>>>>> a trampoline function that somebody knows how to get out of, and >>>>>>> otherwise we will stop. >>>>>>> >>>>>>> Modified: >>>>>>> lldb/trunk/source/Target/ThreadPlanStepOverRange.cpp >>>>>>> >>>>>>> Modified: lldb/trunk/source/Target/ThreadPlanStepOverRange.cpp >>>>>>> URL: >>>>>>> >>>>>>> >>>>>>> >>>>>>>http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/ThreadP >>>>>>>la >>>>>>> nS >>>>>>> te >>>>>>> pOverRange.cpp?rev=190149&r1=190148&r2=190149&view=diff >>>>>>> >>>>>>> >>>>>>> >>>>>>>==================================================================== >>>>>>>== >>>>>>> == >>>>>>> == >>>>>>> ==== >>>>>>> --- lldb/trunk/source/Target/ThreadPlanStepOverRange.cpp (original) >>>>>>> +++ lldb/trunk/source/Target/ThreadPlanStepOverRange.cpp Fri Sep 6 >>>>>>> 07:43:14 2013 >>>>>>> @@ -121,7 +121,7 @@ ThreadPlanStepOverRange::ShouldStop (Eve >>>>>>> // in so I left out the target check. And sometimes the >>>>>>> module comes in as the .o file from the >>>>>>> // inlined range, so I left that out too... >>>>>>> >>>>>>> - bool older_ctx_is_equivalent = true; >>>>>>> + bool older_ctx_is_equivalent = false; >>>>>>> if (m_addr_context.comp_unit) >>>>>>> { >>>>>>> if (m_addr_context.comp_unit == >>>>>>> older_context.comp_unit) >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> lldb-commits mailing list >>>>>>> [email protected] >>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits >>>>>> >>>>>> <older_ctx_equivalent_inline_stepping_fix.patch> >>>>> >>>> >>> >> >> <inline_stepping_fix_v2.patch> >
inline_stepping_fix_v4.patch
Description: inline_stepping_fix_v4.patch
_______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
