[PATCH] D57939: Refactor RelocVisitor and fix computation of SHT_RELA-typed relocation entries
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 21 09:52:24 PST 2019
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
More information about the llvm-commits
mailing list