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:
> > 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.
================
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.
----------------
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)
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
