[DebugInfo] Bug 41152 - Improve dumping of empty location expressions

[sohamdixit]

Summary

Fixing Bug 41152

https://bugs.llvm.org/show_bug.cgi?id=41152#c0

[SouraVX]

Please clang-format the patch :)
FYI https://llvm.org/docs/Contributing.html#how-to-submit-a-patch

[SouraVX]

llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp

329

How about factoring this as:

if (!Data.getData().size())
  OS << "(empty)";

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp

329

Probably best to go the whole way:

if (Data.getData().empty())
  OS << "(empty)";

?

[aprantl]

llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp

329

Below we're using <> to signal out-of-band errors like <unknown register>, so perhaps this should be "<empty>"

[sohamdixit]

Update Revision D103502: Bug 41152 - [DebugInfo] Better dumping of empty location expression

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp

330

Indentation (please run the change through clang-format (only the changed lines, there's a utility that can do this (reformat only the changed lines) for you - in clang/tools/clang-format/git-clang-format)

And also probably remove the braces - LLVM code doesn't usually use braces on single line blocks.

[sohamdixit]

Update Revision D103502: Bug 41152 - [DebugInfo] Better dumping of empty location expression

[jhenderson]

Mostly looks good to me, but would be nice if one of the earlier reviewers gives thumbs up before we commit this patch.

If I'd written this patch myself, I'd have probably also added a new dedicated test that explicitly checks the output for empty location ranges. It looks like the existing test is producing an empty range as a side-effect of the behaviour under test, and so it would be quite possible that the test coverage would be lost if this beahviour changed/the testing method changed for it. What do others think?

Bug 41152 - [DebugInfo] Better dumping of empty location expression

As a tip for this summary - we usually put tags at the start of the summary, so I'd relabel this as "[DebugInfo] Bug 41152 - Improve dumping of empty location expressions". Note also the slight change of wording too ("improve" is a verb, making the summary a full imperative sentence).

FYI for the other reviewers: as I believe this is @sohamdixit's first LLVM patch to land, I'm happy to push it for him once approval has been given.

[SouraVX]

It looks like the existing test is producing an empty range as a side-effect of the behaviour under test, and so it would be quite possible that the test coverage would be lost if this beahviour changed/the testing method changed for it. What do others think?

+1, I think one small asm test case with just related things (section .debug_loclists and the entry) would be great to have.
Sort of :

$ cat test.s
.Lfunc_begin0:
.Ltmp1:
.section .debug_loclists, "",@progbits
        .long   .Ldebug_list_header_end0-.Ldebug_list_header_start0 # Length
.Ldebug_list_header_start0:
        .short  5                               # Version
        .byte   8                               # Address size
        .byte   0                               # Segment selector size
        .long   1                               # Offset entry count
.Lloclists_table_base0:
        .long   .Ldebug_loc0-.Lloclists_table_base0
.Ldebug_loc0:
        .byte   4                               # DW_LLE_offset_pair
        .uleb128 .Lfunc_begin0-.Lfunc_begin0    #   starting offset
        .uleb128 .Ltmp1-.Lfunc_begin0           #   ending offset
        .byte   0                               ### empty
        .byte   0                               # DW_LLE_end_of_list
.Ldebug_list_header_end0:

Which materializing into existing thing & post this should materialize into <empty>

$  llvm-dwarfdump --debug-loclists test.o
test.o: file format elf64-x86-64

.debug_loclists contents:
locations list header: length = 0x00000011, format = DWARF32, version = 0x0005, addr_size = 0x08, seg_size = 0x00, offset_entry_count = 0x00000001
offsets: [
0x00000004
]
0x00000010:
            DW_LLE_offset_pair     (0x0000000000000000, 0x0000000000000000):

[dblaikie]

In D103502#2801992, @jhenderson wrote:

Mostly looks good to me, but would be nice if one of the earlier reviewers gives thumbs up before we commit this patch.

Sure, I'm OK with it. Hadn't given it a great deal of thought/look other than out of concern for how the DWARF parsing libraries might be impacted.

If I'd written this patch myself, I'd have probably also added a new dedicated test that explicitly checks the output for empty location ranges. It looks like the existing test is producing an empty range as a side-effect of the behaviour under test, and so it would be quite possible that the test coverage would be lost if this beahviour changed/the testing method changed for it. What do others think?

Yep, sounds like a fair point to me.

Bug 41152 - [DebugInfo] Better dumping of empty location expression

As a tip for this summary - we usually put tags at the start of the summary, so I'd relabel this as "[DebugInfo] Bug 41152 - Improve dumping of empty location expressions". Note also the slight change of wording too ("improve" is a verb, making the summary a full imperative sentence).

Also can be nice to write the bug as "PR41152" rather than "Bug 41152" - it's a bit more obscure, but PR* syntax is pretty common in LLVM and a few characters shorter to fit in the commit message limits.

[sohamdixit]

Updated [DebugInfo] Bug 41152 - Improve dumping of empty location expressions

[jhenderson]

LGTM once my comments have been addressed.

llvm/test/MC/X86/dwarf-improve-empty-location-exp.test

1 ↗

(On Diff #354547)

As this test is about testing functionality in the DebugInfo library, I'd move it into llvm/test/DebugInfo/X86.

Also, I'd rename the test file to "dwarf-empty-expression.s":

• In test names, don't use terms like "improve" or "fix" or similar, since the test name should be independent of the commit (i.e. the test will still be relevant long after the commit has been made).
• Use "expression", to avoid abbreviations.
• I don't think you need to be explicit about "location" as the code under test is about the expression printing (expressions may be used for locations, but that's not relevant to the code under test).
• Use ".s" for the extension because ".s" is the file extension used for raw assembly files.

mypatch.patch

1 ↗

(On Diff #354547)

I don't think you meant to add this file to your patch.

[stella.stamenova]

It looks like these change broken some lldb tests. The windows bot already had a couple of failures, so you may have missed it:

https://lab.llvm.org/buildbot/#/builders/83/builds/7757

The newly broken test is debug_loc.s

[jhenderson]

In D103502#2848372, @stella.stamenova wrote:

It looks like these change broken some lldb tests. The windows bot already had a couple of failures, so you may have missed it:

https://lab.llvm.org/buildbot/#/builders/83/builds/7757

The newly broken test is debug_loc.s

Thanks for fixing this!

llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp

bool DWARFExpression::Operation::print(raw_ostream &OS, DIDumpOptions DumpOpts,
   return true;
 }
 
 void DWARFExpression::print(raw_ostream &OS, DIDumpOptions DumpOpts,
                             const MCRegisterInfo *RegInfo, DWARFUnit *U,
                             bool IsEH) const {
   uint32_t EntryValExprSize = 0;
   uint64_t EntryValStartOffset = 0;
+  if (Data.getData().size()==0){
+    OS << "(empty)";
+    }
   for (auto &Op : *this) {
     if (!Op.print(OS, DumpOpts, this, RegInfo, U, IsEH)) {
       uint64_t FailOffset = Op.getEndOffset();
       while (FailOffset < Data.getData().size())
         OS << format(" %02x", Data.getU8(&FailOffset));
       return;
     }
 

llvm/test/MC/X86/dwarf-size-field-overflow.test

 # This test generates too many debug location entries to fit into 65KB required
 # by DWARF < 5. Check that the location is set to 0 instead of crashing.
 #
 # RUN: %python %s 5750 | llc -mtriple=x86_64-apple-darwin -filetype=obj -o - \
 # RUN:    | llvm-dwarfdump - | FileCheck %s
 #
 # CHECK:       0x0000004d:       DW_TAG_formal_parameter
 # CHECK-NEXT:      DW_AT_location   (0x00000000
-# CHECK-NEXT:           [0x0000000000000000,  0x0000000000000008): )
+# CHECK-NEXT:           [0x0000000000000000,  0x0000000000000008): (empty))
 # CHECK-NEXT:      DW_AT_name   ("self")
 
 import sys
 
 SKELETON = """define void @test() !dbg !24 {{
   {}
   call void @foo(), !dbg !34
   ret void, !dbg !34