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/ThreadPlanS >>>> 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
