[PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm not really up to speed with the .debug_*_index sections, so I haven't looked really at the overall approach. I've just provided some basic stylistic comments.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFUnitIndex.h:22
 
+// Pre-standard implementation of package files defined a number of section
+// identifiers with values that clash definitions in the DWARFv5 standard.
----------------
I'd guess this should probably use doxygen-style comments?

Same goes for below.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFUnitIndex.h:66
+// pre-standard GNU proposal or 5 for DWARFv5 package file.
+uint32_t SerializeSectionKind(DWARFSectionKind Kind, unsigned IndexVersion);
+
----------------
lowerCamelCase for function names. Same below.


================
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
----------------
"items in columns ColumnKinds" doesn't read well to me. I'm not sure if its missing punctuation, an extra word, or what.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnitIndex.cpp:101
+    if (Version != 5)
+      return false;
+    *OffsetPtr += 2; // Skip padding.
----------------
Probably out of scope for this change, but this should return an llvm::Error instead to say why parsing failed.


================
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)
----------------
also lay -> are


================
Comment at: llvm/test/DebugInfo/X86/dwp-v2-cu-index.s:1
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o - | \
+# RUN:   llvm-dwarfdump -debug-cu-index - | \
----------------
Might be worth a brief comment at the top of this test saying this is the pre-standard GCC version.


================
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 - | \
----------------
I might have missed something, but is this relevant? I thought this patch was for supporting .debug_cu_index?


================
Comment at: llvm/test/DebugInfo/X86/dwp-v2-tu-index.s:1
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o - | \
+# RUN:   llvm-dwarfdump -debug-tu-index - | \
----------------
Add a comment again about "v2".


================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:614
+    if (CUIndex.getVersion() != 2)
+      return make_error<DWPError>("Unsupported cu_index version");
 
----------------
I see the above error message starts with a capital letter, but more generally I think we try to use lower-case for error messages. Maybe worth doing it right here and changing the above line in a separate change?


================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:653
+      if (TUIndex.getVersion() != 2)
+        return make_error<DWPError>("Unsupported tu_index version");
       addAllTypesFromDWP(
----------------
Same comment as above.


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

https://reviews.llvm.org/D75929