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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 5 inline comments as done.
MaskRay 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.
----------------
dblaikie wrote:
> 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)).
Deleted -2/-3 hack and added supports* functions.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57939