llvm-dwarfdump: automatically dump both regular and .dwo variant of sections
ClosedPublic

Authored by aprantl on Tue, Sep 12, 1:56 PM.

Details

Summary

Since users typically don't really care about the .dwo / non.dwo distinction, this patch makes it so dwarfdump --debug-<info,...> dumps .debug_info and (if available) also .debug_info.dwo. This simplifies the command line interface (I've removed all dwo-specific dump options) and makes the tool friendlier to use.

Diff Detail

Repository
rL LLVM
aprantl created this revision.Tue, Sep 12, 1:56 PM

Some thoughts/suggestions

lib/DebugInfo/DWARF/DWARFContext.cpp
367–370 ↗(On Diff #114905)

Should this condition be sunk into dumpStringOffsetsSection?

test/DebugInfo/X86/fission-no-inlining.ll
5 ↗(On Diff #114905)

Maybe "CHECK: contents:" So it's trivially obvious it's only checking for subprogram in the debug_info section?

test/DebugInfo/X86/generate-odr-hash.ll
77 ↗(On Diff #114905)

Not following this change - it looks like it's checking for types in debug_types instead of debug_types.dwo?

130 ↗(On Diff #114905)

Also probably "CHECK-NOT: contents:"

test/DebugInfo/X86/live-debug-variables.ll
30 ↗(On Diff #114905)

Should we print the header if there's nothing in the section? I've know we do it for some sections and not for others - and kind of think maybe we shouldn't do it for any sections.

test/DebugInfo/X86/split-dwarf-multiple-cu-hash.ll
9 ↗(On Diff #114905)

CHECK: contents:

test/DebugInfo/X86/split-dwarf-omit-empty.ll
21 ↗(On Diff #114905)

CHECK: contents:

aprantl updated this revision to Diff 114912.Tue, Sep 12, 2:30 PM
aprantl marked 4 inline comments as done.

Address review feedback.

lib/DebugInfo/DWARF/DWARFContext.cpp
367–370 ↗(On Diff #114905)

No, it shouldn't be there at all to mirror all the other cases.

test/DebugInfo/X86/generate-odr-hash.ll
130 ↗(On Diff #114905)

No, it looks like there's another section in between in some of the FileCheck runs using the same CHECK prefix.

test/DebugInfo/X86/live-debug-variables.ll
30 ↗(On Diff #114905)

Currently we print the section header when the section was explicitly requested (either through --debug-loc or --all). We only print the .dwo section header when the non-dwo variant was requested and it is non-empty. My selfish thinking behind this decision was that we don't want the .dwo sections to show up at all on platforms that don't use DWOs.

probinson added inline comments.Tue, Sep 12, 2:41 PM
lib/DebugInfo/DWARF/DWARFContext.cpp
213 ↗(On Diff #114905)

IIUC dumping a .dwo file, we'll see:

.debug_abbrev contents:

.debug_abbrev.dwo contents:
<stuff>

etc for each section type, which seems a little weird.

When dumping a .o file without a .debug_types section, should --debug-types

  1. print nothing
  2. print .debug_types:\nEOF

If the answer is (2), how should it behave on an empty .dwo file?

UI is tricky. I think I lean in the direction that if the section doesn't exist, don't print anything, even the "contents:" line.
This is certainly how I want --all to behave, and obviously it's easiest and self-consistent if that's how the tool always behaves. So for example --debug-types when there is no .debug_types section should print nothing.
I could see an argument that if I explicitly asked for a particular section and it doesn't exist, I should get the "contents:" line with nothing after it. But that's really not how I want --all to behave.

Okay, I'll implement a "don't print anything if the section doesn't exist" approach, since that seems to align best with what everybody desires. Printing an empty "contents" only if the section was explicitly required is trickier to implement, so I'll leave this for a future improvement.

probinson added inline comments.Wed, Sep 13, 11:44 AM
include/llvm/DebugInfo/DIContext.h
127 ↗(On Diff #114912)

I am reminded that MSVC traditionally has not correctly handled enum values wider than 32 bits, so I'm happy to see this change and apologize for not noticing it in the original patch that added DIDumpType.
Or has MSVC learned to do 64-bit enum values? It used to just silently truncate to 32 bits.

rnk added inline comments.Wed, Sep 13, 11:48 AM
include/llvm/DebugInfo/DIContext.h
127 ↗(On Diff #114912)

Microsoft never implemented the rules about deducing the type of the enum from the enumerator values. It would be an ABI break in some ways if they did that today. You could argue that truncating enumerators that were too wide was already a bug, but that's the status.

Instead, they implemented strong enums as a way to let the user explicitly say "yeah, I really want a uint64_t", so this code should work fine.

probinson added inline comments.Wed, Sep 13, 11:55 AM
include/llvm/DebugInfo/DIContext.h
127 ↗(On Diff #114912)

Good to know, thanks!

aprantl updated this revision to Diff 115111.Wed, Sep 13, 1:58 PM

With this update:

  1. llvm-dwarfdump --all prints only section headers (like .debug_info contents:) of non-empty sections.

2a. llvm-dwarfdump --<debug-info, ...> always prints a section header
2b. On platforms that support DWOs, llvm-dwarfdump --<debug-info ..> also always prints a section header for the corresponding .dwo section.

aprantl updated this revision to Diff 115116.Wed, Sep 13, 2:19 PM

Now comes with ROCK-SOLID .dwo detection!

dblaikie added inline comments.Wed, Sep 13, 2:26 PM
lib/DebugInfo/DWARF/DWARFContext.cpp
242–244 ↗(On Diff #115116)

Ah, so one extra wrinkle is that dwo sections can appear in non-dwo files...

For example when LLVM (& GCC) produce Fission output, they first output a single object file (with all the dwo and non-dwo, and text, etc) then use objcopy to extract the .dwo sections out into a separate file.

So not dumping DWO sections when present, even when not examining a .dwo file, is problematic. But they can silently not have headers (ie: if you ask to dump a types in a .o file, I don't think there's a need to print the types.dwo header if types.dwo is not present) - I think that's OK.

Does that make sense?

aprantl updated this revision to Diff 115119.Wed, Sep 13, 2:27 PM

use sys::path instead

aprantl added inline comments.Wed, Sep 13, 2:35 PM
lib/DebugInfo/DWARF/DWARFContext.cpp
242–244 ↗(On Diff #115116)

I think so. The behavior as implemented is:

When specifically requesting a section:

  • in a .o file: empty .dwo section headers are skipped
  • in a .dwo file: empty .dwo section headers are printed
  • in both cases: nonempty .dwo sections are dumped.

I think this is what we want, right?

probinson added inline comments.Wed, Sep 13, 2:36 PM
lib/DebugInfo/DWARF/DWARFContext.cpp
243 ↗(On Diff #115119)

Hmmm now you are attaching a semantic significance to what is just a naming convention. I don't think that's appropriate.
We should not worry about what "kind" of file it is, just whether the sections exist or not.

aprantl updated this revision to Diff 115120.Wed, Sep 13, 2:36 PM

Clarify comment.

aprantl added inline comments.Wed, Sep 13, 2:40 PM
lib/DebugInfo/DWARF/DWARFContext.cpp
243 ↗(On Diff #115119)

Please note that this affects only whether we print *section headers for empty sections*.
The idea behind this condition is that we want to hide empty .dwo section headers when dumping a non-.dwo file. (With the one wrinkle that the intermediate products of clang that contain both .dwo and non-dwo sections are counted as non-dwo).

aprantl updated this revision to Diff 115126.Wed, Sep 13, 2:47 PM

Implement David's suggestion to skip empty non-dwo section headers in .dwo files.

dblaikie accepted this revision.Wed, Sep 13, 2:52 PM

Sounds good to me

This revision is now accepted and ready to land.Wed, Sep 13, 2:52 PM
probinson added inline comments.Wed, Sep 13, 2:52 PM
lib/DebugInfo/DWARF/DWARFContext.cpp
243 ↗(On Diff #115119)

Ok, I get that it's just a heuristic for deciding when to suppress headers for empty sections.
If you make it match .dwp as well as .dwo file extensions, I can live with that.

This revision was automatically updated to reflect the committed changes.