[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.
----------------
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?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57939