This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Fix crash for DWARFDie::dump.
ClosedPublic

Authored by ayermolo on Mar 31 2021, 5:49 PM.

Details

Summary

When DIE is extracted manually, the DieArray is empty. When dump is invoked on aforementioned DIE it tries to extract child, even if Dump options say otherwise. Resulting in crash.

Diff Detail

Unit TestsFailed

Event Timeline

ayermolo created this revision.Mar 31 2021, 5:49 PM
ayermolo requested review of this revision.Mar 31 2021, 5:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2021, 5:49 PM
ayermolo edited the summary of this revision. (Show Details)
ayermolo added a comment.EditedApr 5 2021, 9:53 AM

Test case?

I am not sure how to test it. I think only way to trigger it is to manually extract DIE with children like done here: https://github.com/facebookincubator/BOLT/blob/68abc968b706b55585b1b8be315aef5d3bf90b1c/bolt/src/DWARFRewriter.cpp#L129
Then invoke dump() on it.

I guess one way would be to add this method of an extraction to an API, @maksfb or I were thinking of doing it, and modify one of the tools to use it.
Another write a small program that does it and have it as part of the test suite.

My understanding it's done that way vs using current APIs is to keep memory footprint down.

ayermolo added a subscriber: maksfb.Apr 5 2021, 9:57 AM

Test case?

I am not sure how to test it.

How about a unit test? We have some for the DWARF parser already.

Test case?

I am not sure how to test it.

How about a unit test? We have some for the DWARF parser already.

Ah missed it.
Are those YAML tests manually constructed? I tried obj2yaml and copied just debug parts.

  1. parsing failed
  2. Syntax is different then what is in the tests.
    • Name: .debug_info Type: SHT_PROGBITS AddressAlign: 0x1 Content: A6000000040000000000080100000000210086000000000000008F000000700540000000000044000000027005400000000000120000000156D5000000E100000001028C0000000302917CF200000001028C00000000049005400000000000240000000156ED00000001068C00000003029178F600000001068C00000003029170FB0000000106930000000005E900000005040698000000069D00000007A20000000500010000060100

Test case?

I am not sure how to test it.

How about a unit test? We have some for the DWARF parser already.

Ah missed it.
Are those YAML tests manually constructed? I tried obj2yaml

Sorry, I don't know too much about the yaml2obj/obj2yaml stuff - not sure what state it's in @jhenderson and @labath might have more context on the current state.

but there are also pure unit tests that use LLVM APIs to build DWARF and then test that - perhaps one of those would be suitable here?

and copied just debug parts.

  1. parsing failed
  2. Syntax is different then what is in the tests.
    • Name: .debug_info Type: SHT_PROGBITS AddressAlign: 0x1 Content: A6000000040000000000080100000000210086000000000000008F000000700540000000000044000000027005400000000000120000000156D5000000E100000001028C0000000302917CF200000001028C00000000049005400000000000240000000156ED00000001068C00000003029178F600000001068C00000003029170FB0000000106930000000005E900000005040698000000069D00000007A20000000500010000060100
ayermolo updated this revision to Diff 335924.Apr 7 2021, 2:09 PM

Added a test using APIs, moved getHeaderSize from protected to public. Aligns with other get APIs that are already public.

ayermolo updated this revision to Diff 335927.Apr 7 2021, 2:17 PM

nit changes

dblaikie added inline comments.Apr 7 2021, 7:57 PM
llvm/unittests/DebugInfo/DWARF/DWARFDieManualExtractTest.cpp
69

Pass in a str_ostream or the like, and validate that the dump does what the comment above says (dumping only the current DIE and not any children)

ayermolo updated this revision to Diff 337258.Apr 13 2021, 2:15 PM

Updated test with check of dump().

dblaikie accepted this revision.Apr 13 2021, 3:28 PM

Looks good, thanks!

This revision is now accepted and ready to land.Apr 13 2021, 3:28 PM
ayermolo updated this revision to Diff 337325.Apr 13 2021, 8:39 PM

nit: fix up clang tidy

Looks good, thanks!

Thanks for reviewing. Will ask teammates to commit once it passes all the tests. Looks like it failed clang tidy.

ayermolo updated this revision to Diff 337559.Apr 14 2021, 2:16 PM

fixing ling/tidy.

This revision was automatically updated to reflect the committed changes.