This is an archive of the discontinued LLVM Phabricator instance.

[dsymutil] Don't clone empty CUs
ClosedPublic

Authored by JDevlieghere on Feb 8 2019, 2:38 PM.

Details

Summary

The DWARF standard says that an empty compile unit is not valid:

Each such contribution consists of a compilation unit header (see Section 7.5.1.1 on page 200) followed by a single DW_TAG_compile_unit or DW_TAG_partial_unit debugging information entry, together with its children.

Therefore we shouldn't clone them in dsymutil.

Diff Detail

Event Timeline

JDevlieghere created this revision.Feb 8 2019, 2:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2019, 2:38 PM
JDevlieghere retitled this revision from [dsymutil] Update unobfuscation logic. to [dsymutil] Don't clone empty CUs.Feb 8 2019, 2:38 PM
JDevlieghere edited the summary of this revision. (Show Details)Feb 8 2019, 2:44 PM
friss added a comment.Feb 8 2019, 2:47 PM

I might be missing something, but empty as in "has no DIEs at all" is invalid. I don't think having one DIE with no children counts as invalid (it might not be really useful, but this review is about it being invalid, right?)

You mean an empty compile unit *header* is not valid, right?

Only skip empty CUs, not DW_TAG_compile_units without children.

aprantl added inline comments.Feb 8 2019, 4:19 PM
llvm/tools/dsymutil/CompileUnit.cpp
60

@friss Do you remember why that comment is there? Isn't a header without a DW_TAG_compile_unit invalid DWARF?

llvm/tools/dsymutil/DwarfLinker.cpp
2274

This change looks reasonable.

dblaikie added inline comments.
llvm/test/tools/dsymutil/X86/generate-empty-CU.test
5

This sentence seems incomplete ("Dsymutil should not generate an empty CU and it", "and it" what?)

friss added inline comments.Feb 12 2019, 8:49 AM
llvm/tools/dsymutil/CompileUnit.cpp
60

Must have been because dsymutil-classic was doing it. I don't recall any good reason we would generate this.

aprantl accepted this revision.Feb 12 2019, 4:16 PM
aprantl added inline comments.
llvm/tools/dsymutil/DwarfLinker.cpp
2259

This looks like an unrelated NFC commit?

This revision is now accepted and ready to land.Feb 12 2019, 4:16 PM
This revision was automatically updated to reflect the committed changes.