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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

Thanks, Wolfgang. With these few nits addressed this LGTM.



================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:113
+              if (!L)
+                return true; // Invalid L should always precede R.
+              if (R)
----------------
wolfgangp wrote:
> wolfgangp wrote:
> > probinson wrote:
> > > aprantl wrote:
> > > > Not sure if this matters, but won't this be nondeterministic when both L and R are invalid?
> > > I think the most compact code would be
> > > ```
> > > if (L && R) return L->base < R->base;
> > > return R;
> > > ```
> > > if both are valid, order depends on the base.
> > > if L is invalid and R valid, L sorts before R.
> > > if L is valid and R invalid, R sorts before L.
> > > if both are invalid, they are equivalent (test(R,L) == test(L,R)).
> > > 
> > > Not sure if this matters, but won't this be nondeterministic when both L and R are invalid?
> > 
> > That's true. Several invalid contributions in the vector will probably end up in different locations when the initial order is different, but all that matters is that all the invalid ones end up at the beginning. It would only matter if we tried to be more precise about how and why they are invalid (bad start vs. invalid length etc.), but this patch doesn't try to do that. It's not clear to me if it would be worth it.
> > I think the most compact code would be 
> 
> Sleek! I can put that in, if you guys prefer.
> 
I like Paul's suggestion. 


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:512
+  (void)DA.getU16(&Offset); // padding
+  StrOffsetsContributionDescriptor Descriptor(Offset, Size, Version, DWARF64);
+  return Optional<StrOffsetsContributionDescriptor>(Descriptor);
----------------
`return StrOffsetsContributionDescriptor(Offset, Size, Version, DWARF64);`


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:527
+  (void)DA.getU16(&Offset); // padding
+  StrOffsetsContributionDescriptor Descriptor(Offset, ContributionSize, Version,
+                                              DWARF32);
----------------
`return StrOffsetsContributionDescriptor(Offset, ContributionSize, Version, DWARF32);`


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:567
+  // DWARF32 format.
+  return Optional<StrOffsetsContributionDescriptor>(
+      StrOffsetsContributionDescriptor(Offset, Size, 4, DWARF32));
----------------
`return StrOffsetsContributionDescriptor(Offset, Size, 4, DWARF32);`


https://reviews.llvm.org/D41146