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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


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


================
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;
----------------
I think I would prefer a bitwise not `~`  over `-` here.


================
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:
> 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.


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:519
+  Descriptor.FormParams.Format = DWARF64;
+  Descriptor.Valid = true;
+  return true;
----------------
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.



================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:551
+  // Attempt to find a DWARF64 contribution 16 bytes before the base.
+  uint32_t HeaderOffset = (uint32_t)Descriptor.Base - 16;
+  if (!parseDWARF64StringOffsetsTableHeader(DA, HeaderOffset, Descriptor)) {
----------------
Do we need to guard against underflow here?


https://reviews.llvm.org/D41146