Re: [lldb-dev] [llvm] r343874 - DwarfDebug: Pick next location in case of missing location at block begin
Sadly, after this commit TestDataFormatterLibcxxList.py started failing with:
```
output: Process 67333 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = step over
frame #0: 0x00010eb0 a.out`main at main.cpp:33:16
30 (text_list.push_back(std::string("smart")));
31
32 printf("// Set second break point at this line.");
-> 33 (text_list.push_back(std::string("!!!")));
^
34
35 std::list countingList = {3141, 3142, 3142,3142,3142, 3142,
3142, 3141};
36 countingList.sort();
runCmd: frame variable text_list[0]
output: (std::__1::basic_string,
std::__1::allocator >) [0] = "goofy"
Expecting sub string: goofy
Matched
runCmd: frame variable text_list[3]
output: None
Expecting sub string: !!!
```
URL: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/10854
I confirmed that reverting this fixes the issue.
I think the problem is that we’re breaking on an instruction that looks like
it’s on line 33, but it’s actually “before” the full effect of the push_back()
is visible.
In which case the test is wrong, because it’s setting the breakpoint too early…
vedant
> On Oct 5, 2018, at 11:29 AM, Matthias Braun via llvm-commits
> wrote:
>
> Author: matze
> Date: Fri Oct 5 11:29:24 2018
> New Revision: 343874
>
> URL: http://llvm.org/viewvc/llvm-project?rev=343874&view=rev
> Log:
> DwarfDebug: Pick next location in case of missing location at block begin
>
> Context: Compiler generated instructions do not have a debug location
> assigned to them. However emitting 0-line records for all of them bloats
> the line tables for very little benefit so we usually avoid doing that.
>
> Not emitting anything will lead to the previous debug location getting
> applied to the locationless instructions. This is not desirable for
> block begin and after labels. Previously we would emit simply emit
> line-0 records in this case, this patch changes the behavior to do a
> forward search for a debug location in these cases before emitting a
> line-0 record to further reduce line table bloat.
>
> Inspired by the discussion in https://reviews.llvm.org/D52862
>
> Added:
>llvm/trunk/test/DebugInfo/X86/dwarf-no-source-loc.mir
> Modified:
>llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h
>llvm/trunk/test/DebugInfo/AArch64/line-header.ll
>llvm/trunk/test/DebugInfo/ARM/single-constant-use-preserves-dbgloc.ll
>llvm/trunk/test/DebugInfo/Mips/delay-slot.ll
>llvm/trunk/test/DebugInfo/NVPTX/debug-info.ll
>llvm/trunk/test/DebugInfo/X86/dwarf-no-source-loc.ll
>
> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=343874&r1=343873&r2=343874&view=diff
> ==
> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)
> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Fri Oct 5 11:29:24 2018
> @@ -1313,6 +1313,49 @@ void DwarfDebug::collectEntityInfo(Dwarf
> }
> }
>
> +static const DebugLoc &
> +findNextDebugLoc(MachineBasicBlock::const_iterator MBBI,
> + MachineBasicBlock::const_iterator MBBE) {
> + static DebugLoc NoLocation;
> + for ( ; MBBI != MBBE; ++MBBI) {
> +if (MBBI->isDebugInstr())
> + continue;
> +const DebugLoc &DL = MBBI->getDebugLoc();
> +if (DL)
> + return DL;
> + }
> + return NoLocation;
> +}
> +
> +void DwarfDebug::emitDebugLoc(const DebugLoc &DL) {
> + unsigned LastAsmLine =
> + Asm->OutStreamer->getContext().getCurrentDwarfLoc().getLine();
> +
> + // We have an explicit location, different from the previous location.
> + // Don't repeat a line-0 record, but otherwise emit the new location.
> + // (The new location might be an explicit line 0, which we do emit.)
> + unsigned Line = DL.getLine();
> + if (PrevInstLoc && Line == 0 && LastAsmLine == 0)
> +return;
> + unsigned Flags = 0;
> + if (DL == PrologEndLoc) {
> +Flags |= DWARF2_FLAG_PROLOGUE_END | DWARF2_FLAG_IS_STMT;
> +PrologEndLoc = DebugLoc();
> + }
> + // If the line changed, we call that a new statement; unless we went to
> + // line 0 and came back, in which case it is not a new statement.
> + unsigned OldLine = PrevInstLoc ? PrevInstLoc.getLine() : LastAsmLine;
> + if (Line && Line != OldLine)
> +Flags |= DWARF2_FLAG_IS_STMT;
> +
> + const MDNode *Scope = DL.getScope();
> + recordSourceLine(Line, DL.getCol(), Scope, Flags);
> +
> + // If we're not at line 0, remember this location.
> + if (Line)
> +PrevInstLoc = DL;
> +}
> +
> // Process beginning of an instruction.
> void DwarfDebug::beginInstruction(const MachineInstr *MI) {
> DebugHandlerBase::beginInstruction(MI);
> @@ -1352,54 +1395,41 @@ void DwarfDebug::beginInstructio
Re: [lldb-dev] [llvm] r343874 - DwarfDebug: Pick next location in case of missing location at block begin
So what should we do? Revert the llvm commit, fix the LLDB test, xfail on lldb?
I'd be fine with any but don't want to learn how lldb tests work at this
moment...
> On Oct 5, 2018, at 4:11 PM, Vedant Kumar via llvm-commits
> wrote:
>
> Sadly, after this commit TestDataFormatterLibcxxList.py started failing with:
>
> ```
> output: Process 67333 stopped
> * thread #1, queue = 'com.apple.main-thread', stop reason = step over
> frame #0: 0x00010eb0 a.out`main at main.cpp:33:16
>30 (text_list.push_back(std::string("smart")));
>31
>32 printf("// Set second break point at this line.");
> -> 33 (text_list.push_back(std::string("!!!")));
> ^
>34
>35 std::list countingList = {3141, 3142, 3142,3142,3142,
> 3142, 3142, 3141};
>36 countingList.sort();
>
>
> runCmd: frame variable text_list[0]
> output: (std::__1::basic_string,
> std::__1::allocator >) [0] = "goofy"
>
> Expecting sub string: goofy
> Matched
>
> runCmd: frame variable text_list[3]
> output: None
>
> Expecting sub string: !!!
> ```
>
> URL: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/10854
>
>
> I confirmed that reverting this fixes the issue.
>
> I think the problem is that we’re breaking on an instruction that looks like
> it’s on line 33, but it’s actually “before” the full effect of the
> push_back() is visible.
>
> In which case the test is wrong, because it’s setting the breakpoint too
> early…
>
> vedant
>
>> On Oct 5, 2018, at 11:29 AM, Matthias Braun via llvm-commits
>> mailto:llvm-comm...@lists.llvm.org>> wrote:
>>
>> Author: matze
>> Date: Fri Oct 5 11:29:24 2018
>> New Revision: 343874
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=343874&view=rev
>>
>> Log:
>> DwarfDebug: Pick next location in case of missing location at block begin
>>
>> Context: Compiler generated instructions do not have a debug location
>> assigned to them. However emitting 0-line records for all of them bloats
>> the line tables for very little benefit so we usually avoid doing that.
>>
>> Not emitting anything will lead to the previous debug location getting
>> applied to the locationless instructions. This is not desirable for
>> block begin and after labels. Previously we would emit simply emit
>> line-0 records in this case, this patch changes the behavior to do a
>> forward search for a debug location in these cases before emitting a
>> line-0 record to further reduce line table bloat.
>>
>> Inspired by the discussion in https://reviews.llvm.org/D52862
>>
>>
>> Added:
>>llvm/trunk/test/DebugInfo/X86/dwarf-no-source-loc.mir
>> Modified:
>>llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h
>>llvm/trunk/test/DebugInfo/AArch64/line-header.ll
>>llvm/trunk/test/DebugInfo/ARM/single-constant-use-preserves-dbgloc.ll
>>llvm/trunk/test/DebugInfo/Mips/delay-slot.ll
>>llvm/trunk/test/DebugInfo/NVPTX/debug-info.ll
>>llvm/trunk/test/DebugInfo/X86/dwarf-no-source-loc.ll
>>
>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=343874&r1=343873&r2=343874&view=diff
>>
>>
>> ==
>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Fri Oct 5 11:29:24 2018
>> @@ -1313,6 +1313,49 @@ void DwarfDebug::collectEntityInfo(Dwarf
>> }
>> }
>>
>> +static const DebugLoc &
>> +findNextDebugLoc(MachineBasicBlock::const_iterator MBBI,
>> + MachineBasicBlock::const_iterator MBBE) {
>> + static DebugLoc NoLocation;
>> + for ( ; MBBI != MBBE; ++MBBI) {
>> +if (MBBI->isDebugInstr())
>> + continue;
>> +const DebugLoc &DL = MBBI->getDebugLoc();
>> +if (DL)
>> + return DL;
>> + }
>> + return NoLocation;
>> +}
>> +
>> +void DwarfDebug::emitDebugLoc(const DebugLoc &DL) {
>> + unsigned LastAsmLine =
>> + Asm->OutStreamer->getContext().getCurrentDwarfLoc().getLine();
>> +
>> + // We have an explicit location, different from the previous location.
>> + // Don't repeat a line-0 record, but otherwise emit the new location.
>> + // (The new location might be an explicit line 0, which we do emit.)
>> + unsigned Line = DL.getLine();
>> + if (PrevInstLoc && Line == 0 && LastAsmLine == 0)
>> +return;
>> + unsigned Flags = 0;
>> + if (DL == PrologEndLoc) {
>> +Flags |= DWARF2_FLAG_PROLOGUE_END | DWARF2_FLAG_IS_STMT;
>> +PrologEndLoc = DebugLoc();
>> +
Re: [lldb-dev] [llvm] r343874 - DwarfDebug: Pick next location in case of missing location at block begin
No worries, I’ve relaxed the test for now in r343899 to get the bots going
again. Essentially, the test will just step one more time to make sure the full
effect of the push_back() is visible.
If folks on lldb-dev (cc’d) have a better idea we can try it out.
vedant
> On Oct 5, 2018, at 4:15 PM, Matthias Braun wrote:
>
> So what should we do? Revert the llvm commit, fix the LLDB test, xfail on
> lldb? I'd be fine with any but don't want to learn how lldb tests work at
> this moment...
>
>> On Oct 5, 2018, at 4:11 PM, Vedant Kumar via llvm-commits
>> mailto:llvm-comm...@lists.llvm.org>> wrote:
>>
>> Sadly, after this commit TestDataFormatterLibcxxList.py started failing with:
>>
>> ```
>> output: Process 67333 stopped
>> * thread #1, queue = 'com.apple.main-thread', stop reason = step over
>> frame #0: 0x00010eb0 a.out`main at main.cpp:33:16
>>30(text_list.push_back(std::string("smart")));
>>31
>>32printf("// Set second break point at this line.");
>> -> 33(text_list.push_back(std::string("!!!")));
>> ^
>>34
>>35std::list countingList = {3141, 3142, 3142,3142,3142,
>> 3142, 3142, 3141};
>>36countingList.sort();
>>
>>
>> runCmd: frame variable text_list[0]
>> output: (std::__1::basic_string,
>> std::__1::allocator >) [0] = "goofy"
>>
>> Expecting sub string: goofy
>> Matched
>>
>> runCmd: frame variable text_list[3]
>> output: None
>>
>> Expecting sub string: !!!
>> ```
>>
>> URL: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/10854
>>
>>
>> I confirmed that reverting this fixes the issue.
>>
>> I think the problem is that we’re breaking on an instruction that looks like
>> it’s on line 33, but it’s actually “before” the full effect of the
>> push_back() is visible.
>>
>> In which case the test is wrong, because it’s setting the breakpoint too
>> early…
>>
>> vedant
>>
>>> On Oct 5, 2018, at 11:29 AM, Matthias Braun via llvm-commits
>>> mailto:llvm-comm...@lists.llvm.org>> wrote:
>>>
>>> Author: matze
>>> Date: Fri Oct 5 11:29:24 2018
>>> New Revision: 343874
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=343874&view=rev
>>>
>>> Log:
>>> DwarfDebug: Pick next location in case of missing location at block begin
>>>
>>> Context: Compiler generated instructions do not have a debug location
>>> assigned to them. However emitting 0-line records for all of them bloats
>>> the line tables for very little benefit so we usually avoid doing that.
>>>
>>> Not emitting anything will lead to the previous debug location getting
>>> applied to the locationless instructions. This is not desirable for
>>> block begin and after labels. Previously we would emit simply emit
>>> line-0 records in this case, this patch changes the behavior to do a
>>> forward search for a debug location in these cases before emitting a
>>> line-0 record to further reduce line table bloat.
>>>
>>> Inspired by the discussion in https://reviews.llvm.org/D52862
>>>
>>>
>>> Added:
>>>llvm/trunk/test/DebugInfo/X86/dwarf-no-source-loc.mir
>>> Modified:
>>>llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>>llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h
>>>llvm/trunk/test/DebugInfo/AArch64/line-header.ll
>>>llvm/trunk/test/DebugInfo/ARM/single-constant-use-preserves-dbgloc.ll
>>>llvm/trunk/test/DebugInfo/Mips/delay-slot.ll
>>>llvm/trunk/test/DebugInfo/NVPTX/debug-info.ll
>>>llvm/trunk/test/DebugInfo/X86/dwarf-no-source-loc.ll
>>>
>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=343874&r1=343873&r2=343874&view=diff
>>>
>>>
>>> ==
>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)
>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Fri Oct 5 11:29:24
>>> 2018
>>> @@ -1313,6 +1313,49 @@ void DwarfDebug::collectEntityInfo(Dwarf
>>> }
>>> }
>>>
>>> +static const DebugLoc &
>>> +findNextDebugLoc(MachineBasicBlock::const_iterator MBBI,
>>> + MachineBasicBlock::const_iterator MBBE) {
>>> + static DebugLoc NoLocation;
>>> + for ( ; MBBI != MBBE; ++MBBI) {
>>> +if (MBBI->isDebugInstr())
>>> + continue;
>>> +const DebugLoc &DL = MBBI->getDebugLoc();
>>> +if (DL)
>>> + return DL;
>>> + }
>>> + return NoLocation;
>>> +}
>>> +
>>> +void DwarfDebug::emitDebugLoc(const DebugLoc &DL) {
>>> + unsigned LastAsmLine =
>>> + Asm->OutStreamer->getContext().getCurrentDwarfLoc().getLine();
>>> +
>>> + // We hav
Re: [lldb-dev] [llvm] r343874 - DwarfDebug: Pick next location in case of missing location at block begin
So in the test scenario, we have this code:
printf("// Set second break point at this line.");
(text_list.push_back(std::string("!!!")));
and we have a breakpoint on the printf line. We've just continued to hit the
breakpoint at printf. Then we do next twice. That should certainly get us
past the push_back. If it does not that's either a bug in the line tables or
in lldb's handling of them. You would not expect to have to next three times
to go from the start of the printf line to past the push_back of !!!.
Considered as a test about stepping, we should applaud the test for having
caught a bug, and go figure out whether the line tables are bogus in this
instance or just innovative and lldb will have to cope...
Considered as a test about data formatters, it is a little silly to try to
drive it using next's since the push_backs are going to introduce a bunch of
inlining and the debug information for inlining is often a bit wonky...
Best course is to use the breakpoints in this test to drive from point to
point, and make a reduced stepping test case that just shows this bad stepping
behavior. Then we can fix the latter test failure either in clang or lldb as
is appropriate.
Jim
> On Oct 5, 2018, at 4:18 PM, Vedant Kumar via lldb-dev
> wrote:
>
> No worries, I’ve relaxed the test for now in r343899 to get the bots going
> again. Essentially, the test will just step one more time to make sure the
> full effect of the push_back() is visible.
>
> If folks on lldb-dev (cc’d) have a better idea we can try it out.
>
> vedant
>
>> On Oct 5, 2018, at 4:15 PM, Matthias Braun wrote:
>>
>> So what should we do? Revert the llvm commit, fix the LLDB test, xfail on
>> lldb? I'd be fine with any but don't want to learn how lldb tests work at
>> this moment...
>>
>>> On Oct 5, 2018, at 4:11 PM, Vedant Kumar via llvm-commits
>>> wrote:
>>>
>>> Sadly, after this commit TestDataFormatterLibcxxList.py started failing
>>> with:
>>>
>>> ```
>>> output: Process 67333 stopped
>>> * thread #1, queue = 'com.apple.main-thread', stop reason = step over
>>> frame #0: 0x00010eb0 a.out`main at main.cpp:33:16
>>>30 (text_list.push_back(std::string("smart")));
>>>31
>>>32 printf("// Set second break point at this line.");
>>> -> 33 (text_list.push_back(std::string("!!!")));
>>>^
>>>34
>>>35 std::list countingList = {3141, 3142, 3142,3142,3142,
>>> 3142, 3142, 3141};
>>>36 countingList.sort();
>>>
>>>
>>> runCmd: frame variable text_list[0]
>>> output: (std::__1::basic_string,
>>> std::__1::allocator >) [0] = "goofy"
>>>
>>> Expecting sub string: goofy
>>> Matched
>>>
>>> runCmd: frame variable text_list[3]
>>> output: None
>>>
>>> Expecting sub string: !!!
>>> ```
>>>
>>> URL: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/10854
>>>
>>> I confirmed that reverting this fixes the issue.
>>>
>>> I think the problem is that we’re breaking on an instruction that looks
>>> like it’s on line 33, but it’s actually “before” the full effect of the
>>> push_back() is visible.
>>>
>>> In which case the test is wrong, because it’s setting the breakpoint too
>>> early…
>>>
>>> vedant
>>>
On Oct 5, 2018, at 11:29 AM, Matthias Braun via llvm-commits
wrote:
Author: matze
Date: Fri Oct 5 11:29:24 2018
New Revision: 343874
URL: http://llvm.org/viewvc/llvm-project?rev=343874&view=rev
Log:
DwarfDebug: Pick next location in case of missing location at block begin
Context: Compiler generated instructions do not have a debug location
assigned to them. However emitting 0-line records for all of them bloats
the line tables for very little benefit so we usually avoid doing that.
Not emitting anything will lead to the previous debug location getting
applied to the locationless instructions. This is not desirable for
block begin and after labels. Previously we would emit simply emit
line-0 records in this case, this patch changes the behavior to do a
forward search for a debug location in these cases before emitting a
line-0 record to further reduce line table bloat.
Inspired by the discussion in https://reviews.llvm.org/D52862
Added:
llvm/trunk/test/DebugInfo/X86/dwarf-no-source-loc.mir
Modified:
llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.h
llvm/trunk/test/DebugInfo/AArch64/line-header.ll
llvm/trunk/test/DebugInfo/ARM/single-constant-use-preserves-dbgloc.ll
llvm/trunk/test/DebugInfo/Mips/delay-slot.ll
llvm/trunk/test/DebugInfo/NVPTX/debug-info.ll
llvm/trunk/test/DebugInfo/X86/dwarf-no-source-loc.ll
Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
URL:
http://llvm.org/viewvc/llvm-p
