[PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin marked 2 inline comments as done.
ikudrin added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFUnitIndex.h:47-48
+// For pre-standard ones, which correspond to sections being deprecated in
+// DWARFv5, the values are chosen more or less arbitrary and a tag "_EXT_"
+// is added to the names.
+//
----------------
dblaikie wrote:
> ikudrin wrote:
> > dblaikie wrote:
> > > Probably not arbitrarily - in the sense that this is an extension that consumers/producers will need to agree to - so maybe saying that ("non-standard extension"/"proposed for standardization" or something to that effect) and/or linking to the dwarf-discuss thread to support why these values were chosen & they can't be changed arbitrarily.
> > As far as the enum is internal, no one should really worry about the actual values; they are not important and do not need any kind of proof. They may be really arbitrary, that will change nothing. That is what I meant when said "more or less".
> > 
> > The plan is that this patch supports DWARFv5 unit indexes, but not the proposed combined indexes. When the combined indexes are approved, there will be another patch, which moves the enum with all extensions in the public space. At that moment the factual values will become important, and the references to the descriptive document will be added. Do you think it will be possible to add such a document to the [[ http://wiki.dwarfstd.org | DWARF Wiki ]]?
> Hmm, I'm confused then - ah, OK - so you've added the enum to support encoding the version 2 and version 5 tables into one internal data structure, but haven't extended it to actually dump or use (for parsing: eg to find the debug_loc.dwo contribution for a v4 unit described by a v5 index) them when parsing/rendering a v5 index.
> 
> OK, sorry I hadn't realized that. Then, yes, the comment makes sense for now. Perhaps "the values are only used internally/not parsed from input files (if these values appear in input files they will be considered "unknown")" would be more suitable?
> 
> > The plan is that this patch supports DWARFv5 unit indexes, but not the proposed combined indexes. When the combined indexes are approved, there will be another patch, which moves the enum with all extensions in the public space. At that moment the factual values will become important, and the references to the descriptive document will be added. Do you think it will be possible to add such a document to the DWARF Wiki?
> 
> Given the DWARF committee is not in session at the moment (I think) & it'll be a while before another spec is published - I think it'll be necessary and appropriate to implement support for the extension columns in llvm-dwarfdump at least before/when they're implemented in llvm-dwp (which will be soon) to support testing that functionality & working with such files.
> 
> Might be able to put something on the DWARF wiki, but I don't know much about it/how things go up there.
> Perhaps "the values are only used internally/not parsed from input files (if these values appear in input files they will be considered "unknown")" would be more suitable?

The comment says something similar in lines 52-53. Do you think it should be extended?

> I think it'll be necessary and appropriate to implement support for the extension columns in llvm-dwarfdump at least before/when they're implemented in llvm-dwp (which will be soon) to support testing that functionality & working with such files.

I think the same. The only concern in my side is that the proposal should be formulated as an RFC or similar document before implementing it in the code so that all the implementations can reference the same source. For my taste, a link to the middle of a forum conversation cannot be considered as a reliable source.


================
Comment at: llvm/test/DebugInfo/X86/dwp-v5-loclists.s:1-3
+## The test checks that v5 compile units in package files read their
+## location tables from .debug_loclists.dwo sections.
+## See dwp-v2-loc.s for pre-v5 units.
----------------
dblaikie wrote:
> Might be possible/better to test this without debug_abbrev and debug_info - make the test shorter & dump only the loclists section itself? Yeah, not exactly valid, but no reason the dumper shouldn't support it and it'd be a more targeted test (apply this suggestion to other tests if possible/acceptable too)
This test is for changes in the constructor of `DWARFUnit`. It checks that `DWARFUnit` takes locations from the right place, so all four sections are necessary.


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

https://reviews.llvm.org/D75929