[PATCH] D51081: [DWARF v5] Refactoring range list dumping to fold DWARF v4 functionality into v5 handling (almost NFC).

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFListTable.h:52
+  bool empty() const {
+    return Entries.empty() || Entries.begin()->isEndOfList();
+  }
----------------
wolfgangp wrote:
> dblaikie wrote:
> > wolfgangp wrote:
> > > probinson wrote:
> > > > dblaikie wrote:
> > > > > Why this new special case - when would the range list contain an empty range?
> > > > It's explicitly legal to have a list that consists solely of the end-of-list marker.  Not saying any producer would actually emit it this way, but it's one way to describe an empty range.  The parser should handle this case smoothly.
> > > Also , the DWARF5 rnglist implementation keeps the end-of-list entry in the list (unlike the DWARF4 implementation), which makes display a bit more straightforward.
> > Good point - I guess I could narrow/clarify my comment somewhat: Why is the emptiness of a range list important? I'd figure the printing algorithm would work the same either way.
> >Good point - I guess I could narrow/clarify my comment somewhat: Why is the emptiness of a > range list important? I'd figure the printing algorithm would work the same either way.
> 
> This was motivated by the code in DwarfLinker.cpp (dsymutil) that needs to check for the emptiness of a list during range patching. With the end-of-list entry as part of the Entries vector it seemed prudent to provide this method.
Could this be addressed by keeping the DWARF4 end-of-list entry too, for consistency?

Though I'm still a bit confused - I'm not sure why DwarfLinker.cpp would need to worry about whether the list is empty or not? Maybe as an optimization, but otherwise I'd figure it would just iterate over the list & if it's empty, it would iterate over zero things.

& why does this change you're making cause this extra case to be added - I thought this code (DWARFListtype) was for DWARF5 rnglists already & now it was getting DWARF4 support - but it sounds like you had to add a case that is for the DWARF5 case... so I'm a bit confused?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFListTable.h:72-77
+  /// Print a header for the list. Returns the indentation that is to be used
+  /// when subsequently dumping each list entry.
+  unsigned printListHeader(raw_ostream &OS, uint8_t AddrSize,
+                           DIDumpOptions DIDumpOpts) const {
+    return 0;
+  }
----------------
wolfgangp wrote:
> dblaikie wrote:
> > This seems to be dead in this patch - maybe remove it from here and add it in whichever patch adds its use?
> Actually, it's used further down in DWARFListTableBase<DWARFListType>::dump
I guess perhaps I was thinking: this function seems to be a no-op & should be removed from this patch - and added back in when it's used/needed?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFListTable.h:87-90
+    /// The DWARF version number. We assume version 5 until we either extract
+    /// the version from the section or explicitly set it (e.g. to fake a
+    /// table for DWARF v4 and earlier).
+    uint16_t Version = 5;
----------------
wolfgangp wrote:
> dblaikie wrote:
> > That sounds like in all cases we know the version from the input, right? So when/why would this assumption be needed?
> During extraction we're using getTableLength() (I've renamed it from just 'length()' in the first version of this review) for validation and also to determine if we can recover from an error and keep parsing the section. With the creation of an artificial table for DWARF 4 range lists the calculation of the table length is now dependent on the version, since for DWARF 5 we need to add the size of the length field to the length we extracted. In DWARF 4 the length is simply the size of the section as is. 
> If we find an error in the table header before we get to extract the version, the version would obviously not be set to anything meaningful and would confuse our recovery in DWARFContext.cpp. Also, presetting the version allows us to use getTableLength() before we actually extract the version for validation.
> But it's perhaps more appropriate to preset the version during extraction, so I'm doing it there now.
> 
I'm pretty confused by this - how does having a hardcoded (possibly incorrect) version help with error recovery? I guess it's correct based on the section being extracted from (though in the future it could be incorrect when we get to DWARFv6)?

What sort of recovery would be done when the version couldn't be read from the input?


https://reviews.llvm.org/D51081