[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:119
+      Contributions.end());
+  return std::move(Contributions);
+}
----------------
JDevlieghere wrote:
> Moving the `ContributionCollection` will actually prevent RVO (the compiler should warn you about this). 
Looking at the gcc-generated code it doesn't seem to make a difference (probably due to inlining) but yes.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:181
+      // the offset.
+      if (StringOffset <= 0xffffffffULL) {
         uint32_t StringOffset32 = (uint32_t)StringOffset;
----------------
aprantl wrote:
> std::numeric_limits<uint32_t>::max() ? (not sure if this is actually better)
Looks better anyway.


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:494
+  // the section we validate for a multiple of the entry size.
+  uint64_t ValidationSize = (Size + EntrySize - 1) & (-(uint64_t)EntrySize);
+  uint32_t ValidationOffset = (uint32_t)Base;
----------------
aprantl wrote:
> aprantl wrote:
> > I think I would prefer a bitwise not `~`  over `-` here.
> Do we need to check for overflow here? Otherwise we'll get a fuzzer bugreport incoming soon.
This is a real edge case (size > 0xfffffffffffffff8), but fuzzer will probably find it.


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:519
+  Descriptor.FormParams.Format = DWARF64;
+  Descriptor.Valid = true;
+  return true;
----------------
aprantl wrote:
> It still makes me uneasy that `Valid` is only set on one path. (Ok, with the assert that makes more sense now).
> Would it be even better for this function to return an `Expected/Optional<StrOffsetsContributionDescriptor>` instead of having an explicit `bool Valid` member? This would make it less likely that we accidentally operate on an invalid record.
> 
I refactored this using Optional<>. You're right, the valid bit looks so last century...


https://reviews.llvm.org/D41146