[PATCH] D37696: [dwarfdump] Add DWARF verifiers for address ranges

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

In https://reviews.llvm.org/D37696#869811, @dblaikie wrote:

> Also - how does this work (what does it do, does it catch things correctly) when there are DIEs without ranges - such as compile_unit{namespace{subprogram}} - does it still check that the subprogram is contained within the compile_unit?


Currently it only looks at an entity's direct parent. It will not detect that the subprogram is not part of the compile unit in this situation. If we keep a reference to the parent in each DWARFAddressRange we could walk up the chain until we find the first ancestor with a range. If it's contained this would would guarantee that it's also contained further up.

Would you agree to keep that for a separate diff? This situation would only result in a false-negative and I really think we would benefit from having this functionality checked-in.



================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:338
+  // Verify that ranges are contained within their parent.
+  const bool ShouldBeContained = !Ranges.empty() && !ParentRI.Ranges.empty() &&
+                                 !(Die.getTag() == DW_TAG_subprogram &&
----------------
dblaikie wrote:
> dblaikie wrote:
> > Would this correctly check that the nested subprogram is contained within the CU, though? (looks like it just punts on the nested subprogram entirely?)
> the LLVM style doesn't generally use top-level const on locals - probably best to drop that for consistency
Interesting. What's the reasoning behind this? 


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:2618-2619
+              - Value:           0x0000000000002000
+          - AbbrCode:        0x00000000
+            Values:
+  )";
----------------
dblaikie wrote:
> Wouldn't this need three zero entries? (one to finish each DIE's child list)
Thanks, I modified an existing test but forgot to update this part. 


Repository:
  rL LLVM

https://reviews.llvm.org/D37696