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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Found a few more nitpicks, but otherwise I'm happy now. Thanks!



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFUnit.h:211
+  /// Start, length, and DWARF format of the unit's contribution to the string
+  /// offsets table (DWARF v5). It is initially invalid.
+  Optional<StrOffsetsContributionDescriptor> StringOffsetsTableContribution;
----------------
s/It is initially invalid.//


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:113
+               const Optional<StrOffsetsContributionDescriptor> &R) {
+              return L ? R ? L->Base < R->Base : false : true;
+            });
----------------
This is clever, but perhaps expanding it makes it more readable?


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:122
+                                 : false
+                             : false;
+                  }),
----------------
Is this intentionally different to the previous comparator? To the casual reader (me) it's non-obvious why. Some comments or more verbose code might help.


================
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;
----------------
wolfgangp wrote:
> 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.
And now with the new operator, this looks really like llvm::alignAddr() :-)


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:522
+parseDWARF32StringOffsetsTableHeader(DWARFDataExtractor &DA, uint32_t Offset) {
+  if (!DA.isValidOffsetForDataOfSize(Offset, 8)) {
+    return Optional<StrOffsetsContributionDescriptor>();
----------------
LLVM style wants to remove extra braces


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:526
+  uint32_t ContributionSize = DA.getU32(&Offset);
+  if (ContributionSize >= 0xfffffff0) {
+    return Optional<StrOffsetsContributionDescriptor>();
----------------
same here


https://reviews.llvm.org/D41146