[PATCH] D61600: [DebugInfo] More precise variable range for stack locations
Jeremy Morse via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 7 09:04:49 PDT 2019
jmorse added inline comments.
================
Comment at: lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp:375
unsigned RegNo = DbgValue->getOperand(0).getReg();
- if (TRI->isVirtualRegister(RegNo) || ChangingRegs.test(RegNo))
+ if (TRI->isVirtualRegister(RegNo) || ChangingRegs.test(RegNo) ||
+ FrameReg == RegNo)
----------------
dstenb wrote:
> Perhaps this must not be done in this patch, but I wonder if we should close all register-described values here?
>
> For example:
>
> ```
> volatile int g;
>
> int baz(int p) {
> int local = 123;
>
> if (g > 1) {
> local = p;
> g += p;
> }
>
> return 3;
> }
> ```
>
> when compiling the above with `clang -O1 -g --target=x86_64-unknown-linux-gnu` we land in the following MIR after livedebugvalues:
>
> ```
> bb.0.entry:
> successors: %bb.1(0x40000000), %bb.2(0x40000000)
> liveins: $edi
>
> DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !18
> DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !18
> DBG_VALUE 123, $noreg, !17, !DIExpression(), debug-location !19
> renamable $eax = MOV32rm $rip, 1, $noreg, @g, $noreg, debug-location !20 :: (volatile dereferenceable load 4 from @g, !tbaa !22)
> CMP32ri8 killed renamable $eax, 2, implicit-def $eflags, debug-location !26
> JCC_1 %bb.2, 12, implicit $eflags, debug-location !27
>
> bb.1.if.then:
> successors: %bb.2(0x80000000)
> liveins: $edi
>
> DBG_VALUE 123, $noreg, !17, !DIExpression(), debug-location !19
> DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !18
> DBG_VALUE $edi, $noreg, !17, !DIExpression(), debug-location !19
> ADD32mr $rip, 1, $noreg, @g, $noreg, killed renamable $edi, implicit-def dead $eflags, debug-location !28 :: (volatile store 4 into @g, !tbaa !22), (volatile dereferenceable load 4 from @g, !tbaa !22)
>
> bb.2.if.end:
> DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !18
> $eax = MOV32ri 3, debug-location !31
> RETQ $eax, debug-location !31
> ```
>
> As seen, in the `g > 1` branch the `local` variable is described by edi. As edi is not modified, it will not be contained in `ChangingRegs`.
>
> As we don't close the range here, this means that we'll incorrectly describe `local` using edi for the rest of the function:
>
> ```
> DW_AT_location (0x00000000
> [0x0000000000000000, 0x000000000000000b): DW_OP_consts +123, DW_OP_stack_value
> [0x000000000000000b, 0x0000000000000017): DW_OP_reg5 RDI)
> ```
>
> ```
> 0000000000000000 baz:
> 0: 8b 05 00 00 00 00 movl (%rip), %eax
> 6: 83 f8 02 cmpl $2, %eax
> 9: 7c 06 jl 6 <baz+0x11>
> b: 01 3d 00 00 00 00 addl %edi, (%rip)
> 11: b8 03 00 00 00 movl $3, %eax
> 16: c3 retq
> ```
>
> Perhaps this must not be done in this patch, but I wonder if we should close all register-described values here?
I'd say definite yes -- when I dug into the history [0] for PR41600 (which this patch fixes I think) it sounded like ChangingRegs was a mild hack to work around variable locations being unreliable ~5 years ago. IMHO (YMMV) we might be able to delete it in another patch as a now redundant feature. (Plus as you demonstrate, it can be broken).
[0] https://reviews.llvm.org/D3933
================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1359
+ // TODO: Add support for single value fragment locations.
+ if (Entries.size() == 1 && !MInsn->getDebugExpression()->isFragment()) {
+ const auto *End = HistoryMapEntries.back().isClobber()
----------------
NikolaPrica wrote:
> dstenb wrote:
> > NikolaPrica wrote:
> > > dstenb wrote:
> > > > It seems that this can give incorrect single value locations for cases where we have produced location list entries with empty ranges.
> > > >
> > > > For example:
> > > >
> > > > ```
> > > > frame-setup PUSH64r undef $rax, implicit-def $rsp, implicit $rsp
> > > > DBG_VALUE 123, $noreg, !12, !DIExpression(), debug-location !13
> > > > DBG_VALUE 456, $noreg, !12, !DIExpression(), debug-location !13
> > > > CALL64pcrel32 @foo, csr_64, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, debug-location !14
> > > > $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, debug-location !15
> > > > $rcx = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !15
> > > > RETQ killed $eax, debug-location !15
> > > > ```
> > > >
> > > > Here, the first debug value will result in a location list entry with an empty range, since it is immediately followed by the second, and such entries are omitted from the location list, resulting in a single entry in `Entries` for the constant value 456. However, `MInsn` is the first debug value, so we will create a single location description with the wrong value:
> > > >
> > > >
> > > > ```
> > > > 0x00000043: DW_TAG_variable
> > > > DW_AT_const_value (123)
> > > > ```
> > > >
> > > > I'm not sure what the check should be to avoid these sort of cases. Perhaps check that all debug value entries in `HistoryMapEntries` are described by the same location, and have the same expression?
> > > Such check could be return value of `buildLocationList`? Or we can just take the last `DebugInstr` from `HistoryMapEntries` which could be the last entry or the entry before the last one?
> > > Or we can just take the last DebugInstr from HistoryMapEntries which could be the last entry or the entry before the last one?
> >
> > That is perhaps possible. I tried to think if we can land in a situation where the last `DebugInstr` in `HistoryMapEntries` produces a location list entry with an empty range. For example something like this:
> >
> > ```
> > DBG_VALUE 123, $noreg, !12, !DIExpression(), debug-location !13
> > [...]
> > DBG_VALUE 123, $noreg, !12, !DIExpression(), debug-location !13
> > DBG_VALUE 456, $noreg, !12, !DIExpression(), debug-location !13
> > [end of function]
> > ```
> >
> > However, AFAIK we can't insert debug values after terminators currently (the machine verifier will complain about that), and with a presence of a terminator in the last block then the range will not be empty, so perhaps this is guaranteed? I'm not completely sure.
> I'm not sure either. But I suppose that such situation can appear for local variable that ends at some basic block? Maybe it is safer perform such verification in `buildLocationList` and return it as result?
Great question @ empty ranges; would those be filtered out by this clause [0]?
I believe there are certain circumstances where LLVM won't have a (Machine IR) terminator after a function call declared __attribute__((noreturn)), often at the end of a function. A quick experiment shows that during IR optimisation another part of the compiler places an 'unreachable' inst after the noreturn call and drops any dbg.values, so I doubt this scenario is accessible from source languages.
[0] https://github.com/llvm/llvm-project/blob/da82ce99b7460164ffb841619aadb44f397d2106/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1189
================
Comment at: test/DebugInfo/MIR/X86/dbg-stack-value-range.mir:3-8
+# Test is designed to acknowledge situation when variable is preserved through
+# non-changing stack pointer register location at certain basic block. If the
+# location is non-changing register then DbgEntityHistoryCalculator won't close
+# variable's range at the end of basic block and it will chose to leave it
+# open till the next occurrence of variable's DBG_VALUE or to the end of function.
+# This can lead to ranges stretching more than they should.
----------------
IMHO the second sentence should describe the desired behaviour ("DbgEntityHistoryCalculator should close the variable's range at the end of the basic block..."), then afterwards describe the error that this test is for. It's not obvious right now that the second sentence is about the error case, until you read the third sentence.
================
Comment at: test/DebugInfo/MIR/X86/dbg-stack-value-range.mir:72
+
+ attributes #0 = { nounwind uwtable "no-frame-pointer-elim"="false" "no-frame-pointer-elim-non-leaf" }
+ attributes #1 = { argmemonly nounwind }
----------------
I understand attributes are a pain to maintain in tests; can they be dropped?
================
Comment at: test/DebugInfo/MIR/X86/dbg-stack-value-range.mir:105
+---
+name: foo
+alignment: 4
----------------
Nit - most of the default MIR data can be omitted, pretty much every attribute that's false. (--simplify-mir to llc will drop it automatically).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61600/new/
https://reviews.llvm.org/D61600
More information about the llvm-commits
mailing list