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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
