[PATCH] D61600: [DebugInfo] More precise variable range for stack locations

[David Stenberg via Phabricator via llvm-commits]

dstenb added inline comments.


================
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:
> > 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61600/new/

https://reviews.llvm.org/D61600