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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 3 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:
> 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).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57939