[PATCH] D57939: Refactor RelocVisitor and fix computation of SHT_RELA-typed relocation entries

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1512-1515
+        // Try our best to check if Resolver can handle this relocation type. It
+        // can if it doesn't return the third argument (-1ULL). It isn't a big
+        // deal if this check fails. We just fail to catch this error early.
+        // It is a TODO to make Resolver more reliable.
----------------
echristo wrote:
> dblaikie wrote:
> > MaskRay wrote:
> > > dblaikie wrote:
> > > > What's the failure mode if this is not diagnosed here?
> > > > 
> > > > If there's a valid fallback/reliable failure later - could we remove this code entirely? (rather than having a best effort then fallback - seems like having two partial solutions in different places is liable to have things slip through)
> > > > What's the failure mode if this is not diagnosed here?
> > > 
> > > The unresolved relocation type will slip through and there will be no diagnostics later when the relocations are actually resolved. (Note that the return types of `visit*` (or `resolve*` after the patch) are `uint64_t`, instead of `Error` `Optional` or others).
> > > 
> > > And the usage (call sites of `getRelocatedValue`) does not allow it to do fancy error handling.
> > > 
> > > ```
> > > uint64_t DWARFDataExtractor::getRelocatedValue(uint32_t Size, uint32_t *Off,
> > >                                                uint64_t *SecNdx) const {
> > >   if (SecNdx)
> > >     *SecNdx = -1ULL;
> > >   if (!Section)
> > >     return getUnsigned(Off, Size);
> > >   Optional<RelocAddrEntry> Rel = Obj->find(*Section, *Off);
> > >   if (!Rel)
> > >     return getUnsigned(Off, Size);
> > >   if (SecNdx)
> > >     *SecNdx = Rel->SectionIndex;
> > >   return getUnsigned(Off, Size) + Rel->Value;
> > > }
> > > ```
> > > 
> > > So I know that this is ugly and can be improved.. But to fix the bug I cannot think of a better way without being more intrusive (I do not want to touch many other places for this patch).
> > OK, it sounds like the comment might be a bit off, then:
> > 
> > " It isn't a big deal if this check fails. We just fail to catch this error early."
> > 
> > That last bit sounds like "well, we don't catch it /early/, I guess we catch it later" - but it sounds like we don't catch it later? So I'm not sure if it's not a big deal.
> > 
> > I'm not sure the partial solution is ideal either, as it seems to make it harder to see that it's incomplete/still problematic. Perhaps it'd be better to remove those Resolver -1ULL calls to demonstrate the general failure & implement a general fix - maybe not in this patch, maybe in a precursor or follow-up. Not sure about breaking this temporarily rather than laying out the patches to fix this without a period of regression.
> > 
> > @echristo - got any thoughts on this?
> Taking a look through this ... we've got terrible error handling here. I'm also not a huge fan of storing the resolver inside every part of the map. That said as an incremental step it's not too bad as long as we document it a bit better explaining that the -{1,2,3}ULL mean in the if statement at what the purpose is.
> 
> Then we can probably refactor some of our relocation handling to be able to better check valid relocation first and then go from there.
> 
> WDYT?
> 
> Taking a look through this ... we've got terrible error handling here. I'm also not a huge fan of storing the resolver inside every part of the map. That said as an incremental step it's not too bad as long as we document it a bit better explaining that the -{1,2,3}ULL mean in the if statement at what the purpose is.

I think the bit I'm uncertain about is that this seems like a step backwards in terms of error coverage - not quite incremental progress. But if the existing error coverage isn't reliable in some way (it's not clear to me that it is) - yeah, I don't have strong feelings about preserving it.

> Then we can probably refactor some of our relocation handling to be able to better check valid relocation first and then go from there.

If we can refactor the error checking first so it's preserved through this change, that sounds fine.

Validating relocations up-front might be problematic from a performance perspective ( comes back to some of the design discussions around error handling in lld, etc - up-front error checking that might check too many things (when some sections are discarded, etc)).


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57939/new/

https://reviews.llvm.org/D57939