[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: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:
> 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?
Still curious about this


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:60
+    raw_ostream &OS) {
+  Ranges.clear();
+  auto UnsortedRanges = Die.getAddressRanges();
----------------
dblaikie wrote:
> Perhaps this should go in 'insert' which is only called from this function anyway?
Still curious about this


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:70
+          HasErrors = true;
+        return false;
+      });
----------------
dblaikie wrote:
> 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?)
While adding an assert is useful - I'm still curious about what would go wrong if/when this wasn't removing the elements it's intended to? Is this purely a performance improvement (if so, it's fine as-is, with the assert). Does this change behavior at all, in a way that matters/is intentional? (eg: does it make follow-on error messages better?) If so, that behavior should be tested.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:81-84
+  if (HasErrors) {
+    Die.dump(OS, 0);
+    OS << "\n";
+  }
----------------
dblaikie wrote:
> 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.
Still curious about this


================
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;
+    }
----------------
dblaikie wrote:
> 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?
Still curious about this - this code and DieRangeInfo::insert (& similar, though not identical code for 'contains' tests) were duplicate, and now this code is fixed but DieRangeInfo::insert is not. Please refactor so the code's written once, not twice to avoid this kind of divergence/duplication/repeated work.


================
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();
----------------
dblaikie wrote:
> Why is a rangeless CU a special case/acceptable here, in comparison to say, a rangeless subprogram or lexical_block?
Still curious about this


================
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;
----------------
dblaikie wrote:
> Where, if anywhere, does the case of overlapping CUs get handled? That should be an error, right?
Still wondering about this.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:215-216
+    // Set the Unit DIE info.
+    UnitDI.RI.Die = Die;
+    if (UnitDI.RI.extractRangesAndReportErrors(OS))
+      ++NumDebugInfoErrors;
----------------
dblaikie wrote:
> 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?
Still curious about this


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:131-133
+  auto Iter = std::upper_bound(Begin, End, RHS.Ranges.front());
+  if (Iter != Begin)
+    --Iter;
----------------
I'd probably skip this - the algorithm's linear anyway & pretty tidy without it. I suppose it catches the common case where they don't overlap at all, but might be better done another way (range lists could keep their max/min bound & shortcircuit here, etc)


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:135-141
+    if (R.End <= Iter->Start)
+      continue;
+    while (Iter != End) {
+      if (Iter->intersects(R))
+        return true;
+      ++Iter;
+    }
----------------
This looks more expensive than what I had in mind - I figured a linear search to find the item, then testing 'intersects', rather than testing intersects on every item along the way.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:176-177
+
+  // Check if this range overlaps with any previous sibling ranges and make an
+  // error if needed.
+  if (NOR.insertAndReportErrors(OS, DieRI))
----------------
Probably "emit (or report, to match the function name) an error" ("make an error" sounds curious, like maybe it returns an llvm::Error (though even that I'd expect to be described as "returns an Error") that the caller is responsible for emitting to the user, etc)


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:183
+
+Optional<DWARFDie> DWARFVerifier::NonOverlappingRanges::GetOverlappingRangeInfo(
+    const DieRangeInfo &RI) const {
----------------
DWARFDie is already boolean testable - does it need to be wrapped in Optional? Or could this return an invalid DWARFDie to signal the "no overlap" case?


https://reviews.llvm.org/D32821