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

Wolfgang Pieb via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 20 15:29:28 PST 2017

wolfgangp added inline comments.

Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:113
+               const Optional<StrOffsetsContributionDescriptor> &R) {
+              return L ? R ? L->Base < R->Base : false : true;
+            });
aprantl wrote:
> This is clever, but perhaps expanding it makes it more readable?
Let me know if this is clear enough. There are several ways to write this.

Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:122
+                                 : false
+                             : false;
+                  }),
aprantl wrote:
> 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.
Let me know if this is clearer. Basically, I want to only remove duplicates when they are valid contributions. The dumper quits on the first invalid one anyway and so uniquing invalid contributions seems superfluous.

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:
> 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() :-)
Didn't realize that existed... 

We are aligning a uint64_t, so it's got to be its cousin alignTo().


More information about the llvm-commits mailing list