[Lldb-commits] [lldb] r190149 - Correct logic error found by inspection.
Malea, Daniel
daniel.malea at intel.com
Fri Sep 6 15:03:37 PDT 2013
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, "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>
>>>
>>
>
More information about the lldb-commits
mailing list