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

[Igor Kudrin via Phabricator via lldb-commits]

ikudrin added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFUnitIndex.h:116
+  // This is a parallel array of raw section IDs for columns of unknown kinds.
+  // This array is created only if there are items in columns ColumnKinds with
+  // DW_SECT_EXT_unknown and the only initialized items here are those with
----------------
jhenderson wrote:
> "items in columns ColumnKinds" doesn't read well to me. I'm not sure if its missing punctuation, an extra word, or what.
Thanks for noticing that. It was an ugly result of multiple edits. Rephrased again. Hope it is better now.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnitIndex.cpp:101
+    if (Version != 5)
+      return false;
+    *OffsetPtr += 2; // Skip padding.
----------------
jhenderson wrote:
> Probably out of scope for this change, but this should return an llvm::Error instead to say why parsing failed.
Added to my backlog, but do not mind if anyone willing to fix that.


================
Comment at: llvm/test/DebugInfo/X86/dwp-v2-loc.s:1
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o - | \
+# RUN:   llvm-dwarfdump -debug-info -debug-loc - | \
----------------
jhenderson wrote:
> I might have missed something, but is this relevant? I thought this patch was for supporting .debug_cu_index?
The patch adjusts the code in the constructor of `DWARFUnit` which reads the location table. This test checks that pre-v5 units read their location tables from `.debug_loc.dwo` sections. Its counterpart, `dwp-v5-loclists.s` checks that v5 units use `.debug_loclists.dwo`. These changes might be probably extracted later to a separate patch.


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

https://reviews.llvm.org/D75929