[PATCH] D79962: Fix the verification of DIEs with DW_AT_ranges.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a subscriber: Higuoxing.
jhenderson added a comment.

I don't know if you've noticed, but you might be interested in following @Higuoxing's GSOC project to implement DWARF support for ELF yaml2obj. That would allow you to write this test using ELF, and omit 99% of the noise (to be clear, I'm not suggesting waiting, but it should help you in the future). If you've got any specific comments on his proposal, there's an RFC that's just gone up on the mailing list. I'd hope that some of the principles in that project (auto-generating most of the debug data) might even be more generically useful for Mach-O in the future.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFAddressRange.h:58
+  /// ranges were combined.
+  bool union_range(const DWARFAddressRange &RHS) {
+    if (!intersects(RHS))
----------------
clayborg wrote:
> dblaikie wrote:
> > Probably could use a more explicit name - "union_if_intersecting"?
> that works
Shouldn't it be `unionIfIntersecting`?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:420
+      // more dead stripped address ranges which tend to all be at the same
+      // address: 0 or -1.
+      if (auto PrevRange = RI.insert(Range)) {
----------------
jhenderson wrote:
> clayborg wrote:
> > dblaikie wrote:
> > > Where have you seen -1? Both gold and lld use 0, binutils ld... is more complicated, but I don't think it uses -1? hmm, maybe it does... hmm, at least for ranges it uses 1:  [0x0000000000000001, 0x0000000000000001) which is different from gold and lld but for the address it uses zero:
> > > 
> > >                 DW_AT_low_pc    (0x0000000000000000)
> > >                 DW_AT_high_pc   (0x0000000000000006)
> > > 
> > > 
> > I believe Paul Robinson stated that the Sony linker does this. Paul did I remember correctly?
> Not got time to do a full review myself today, but just chiming in to confirm that the Sony linker patches low_pc to -1 for stripped stuff in .debug_info and .debug_aranges. For .debug_ranges we actually use -2/-2 pairs because -1 is a special value in that section.
> 
> I vaguely recall that there was some discussion on the mailing list about this a couple of years ago, and that there may be some other users of this approach too.
Some more related thoughts:

A range of (0, 0) is the end marker, whilst (-1, ...) is a base address selection entry. I don't know how these are represented (if at all) in the table. Either way, as mentioned (-2, -2) is a range we use for deadstripped code, so if that can be handled, we'd be very grateful! Last time I checked (admittedly quite a long while ago) DWARF verification didn't work for us because of stripped stuff (and possibly other reasons, but we haven't bothered digging further yet).


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:36-40
+    if (Pos->intersects(R)) {
+      DWARFAddressRange Range(*Pos);
+      Pos->union_if_intersecting(R);
+      return Range;
+    }
----------------
I think this and the similar code below be simplified slightly:

```
DWARFAddressRange Range(*Pos);
if (Pos->union_if_intersecting(R))
  return Range;
```

Alternatively, get rid of the "if intersecting" part of the union function (perhaps make it an assertion instead, but I'm not sure).


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/verify_overlapping_cu_ranges.yaml:67
+
+# CHECK-NOT: error: DIE address ranges are not contained in its parent's ranges:
+# CHECK: Verifying .debug_info references...
----------------
Maybe it would be better to add `--implicit-check-not=error:` to FileCheck's invocation. That way, all errors will be detected, regardless of position, and will avoid this check passing spuriously in the future should the text of the message ever change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79962/new/

https://reviews.llvm.org/D79962