[PATCH] D41146: [DWARF] DWARF v5: Rework of string offsets table reader

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:97
 static void
-dumpDWARFv5StringOffsetsSection(raw_ostream &OS, StringRef SectionName,
-                                const DWARFObject &Obj,
-                                const DWARFSection &StringOffsetsSection,
-                                StringRef StringSection, bool LittleEndian) {
+collectContributionData(DWARFContext::cu_iterator_range CUs,
+                        DWARFContext::tu_section_iterator_range TUSs,
----------------
How about just returning the vector by value and relying on copy elision? This signature initially gave me the impression that we might have several calls to this function for the same ContributionCollection.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:149
+    uint64_t ContributionHeader = Contribution.Base;
+    if (Version >= 5)
+      ContributionHeader -= Format == DWARF32 ? 8 : 16;
----------------
I'd add a comment with the stuff you wrote in the patch description here to explain why you're subtracting the 8/16 bytes for DWARFv5 and later. 


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:80
 
 bool DWARFUnit::getStringOffsetSectionItem(uint32_t Index,
                                            uint64_t &Result) const {
----------------
I think it would be nice to have this return an `Optional<int64_t>` rather than this bool/ref combo, but obviously that doesn't have to be part of this patch. 


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:487
+// a multiple of the size of one of its entries.
+static void validateContributionSize(DWARFDataExtractor &DA,
+                                     StrOffsetsContributionDescriptor &Descriptor) {
----------------
What do you think about making this a member function of `StrOffsetsContributionDescriptor`?


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:571
+    if (!Descriptor.Valid) {
+      Descriptor.Valid = true;
+      HeaderOffset = (uint32_t)Offset;
----------------
Why do you need to set the value to true here? If I understand correctly the flag is set to `false` when calling the `parseDWARF64StringOffsetsTableHeader` and I don't immediately see something that relies on it.


https://reviews.llvm.org/D41146