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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Yeah, I'm mostly OK with this direction. It's pretty isolated, maybe easy enough to explain if needed, etc.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:785-787
+enum class IndexType { CUIndex, TUIndex };
+using EntryType = DWARFUnitIndex::Entry::SectionContribution;
+using EntryMap = std::unordered_map<uint32_t, EntryType>;
----------------
These could probably all sink into `fixupIndex` as locals?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:787
+using EntryType = DWARFUnitIndex::Entry::SectionContribution;
+using EntryMap = std::unordered_map<uint32_t, EntryType>;
+void fixupIndex(const DWARFObject &DObj, DWARFContext &C,
----------------
Maybe DenseMap rather than unordered_map?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:2045
   }
-  const DWARFSection &getNamesSection() const override {
-    return NamesSection;
-  }
+  const DWARFSection &getNamesSection() const override { return NamesSection; }
 
----------------
looks like this unrelated change snuck in?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:833
+
+  for (DWARFUnitIndex::Entry &E : Index.getMutableRows()) {
+    if (!E.isValid())
----------------
ayermolo wrote:
> 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.
Fair enough - mixed feelings, but I'll rescind this piece at least.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:843
+    CUOff.Offset = Iter->second.Offset;
+    CUOff.Length = Iter->second.Length;
+  }
----------------
ayermolo wrote:
> 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.
I'd prefer to be a bit less permissive, really - to not end up creating more weird cases that systems might come to depend on.

Maybe fail if the length doesn't match, until we know of any particular case with mismatched lengths that we understand enough to want to/figure out how to support?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnitIndex.cpp:256-259
+DWARFUnitIndex::Entry::SectionContribution *
+DWARFUnitIndex::Entry::getContribution() {
+  return &Contributions[Index->InfoColumn];
+}
----------------
ayermolo wrote:
> dblaikie wrote:
> > Should this return by reference?
> I was trying to keep same return type as the const version. Changed to reference.
oh, yeah, the other should probably change too... ifyou could do that in a separate patch, that'd be great


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137882