[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: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:
> > 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?
> Error recovery (in dumpListSection() in DWARFContext.cpp) relies on the correct interpretation of the length field in the header. In DWARF 5 we need to add sizeof(uint32_t) to the extracted length, in DWARF 4 the length field has simply been set to the section size and already represents the length.
> 
> If we find errors before we extract the version, e.g. while validating the extracted length against section size etc., we return without knowing the version, and getLength( ) doesn't know what to do. Hence I wanted to preset the version until we extract it from the data. 
> 
> The other relevant scenario is when we extract an invalid version from the data. In that case we report the error but need to set the version to something valid in order to have the length interpreted correctly.
> 
> I suppose we could handle this differently by providing a different interface to get the table length for recovery purposes. We always know the version in the context, so we would never have to preset the table version during extraction. Do you have any other suggestions?
I think I'm just generally confused about this still.

(looks like, for now, the Version field is no longer preset? So maybe what I'm asking about has already been addressed? (I Was concerned the default value in the Version field might be misleading or otherwise cause problems later))

Why does the error recovery in dumpListSection only apply to DWARF4 and earlier?

It seems like the contract for "extract" is a bit subtle if you can still access some fields(like length) but not others. Maybe it'd be better to have extract modify an in/out parameter of the offset - rather than relying on reading the length after extract fails? Not sure - and that doesn't address the issue of printing partially extracted lists... - maybe that's not worth supporting?


https://reviews.llvm.org/D51081