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

[Alexander Yermolovich via Phabricator via llvm-commits]

ayermolo marked 3 inline comments as done.
ayermolo 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();
----------------
dblaikie wrote:
> 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?)
Changed it to logAllUnhandledErrors(createError()) for now
To propagate up can chagne DWARFContext::get(CU,TU}Index() to return Expectec<DWARFUnitIndex>?

I was going with something more localized to minimize the impact. I guess it comes down to philosophical question of whether if CU/TU index is partially corrupted we want to consider whole thing corrupted, or keep the current behavior of at least being able to access debug info below 4GB. 


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:833
+
+  for (DWARFUnitIndex::Entry &E : Index.getMutableRows()) {
+    if (!E.isValid())
----------------
dblaikie wrote:
> Rather than exposing mutability in the index interface, could this whole function (fixupIndex) be moved into the index & performed there as part of parsing?
Wasn't part of the feedback from other diff is to minimize impact and not modify cu/tu index parsing, or did I miss understand? We then will need to modify parse to pass in context, and if we are parsing CU or TU.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:843
+    CUOff.Offset = Iter->second.Offset;
+    CUOff.Length = Iter->second.Length;
+  }
----------------
dblaikie wrote:
> any reason to believe the lengths would be incorrect? Perhaps we can limit the scope a bit by not touching those?
I didn't want to assume anything about the producer. If Offset is corrupt, depending how how length is calculated at least one might be corrupt also. Also we are overriding all the offsets, if we mess up on that, doesn't really matter if new length is correct or not.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnitIndex.cpp:256-259
+DWARFUnitIndex::Entry::SectionContribution *
+DWARFUnitIndex::Entry::getContribution() {
+  return &Contributions[Index->InfoColumn];
+}
----------------
dblaikie wrote:
> Should this return by reference?
I was trying to keep same return type as the const version. Changed to reference.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137882