dblaikie 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.
+//
----------------
ikudrin wrote:
> 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.
I think it might be simpler to remove the "more or less arbitrary" part - it
invites questions - just how much less?
And maybe rephrase the "is intended to be" and make it "The enum is for
internal use only & doesn't correspond to any input/output constants".
But otherwise I guess this is fairly temporary so not worth haggling over.
================
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.
----------------
ikudrin wrote:
> 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.
AH, fair enough - thanks!
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
