[Lldb-commits] [lldb] r190149 - Correct logic error found by inspection.
jingham at apple.com
jingham at apple.com
Mon Sep 9 16:26:39 PDT 2013
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 <daniel.malea at intel.com> 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, "jingham at apple.com" <jingham at apple.com> 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 <daniel.malea at intel.com> 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, "jingham at apple.com" <jingham at apple.com> 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 <daniel.malea at intel.com>
>>>> 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" <emaste at freebsd.org> 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
>>>>>> lldb-commits at cs.uiuc.edu
>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>>>>>
>>>>> <older_ctx_equivalent_inline_stepping_fix.patch>
>>>>
>>>
>>
>
> <inline_stepping_fix_v2.patch>
More information about the lldb-commits
mailing list