[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

Wed Mar 18 07:36:22 PDT 2020

ikudrin marked 3 inline comments as done.
ikudrin added a comment.

In D75929#1926834 <https://reviews.llvm.org/D75929#1926834>, @labath wrote:

> (btw, is there a test case for the "unknown column" code path?)

Yes, it is checked in `llvm/test/DebugInfo/X86/debug-cu-index-unknown-section.s`, which was added in D75609 <https://reviews.llvm.org/D75609> and then extended in D75668 <https://reviews.llvm.org/D75668>.

As for unknown columns in general, I believe they are not that important to complicate the code too much. Before D75609 <https://reviews.llvm.org/D75609>, `llvm-dwarfdump` just crashed when saw them. `dwarfdump` prints some useless (for a user) error message. An unknown column cannot be used by clients of the library because they do not know what to do with it. Dumping is the only reason to support unknown identifiers, and that should be done as simple as possible. If the current implementation seems too complicated, we can consider, for example, dropping printing raw IDs for unknown sections.

================
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:
> 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 ]]?

================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnitIndex.cpp:96-100
   Version = IndexData.getU32(OffsetPtr);
+  if (Version != 2) {
+    *OffsetPtr = BeginOffset;
+    Version = IndexData.getU16(OffsetPtr);
+    if (Version != 5)
----------------
dblaikie wrote:
> What endianness is this encoded with? If it's little endian, then a 2 byte field with 2 bytes of zero padding should be the same as reading a 4 bytes, or the other way around, right? So perhaps we could just always read it as 2 bytes then 2 bytes of padding rather than having to the version/reset/reread dance here?
The endianness comes from the input file. I cannot find anything in the standard or the pre-standard proposal that would restrict it to be only little-endian. Thus, we should handle both cases.

================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnitIndex.cpp:131

+  // Fix InfoColumnKind: in DWARFv5, type units also lay in .debug_info.dwo.
+  if (Header.Version == 5)
----------------
dblaikie wrote:
> jhenderson wrote:
> > also lay -> are
> Should we be fixing it up here - or should we avoid setting it incorrectly in the first place?
As it is written at the moment, we have to pass something to distinct TU and CU indexes, for v2 cases. We may pass, say, a bool, but `true` and `false` are not that descriptive as `DW_SECT_INFO` and `DW_SECT_TYPES`. I believe that for the purpose of this patch, this fix is the simplest thing to do.

BTW, to support the combined TU index, the code should be adjusted anyway because where probably will be two "main" columns, one for v2 type units in `.debug_types.dwo` and another for v5 type units in `.debug_info.dwo`.

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

https://reviews.llvm.org/D75929