[Lldb-commits] [lldb] r190149 - Correct logic error found by inspection.
Malea, Daniel
daniel.malea at intel.com
Fri Sep 13 13:57:39 PDT 2013
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, "jingham at apple.com" <jingham at apple.com> 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 <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/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
>>>>>>> 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>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: inline_stepping_fix_v4.patch
Type: application/octet-stream
Size: 5002 bytes
Desc: inline_stepping_fix_v4.patch
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20130913/40316932/attachment.obj>
More information about the lldb-commits
mailing list