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

[Wolfgang Pieb via Phabricator via llvm-commits]

wolfgangp 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,
----------------
JDevlieghere wrote:
> 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.
Hmm, OK. I'm a bit old school on returning larger things from functions, but this is cleaner.


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:80
 
 bool DWARFUnit::getStringOffsetSectionItem(uint32_t Index,
                                            uint64_t &Result) const {
----------------
JDevlieghere wrote:
> 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. 
Right. I think this code originally predates the wider use of Optional<>. Refactoring would seem like a good idea, but not in 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) {
----------------
JDevlieghere wrote:
> What do you think about making this a member function of `StrOffsetsContributionDescriptor`?
Makes sense.


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:570
+    parseDWARF64StringOffsetsTableHeader(DA, HeaderOffset, Descriptor);
+    if (!Descriptor.Valid) {
+      Descriptor.Valid = true;
----------------
wolfgangp wrote:
> aprantl wrote:
> > The way that the Valid flag is set as a side-efffect here is a bit difficult to follow. Could it perhaps be a return value of parseDWARF64StringOffsetsTableHeader?
> Sure. I'll bury the resetting of the valid bit inside the parseDWARF* functions as well to localize it a bit more.
Streamlined this a bit. The parse* routines now expect an invalid descriptor in all cases and return the validity. the parseDWARF32* routine technically doesn't need to do that since its return value is never examined, but it seems more consistent that way.


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:571
+    if (!Descriptor.Valid) {
+      Descriptor.Valid = true;
+      HeaderOffset = (uint32_t)Offset;
----------------
JDevlieghere wrote:
> 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.
Right, it wasn't necessary. This should look a bit more consistent now.


https://reviews.llvm.org/D41146