This is an archive of the discontinued LLVM Phabricator instance.

[llvm-dwarfdump] Verify compatible TAG for attributes.
ClosedPublic

Authored by JDevlieghere on Oct 10 2017, 3:49 AM.

Details

Summary

Adds a verification that DW_AT_specification and DW_AT_abstract origin
reference a DIE with a compatible tag.

Diff Detail

Event Timeline

JDevlieghere created this revision.Oct 10 2017, 3:49 AM
dblaikie added inline comments.Oct 10 2017, 7:33 AM
lib/DebugInfo/DWARF/DWARFVerifier.cpp
423 ↗(On Diff #118340)

Can you use DWARFDie::getAttributeValueAsReferencedDie here, perhaps?

test/tools/llvm-dwarfdump/X86/verify_compatible_tags.s
23–24 ↗(On Diff #118340)

Maybe this should have some more words about which one was expected & which one was actually found? (& maybe dumping whose DIEs or some reference to the DIE so the user knows where to look in the DWARF to find the DIEs in question?)

  • Better error message
JDevlieghere marked 2 inline comments as done.Oct 10 2017, 10:06 AM
JDevlieghere added inline comments.
lib/DebugInfo/DWARF/DWARFVerifier.cpp
423 ↗(On Diff #118340)

Unfortunately not, because this makes use of getUnitForOffset which in turn uses a list of units that isn't build when doing the verification.
Furthermore, this is (probably) slightly more efficient as we already have the attribute value and therefore do not need to do any indirections via the abbrev declaration.

aprantl added inline comments.Oct 10 2017, 10:07 AM
lib/DebugInfo/DWARF/DWARFVerifier.cpp
422 ↗(On Diff #118410)

tags

aprantl accepted this revision.Oct 10 2017, 10:09 AM

LGTM with outstanding nitpicks addressed and assuming David is happy with the patch.

lib/DebugInfo/DWARF/DWARFVerifier.cpp
434 ↗(On Diff #118410)

<< "" ?

This revision is now accepted and ready to land.Oct 10 2017, 10:09 AM
aprantl added inline comments.Oct 10 2017, 10:10 AM
test/tools/llvm-dwarfdump/X86/verify_compatible_tags.s
23 ↗(On Diff #118410)

can you use CHECK-NEXT here to make the test even stricter?

JDevlieghere marked 4 inline comments as done.
  • Feedback Adrian
  • Use SmallSet<DWARFDie, 3> (running dsymutil over clang showed that there were never more than 3 elements in the set for valid DWARF).

Wrong diff got updated, back to previous version.

JDevlieghere added a reviewer: probinson.

Rebase & address Dave's comment (via the mailing list).

Use separate vectors for compile and type units.

dblaikie added inline comments.Sep 18 2018, 11:03 AM
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
115–119 ↗(On Diff #165769)

This probably needs to ensure the newly added unit is added such that the vector remains in sorted order (so getUnitForOffset, etc, can rely on that sorted order for offset searches).

It could assert this (if the caller(s) you've added ensure this is true) or do a search/insert to keep things sorted.

Or, if you know you won't be using getUnitForOffset, etc, I guess it doesn't need to. Or if you have a phase where you're building the vector then a separate phase where you're using getUnitForOffset - you could add them all, then sort, then query - that's a bit more efficient that maintaining sorted order when you don't need it.

Make sure addUnit keeps the DWARFUnitVector sorted.

JDevlieghere marked an inline comment as done.Sep 19 2018, 12:35 AM
JDevlieghere added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
115–119 ↗(On Diff #165769)

I've updated the patch to keep thing sorted, it seems like the least bug-prone as we keep the invariant. In the verifier we also build and use the vector at the same time, so sorting at the end is tricky (and would require more state if we want to enforce this, making it a less localized approach than simply sorting).

dblaikie added inline comments.Sep 19 2018, 9:09 PM
llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
281–282 ↗(On Diff #166072)
auto UnitUP = llvm::make_unique<DWARFTypeUnit>(...)

perhaps?

(I'd probably even roll it into the next line:

Unit = TypeUnitVector.addUnit(llvm::make_unique<DWARFTypeUnit>(...));

(similarly below for CompileUnit)

461–464 ↗(On Diff #166072)

Should there be a positive case for tags are the same? (things like a member subprogram - DieTag and RefTag would both be subprogram?) Or a function defined in a namespace (I think GCC puts the declaration in a namespace, the definition at global scope - I forget what Clang does tehre - maybe it doesn't produce a declaration so there's nothing to check there?)

Have you run this check over a large codebase to see that it doesn't produce false positives?

JDevlieghere marked an inline comment as done.
  • Add check for equal tags to cover case where both are subprograms, both are formal params.
  • Ran verifier over clang, no errors.
  • Ran verifier over GCC generated binary I had laying around, also no errors.
JDevlieghere marked 2 inline comments as done.Sep 20 2018, 12:42 AM
JDevlieghere added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
461–464 ↗(On Diff #166072)

We didn't account for the case where the tags were the same, as you suggested. I ran the verifier on a ToT clang and on a gcc-generated binary (some xml lib I had around) and didn't see any false positives.

dblaikie accepted this revision.Sep 20 2018, 11:36 AM

Sounds good - thanks!

This revision was automatically updated to reflect the committed changes.
JDevlieghere marked an inline comment as done.
kito-cheng added inline comments.
llvm/trunk/lib/DebugInfo/DWARF/DWARFVerifier.cpp
463 ↗(On Diff #166408)

Hi Jonas:

Compiler will complain about fallthrough here:
lib/DebugInfo/DWARF/DWARFVerifier.cpp:477:5: warning: this statement may fall through [-Wimplicit-fallthrough=]