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

Mon Feb 18 15:59:43 PST 2019

dblaikie added inline comments.

================
Comment at: ELF/DWARF.cpp:111-112
+  D.p = getAddend<ELFT>(Rel);
+  return RelocAddrEntry{SecIndex, RelocationRef(D, nullptr),
+                        RelocationResolver<typename ELFT::Rela>::Resolve, Val};
 }
----------------
ruiu wrote:
> I found this interface a bit awkward. Passing a DataRefImpl instance just to pass a single integer value seems odd to me.
> 
> I think my question is, is it that hard to read debug info? We don't necessarily have to use the library to read debug info, and if we can just read debug info with a small amount of code, that could be a better way. I don't know if that's the case, but that's something I want to investigate.
If there's something awkward about LLVM's DWARF reading APIs, I'd love to see them improved (open to suggestions/requests/design discussions/patches) rather than forked/partially reimplemented in LLD, if at all reasonable/possible, for what it's worth.

DataRefImpl is a union of the integer and some other alternatives, by the looks of it - so I'm not sure I see this as being especially odd/awkward. I guess it's because some clients want to describe the parameter in one way, and some in another. If it's particularly hard to read here, there could be a convenience wrapper function that packs the parameter into a DataRefImpl/RelocationRef/etc?

Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D57940