[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:60-71
+  for (auto Curr = DieRangeInfos.begin(); Curr != End; ++Curr) {
+    if (Prev != End) {
+      if (Prev->DoesIntersect(*Curr)) {
+        ++NumDebugInfoErrors;
+        OS << "error: DIEs have overlapping address ranges:\n";
+        Prev->Die.dump(OS, 0);
+        Curr->Die.dump(OS, 0);
----------------
This still looks like it only compares adjacent pairs:

   {{1, 2}, {2, 3}, {3, 4}}

Is that not the case? If it does' what if {1,3} overlap, but neither overlaps with 2? Will that be diagnosed?


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:134-163
+    if (!UnitRI.Ranges.empty() && !UnitRI.Contains(VRI.RI)) {
+      ++NumDebugInfoErrors;
+      OS << "error: CU DIE has child with address ranges "
+            "that are not contained in its ranges:\n";
+      UnitRI.Die.dump(OS, 0);
+      Die.dump(OS, 0);
+      OS << "\n";
----------------
I /think/ a lot of this code could be simplified if there were (as I mention in another comment) a generalized representation of the "scope of non-overlapping things with a possible parent they should all be within" that could be used at the CU level, subprogram level, and inter-subprogram level.

Let me know if you'd like me to sketch out such a design in more detail.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:179-180
+  // Verify that any address ranges from this DIE's children don't overlap.
+  if (!VRI.ChildRangesCantOverlap.empty())
+    verifyNoRangesOverlap(VRI.ChildRangesCantOverlap);
+}
----------------
Why not do this as elements are visited, rather than at the end in another pass?

I'd imagine a small class that would take DIEs verify their ranges, merge them, flag merge failures (between previous DIEs or parents) & by the end there'd be nothing left to do?


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:227
 
-void DWARFVerifier::verifyDebugInfoForm(DWARFDie &Die,
+void DWARFVerifier::verifyDebugInfoForm(const DWARFDie &Die,
                                         DWARFAttribute &AttrValue) {
----------------
If this change is independent of the rest of this patch, please commit it separately (doesn't need to be sent for review if you're just constifying something that should've been const to begin with)


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:295
 
-void DWARFVerifier::veifyDebugInfoReferences() {
+void DWARFVerifier::verifyDebugInfoReferences() {
   // Take all references and make sure they point to an actual DIE by
----------------
Similarly, commit without pre-commit review spelling corrections like this - makes it easier to review the real changes without having them conflated with cleanup changes like this.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:316-320
+  // Now go through all function address ranges and check for any that
+  // overlap. All function range infos were added to a std::set that we can
+  // traverse in order and do the checking for overlaps.
+  OS << "Verifying .debug_info function address ranges for overlap...\n";
+  verifyNoRangesOverlap(AllFunctionDieRangeInfos);
----------------
I'd expect to do the subprogram checking incrementally, the same as the lexical scopes are done incrementally when another lexical scope is visited? Is there a reason to do it differently?

(possibly even using the same machinery/abstraction - even if it needs to be special cased to handle the interesting nesting cases)


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:330
+    VerifyDieInfo ParentVRI;
+    verifyDie(CU->getUnitDIE(ExtractUnitDIEOnly), UnitRI, ParentVRI);
   }
----------------
Probably do this with

  getUnitDIE(/* ExtractUnitDIEOnly = */ false)

rather than a local constant. Or try the trick Adrian did in a recent patch - an enum with a single value as a constant:

  enum { ExtractAllDIEs = false };

(& could include that in the header where getUnitDIE is declared)


https://reviews.llvm.org/D32821