Thanks for the feedback Jim. I traced through the code and noted m_addr_context and older_context had equal comp_unit, function members, but the block members were not equal. That is, the check on ThreadPlanStepOver.cpp:131 (in the original version) was failing.
In this test-case there were two levels of inlining going on, if that helps.. Thanks, 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/ThreadPla >>>>>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> >>> >> > _______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
