Adds a verification that DW_AT_specification and DW_AT_abstract origin
reference a DIE with a compatible tag.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
| lib/DebugInfo/DWARF/DWARFVerifier.cpp | ||
|---|---|---|
| 423 | Can you use DWARFDie::getAttributeValueAsReferencedDie here, perhaps? | |
| test/tools/llvm-dwarfdump/X86/verify_compatible_tags.s | ||
| 23–24 | 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?) | |
| lib/DebugInfo/DWARF/DWARFVerifier.cpp | ||
|---|---|---|
| 423 | Unfortunately not, because this makes use of getUnitForOffset which in turn uses a list of units that isn't build when doing the verification. | |
| lib/DebugInfo/DWARF/DWARFVerifier.cpp | ||
|---|---|---|
| 422 | tags | |
Comment Actions
LGTM with outstanding nitpicks addressed and assuming David is happy with the patch.
| lib/DebugInfo/DWARF/DWARFVerifier.cpp | ||
|---|---|---|
| 434 | << "" ? | |
| test/tools/llvm-dwarfdump/X86/verify_compatible_tags.s | ||
|---|---|---|
| 24 | can you use CHECK-NEXT here to make the test even stricter? | |
Comment Actions
- Use SmallSet<DWARFDie, 3> (running dsymutil over clang showed that there were never more than 3 elements in the set for valid DWARF).
| 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. |
| 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). |
| 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? |
Comment Actions
- 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.
| 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. |
| llvm/trunk/lib/DebugInfo/DWARF/DWARFVerifier.cpp | ||
|---|---|---|
| 463 ↗ | (On Diff #166408) | Hi Jonas: Compiler will complain about fallthrough here: |
tags