Page MenuHomePhabricator

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

Authored by sohamdixit on Jun 1 2021, 8:59 PM.

Diff Detail

Event Timeline

sohamdixit created this revision.Jun 1 2021, 8:59 PM
sohamdixit requested review of this revision.Jun 1 2021, 8:59 PM
sohamdixit edited the summary of this revision. (Show Details)Jun 1 2021, 9:14 PM
SouraVX added inline comments.Jun 2 2021, 9:46 AM
llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp
329

How about factoring this as:

if (!Data.getData().size())
  OS << "(empty)";
dblaikie added inline comments.Jun 2 2021, 4:30 PM
llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp
329

Probably best to go the whole way:

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

?

aprantl added inline comments.Jun 3 2021, 4:13 PM
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 updated this revision to Diff 349759.Jun 3 2021, 9:01 PM

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

dblaikie added inline comments.Jun 3 2021, 9:22 PM
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.

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

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 accepted this revision.Jun 7 2021, 7:48 AM

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):
This revision is now accepted and ready to land.Jun 7 2021, 7:48 AM
dblaikie accepted this revision.Jun 7 2021, 11:35 AM

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 retitled this revision from Bug 41152 - [DebugInfo] Better dumping of empty location expression to [DebugInfo] Bug 41152 - Improve dumping of empty location expressions.Jun 25 2021, 5:18 AM

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

jhenderson accepted this revision.Jun 28 2021, 12:14 AM

LGTM once my comments have been addressed.

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

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

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

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

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!