[PATCH] D32821: Add DWARF verifiers to verify address ranges are correct and scoped correctly.

[David Blaikie via Phabricator via llvm-commits]

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