This is an archive of the discontinued LLVM Phabricator instance.

[dwarfdump] Add DWARF verifiers for address ranges
ClosedPublic

Authored by JDevlieghere on Sep 11 2017, 8:45 AM.

Details

Summary

I started to rebase Greg's differential (D32821) and quickly noticed
that it'd require quite a lot of changes. Rather than blindly fixing the
merge conflicts I started adding the functionality one by one. The
result is both quite similar and different at the same time. This also
gave me a better understanding of the code, which hopefully makes it
easier for me to address any remaining issues.

I've managed to keep everything contained in`DieRangeInfo` (with the
help of the existing DWARFAddressRange). I've done my best to keep
things as analogous as possible between checking for intersection
between an address range within the same DIE and children's address
ranges. The actual checking is also contained to verifyDieRanges.

I've reused all Greg's test so the result should be identical. I've also
ran this on some large internal projects for verification.

Diff Detail

Repository
rL LLVM

Event Timeline

aprantl edited edge metadata.Sep 11 2017, 11:29 AM

Seems generally good. Since David had a lot of comments on the original review, I'll defer to him to accept this.

include/llvm/DebugInfo/DWARF/DWARFVerifier.h
60 ↗(On Diff #114621)

does this comment belong to the next function?

83 ↗(On Diff #114621)

///

lib/DebugInfo/DWARF/DWARFVerifier.cpp
61 ↗(On Diff #114621)

s/fasts/fast./

Thanks Adrian!

dblaikie edited edge metadata.Sep 11 2017, 2:33 PM

Does this handle the interesting cases (test cases are a bit verbose, so I admit I haven't looked through them carefully) that came up in the original review:

Nested functions (a function inside a function - where the inner function shouldn't be contained within the ranges of the outer function (I find this weird, but apparently this is what Adrian & Paul & others think is right, and I can sort of see it - LLVM doesn't produce anything like this))?

Maybe that was the only interesting one...

include/llvm/DebugInfo/DWARF/DWARFVerifier.h
46 ↗(On Diff #114621)

Drop "Ranges(), Children()" from here - they're the default

50 ↗(On Diff #114621)

Drop the "Die()" and "Children()" from here - they're the default anyway?

50 ↗(On Diff #114621)

use "Ranges(std::move(Ranges))" to avoid an unnecessary copy of the Ranges vector

lib/DebugInfo/DWARF/DWARFVerifier.cpp
35–36 ↗(On Diff #114621)

drop else after return

Thanks Dave! There's no logic that deals specifically with nested functions, so I don't think it'll handle this particular case. Before hacking this in, I think it might be good to obtain a real example rather than an artificial one, to ensure we don't make the verification broader than necessary.

Add support and test case for nested functions, which should not be contained by their parent.

Also - how does this work (what does it do, does it catch things correctly) when there are DIEs without ranges - such as compile_unit{namespace{subprogram}} - does it still check that the subprogram is contained within the compile_unit?

lib/DebugInfo/DWARF/DWARFVerifier.cpp
338 ↗(On Diff #115048)

the LLVM style doesn't generally use top-level const on locals - probably best to drop that for consistency

338–341 ↗(On Diff #115048)

Would this correctly check that the nested subprogram is contained within the CU, though? (looks like it just punts on the nested subprogram entirely?)

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
2618–2619 ↗(On Diff #115048)

Wouldn't this need three zero entries? (one to finish each DIE's child list)

Ka-Ka added a subscriber: Ka-Ka.Sep 13 2017, 10:35 AM

Also - how does this work (what does it do, does it catch things correctly) when there are DIEs without ranges - such as compile_unit{namespace{subprogram}} - does it still check that the subprogram is contained within the compile_unit?

Currently it only looks at an entity's direct parent. It will not detect that the subprogram is not part of the compile unit in this situation. If we keep a reference to the parent in each DWARFAddressRange we could walk up the chain until we find the first ancestor with a range. If it's contained this would would guarantee that it's also contained further up.

Would you agree to keep that for a separate diff? This situation would only result in a false-negative and I really think we would benefit from having this functionality checked-in.

lib/DebugInfo/DWARF/DWARFVerifier.cpp
338 ↗(On Diff #115048)

Interesting. What's the reasoning behind this?

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
2618–2619 ↗(On Diff #115048)

Thanks, I modified an existing test but forgot to update this part.

dblaikie accepted this revision.Sep 13 2017, 2:55 PM

Also - how does this work (what does it do, does it catch things correctly) when there are DIEs without ranges - such as compile_unit{namespace{subprogram}} - does it still check that the subprogram is contained within the compile_unit?

Currently it only looks at an entity's direct parent. It will not detect that the subprogram is not part of the compile unit in this situation. If we keep a reference to the parent in each DWARFAddressRange we could walk up the chain until we find the first ancestor with a range. If it's contained this would would guarantee that it's also contained further up.

Would you agree to keep that for a separate diff? This situation would only result in a false-negative and I really think we would benefit from having this functionality checked-in.

Sure, fair enough.

One other thing: maybe revisit the testing (I know you brought that in from the previous patch, but I would've had the same feedback there, once we got past the other issus). It looks like a lot of testing that may be redundant (eg: the specific testing for subprograms V lexical blocks seems like maybe it could be omitted - the code doesn't special case them anyway). Also all the range overlapping checking - is some of that testing redundant or not well reduced? (it looks like full NxMx... sort of testing, when maybe representative examples could be taken without needing the fully combinatorial explosion of checks)

lib/DebugInfo/DWARF/DWARFVerifier.cpp
338 ↗(On Diff #115048)

Isn't considered to add enough safety for the syntactic overhead, I guess. It'd be a /lot/ of const.

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
2618–2619 ↗(On Diff #115048)

Does the test case fail with this mistake? (if it passes, why?)

This revision is now accepted and ready to land.Sep 13 2017, 2:55 PM
This revision was automatically updated to reflect the committed changes.