[PATCH] D137882: [DWARFLibrary] Add support to re-construct cu-index

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:805
+      if (!Header.extract(C, Data, &Offset, DWARFSectionKind::DW_SECT_INFO)) {
+        llvm::errs() << "Failed to parse CU header in DWP file\n";
+        Map.clear();
----------------
Is this how error handling's generally done here? I think maybe the DWARFContext has error handling callbacks that are meant to be used? (& should probably propagate up a failure result through all of this rather than continuing with corrupt data?)


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:812
+      // to do anything.
+      if (Header.getVersion() == 4 && Type == IndexType::TUIndex)
+        break;
----------------
Could you rely on the version of the index? (version 2, I think, for pre-standard index, version 5 for the DWARFv5 standard index) rather than having to wait to parse a unit to see what version it has.

There's currently no way to mix pre-standard and standard indexes, I think (owing to the valid columns accepted in each)? So that should be adequate.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:833
+
+  for (DWARFUnitIndex::Entry &E : Index.getMutableRows()) {
+    if (!E.isValid())
----------------
Rather than exposing mutability in the index interface, could this whole function (fixupIndex) be moved into the index & performed there as part of parsing?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:843
+    CUOff.Offset = Iter->second.Offset;
+    CUOff.Length = Iter->second.Length;
+  }
----------------
any reason to believe the lengths would be incorrect? Perhaps we can limit the scope a bit by not touching those?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnitIndex.cpp:256-259
+DWARFUnitIndex::Entry::SectionContribution *
+DWARFUnitIndex::Entry::getContribution() {
+  return &Contributions[Index->InfoColumn];
+}
----------------
Should this return by reference?


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:259
+static cl::opt<bool>
+    ManuallyGenerateCUIndex("manaully-generate-cu-tu-index",
+                            cl::desc("if the input is dwp file, parse debug "
----------------
Maybe, to avoid the "CU/TU/etc" Could use "Unit" consistently in both flag name and opt variable, etc. (so there's no confusion that maybe it's specifically only for the CUIndex and not the TUIndex)


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:260-261
+    ManuallyGenerateCUIndex("manaully-generate-cu-tu-index",
+                            cl::desc("if the input is dwp file, parse debug "
+                                     "info section and use it to populate "
+                                     "DEBUG_INFO contributions in cu-index. "
----------------
maybe ".debug_info" rather than "debug info"?


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:262
+                                     "info section and use it to populate "
+                                     "DEBUG_INFO contributions in cu-index. "
+                                     "For DWARF5 it also populated TU Index."),
----------------
Maybe DW_SECT_INFO rather than "DEBUG_INFO"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137882