[PATCH] D32821: Add DWARF verifiers to verify address ranges are correct and scoped correctly.
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 15 11:23:45 PDT 2017
dblaikie added inline comments.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:124-134
+ for (const auto &R : RHS.Ranges) {
+ auto Iter = std::lower_bound(SearchBegin, End, R);
+ if (Iter != End && Iter->intersects(R))
+ return true;
+ if (Iter != Begin) {
+ auto Prev = Iter - 1;
+ if (Prev->intersects(R))
----------------
dblaikie wrote:
> This and contains still seems more expensive (O(N log(N))) than I'd expect - it should be doable without too much complexity, linearly - by walking both sorted lists at the same time.
>
> Ah, I see, by moving the starting point forward it's a smaller search space - but still not as efficient, I think, as searching linearly from the start of that range, so maybe replacing lower_bound with std::find with a comparator of some kind?
>
> Similar idea for 'contains' above.
Still wondering about this ^
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:139-153
+ if (RHS.Ranges.empty())
+ return false;
+ if (Ranges.empty())
+ return true;
+ size_t LeftSize = Ranges.size();
+ size_t RightSize = RHS.Ranges.size();
+ for (size_t I = 0; I < LeftSize; ++I) {
----------------
dblaikie wrote:
> Can probably implement this with:
>
> return std::tie(Ranges, Die.getOffset()) < std::tie(RHS.Ranges, RHS.Die.getOffset());
>
> Also - does this need to support < comparison of two entities with exactly the same ranges but a different DIE? When/where does that come up/what behavior is desirable there?
Still curious about this: does this need to support < comparison of two entities with exactly the same ranges but a different DIE? When/where does that come up/what behavior is desirable there?
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:60
+ raw_ostream &OS) {
+ Ranges.clear();
+ auto UnsortedRanges = Die.getAddressRanges();
----------------
Perhaps this should go in 'insert' which is only called from this function anyway?
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:70
+ HasErrors = true;
+ return false;
+ });
----------------
Guessing this should be 'return true;' - otherwise no elements will ever be removed?
Is that tested? (what happens if elements aren't removed here? what would that do to the rest of the code/validation?)
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:81-84
+ if (HasErrors) {
+ Die.dump(OS, 0);
+ OS << "\n";
+ }
----------------
Only dumps one of the two DIEs that are relevant to this situation. It should probably dump both - and at least some test coverage for this to demonstrate the usability of dumping two DIEs, etc.
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:125-131
+ if (Iter != End && Iter->intersects(R))
+ return true;
+ if (Iter != Begin) {
+ auto Prev = Iter - 1;
+ if (Prev->intersects(R))
+ return true;
+ }
----------------
This idiom seems to have appeared in a few places (similar code in DieRangeInfo::insert & sort of similar stuff in 'contains') any chance of factoring that out?
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:156-160
+ // Die has address ranges so we need to check that its address ranges are
+ // contained in this DieInfo's address ranges except if this is a compile
+ // unit that doesn't have address range(s).
+ bool IsCompileUnit = RI.Die.getTag() == DW_TAG_compile_unit;
+ bool CheckContains = !IsCompileUnit || !RI.Ranges.empty();
----------------
Why is a rangeless CU a special case/acceptable here, in comparison to say, a rangeless subprogram or lexical_block?
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:179
+ auto Iter = RangeSet.lower_bound(RI);
+ // Since we are searching using lower_bound, Iter might the address range
+ // that follows RI, so we need to check it in case RI overlaps with the next
----------------
grammar/missing word "Iter <something here> might the address range that follows RI"?
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:206
+void DWARFVerifier::verifyDie(const DWARFDie &Die, DieInfo &ParentDI) {
+ const auto Tag = Die.getTag();
+
----------------
Roll this into its one use:
switch (Die.getTag())
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:213-218
+ case DW_TAG_compile_unit:
+ // Set the Unit DIE info.
+ UnitDI.RI.Die = Die;
+ if (UnitDI.RI.extractRangesAndReportErrors(OS))
+ ++NumDebugInfoErrors;
+ break;
----------------
Where, if anywhere, does the case of overlapping CUs get handled? That should be an error, right?
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:215-216
+ // Set the Unit DIE info.
+ UnitDI.RI.Die = Die;
+ if (UnitDI.RI.extractRangesAndReportErrors(OS))
+ ++NumDebugInfoErrors;
----------------
If the RI is the only part of UnitDI that's used - maybe only have the RI in the first place here?
But maybe the NOR in UnitDI should be used - that would catch cases where two CUs overlap with each other, right? & then all 3 cases (compile_unit, subprogram, lexical_block) would look quite the same, I think, just operating on different DieInfo objects?
================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:225-228
+ case DW_TAG_inlined_subroutine: {
+ if (ParentDI.addContainedDieAndReportErrors(OS, DI.RI))
+ ++NumDebugInfoErrors;
+ } break;
----------------
Inconsistent bracing (some blocks in this switch have braces, some don't, for essentially the same code)
https://reviews.llvm.org/D32821
More information about the llvm-commits
mailing list