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