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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D37696#870056, @JDevlieghere wrote:

> 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.


Sure, fair enough.

One other thing: maybe revisit the testing (I know you brought that in from the previous patch, but I would've had the same feedback there, once we got past the other issus). It looks like a lot of testing that may be redundant (eg: the specific testing for subprograms V lexical blocks seems like maybe it could be omitted - the code doesn't special case them anyway). Also all the range overlapping checking - is some of that testing redundant or not well reduced? (it looks like full NxMx... sort of testing, when maybe representative examples could be taken without needing the fully combinatorial explosion of checks)



================
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 &&
----------------
JDevlieghere wrote:
> 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? 
Isn't considered to add enough safety for the syntactic overhead, I guess. It'd be a /lot/ of const.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:2618-2619
+              - Value:           0x0000000000002000
+          - AbbrCode:        0x00000000
+            Values:
+  )";
----------------
JDevlieghere wrote:
> 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. 
Does the test case fail with this mistake? (if it passes, why?)


Repository:
  rL LLVM

https://reviews.llvm.org/D37696