Page MenuHomePhabricator

[DebugInfo] Unique abbrevs for DIEs with different implicit_const values
ClosedPublic

Authored by vleschuk on Feb 28 2017, 6:00 AM.

Details

Summary

Take DW_FORM_implicit_const attribute value into account when profiling DIEAbbrevData.

Currently if we have two similar types with implicit_const attributes and different values we end up with only one abbrev in .debug_abbrev section. For example consider two structures: S1 with implicit_const attribute ATTR and value VAL1 and S2 with implicit_const ATTR and value VAL2. The .debug_abbrev section will contain only 1 related record:

[N] DW_TAG_structure_type       DW_CHILDREN_yes
        DW_AT_ATTR        DW_FORM_implicit_const  VAL1
        // ....

This is incorrect as struct S2 (with VAL2) will use abbrev record with VAL1.

With this patch we will have two different abbreviations here:

[N] DW_TAG_structure_type       DW_CHILDREN_yes
        DW_AT_ATTR        DW_FORM_implicit_const  VAL1
        // ....

[M] DW_TAG_structure_type       DW_CHILDREN_yes
        DW_AT_ATTR        DW_FORM_implicit_const  VAL2
        // ....

Diff Detail

Event Timeline

vleschuk created this revision.Feb 28 2017, 6:00 AM
dblaikie added inline comments.Feb 28 2017, 8:35 AM
lib/CodeGen/AsmPrinter/DIE.cpp
45

Probably switch this around (don't usually see this kind of reverse comparison in the LLVM codebase):

if (Form == dwarf::DW_FORM_implicit_const)
46

Does this need an unsigned cast like the other calls in this function?

114–117

This should be tested (probably by modifying the existing test that covers implicit_const to verify the value in the abbrev printed here)

unittests/DebugInfo/DWARF/DwarfGenerator.h
135–137 ↗(On Diff #90016)

This seems like a significant API change (exposing this implementation detail) & not really using this API for its original purpose (which was to generate DWARF bytes to test the DWARF parsing APIs, rather than for testing the DWARF generation APIs)

But it's not a bad test, as such... not sure what the right answer is here. Usually the DWARF generation APIs are tested in-situ (in LLVM test cases that exercise them, rather than in unit tests). It might be best to continue to stick with that for now, as much as I do like unit tests.

Well, I guess the parsing APIs need testing too, though - so if you actually make this a round-trip test (then you won't need this new getDIE API) that would be good/acceptable. You might be able to test the dumping support in the unit test too?

vleschuk marked 2 inline comments as done.Feb 28 2017, 9:26 AM
vleschuk added inline comments.
lib/CodeGen/AsmPrinter/DIE.cpp
45

Will do.

46

Nope, Value is signed integer (implicit_const values are always signed according to DWARF standard). And there is proper overloaded AddInteger method for signed ints.

114–117

Actually this is dump method for debugging purposes. If I am not mistaken we do not test debug helpers.

unittests/DebugInfo/DWARF/DwarfGenerator.h
135–137 ↗(On Diff #90016)

Maybe it would be better to proxy AbbrevSet.uniqueAbbreviation() through dwarfgen? In this case we will not expose actual DIE object to caller. What do you think?

Also I am afraid I don't understand how to test dumping from such unit test... Could you please point me in right direction?

vleschuk updated this revision to Diff 90174.Mar 1 2017, 6:29 AM
vleschuk marked 2 inline comments as done.
  • Discarded changes in dwarfgen API
  • Modified test: verify generated DWARF abbrevs using MemoryBuffer and verify dump format using raw_string_ostream
  • Small style changes:

change

DW_FORM_implicit_const == getForm()

to

getForm() == DW_FORM_implicit_const
vleschuk marked 4 inline comments as done.Mar 1 2017, 6:29 AM
dblaikie added inline comments.Mar 1 2017, 10:03 AM
unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
1581–1628

Would it be simpler to visit the DIEs themselves, and verify their attributes represent the correct values?

(I suppose it'd be nice to verify that the two DIEs using the same implicit_const use the same abbreviation, as well)

vleschuk updated this revision to Diff 90221.Mar 1 2017, 1:23 PM

Extended test to verify that two DIEs with equal values refer to the same abbrev, and third DIE with another value refers to another abbrev.

vleschuk marked an inline comment as done.Mar 1 2017, 1:24 PM
vleschuk added inline comments.
unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
1581–1628

Extended test. Left abbrev checking code just to verify that we have exact set of abbrevs that we expect.

vleschuk marked 2 inline comments as done.Mar 1 2017, 1:27 PM
dblaikie accepted this revision.Mar 1 2017, 1:29 PM

I think the tests are probably a bit complicated/long, but should suffice.

Thanks!

This revision is now accepted and ready to land.Mar 1 2017, 1:29 PM
vleschuk closed this revision.Mar 1 2017, 2:27 PM

r296691