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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
