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

[Eric Christopher via Phabricator via llvm-commits]

echristo added inline comments.


================
Comment at: ELF/DWARF.cpp:58-71
+template <class RelTy> struct RelocationResolver {
+  static uint64_t Resolve(object::RelocationRef Ref, uint64_t S,
+                          uint64_t /* A */) {
+    return S + Ref.getRawDataRefImpl().p;
+  }
+};
+
----------------
This could probably use some explanatory comments here. I know what you mean by S and S+A but let's just have it explicitly called out. :)


================
Comment at: ELF/DWARF.cpp:111-112
+  D.p = getAddend<ELFT>(Rel);
+  return RelocAddrEntry{SecIndex, RelocationRef(D, nullptr),
+                        RelocationResolver<typename ELFT::Rela>::Resolve, Val};
 }
----------------
MaskRay wrote:
> echristo wrote:
> > MaskRay wrote:
> > > dblaikie wrote:
> > > > 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?
> > > The slight weird thing here is that `RelocationResolver<typename ELFT::Rela>::Resolve` is the constant for every relocation involved. Putting it to every RelocAddrEntry is wasteful. I'll try improving this but for now I just want to get the SHT_RELA bug fixed.. D57939
> > Seems reasonable. Do you have an idea of how you want to change things?
> 1. Define another set of functions listing supported relocation types for each architecture. The function for x86_64 may look like: `switch (R) { case R_X86_64_64: case R_X86_64_PC32: return true; default: return false; }`. That would cause duplication but I cannot think of a better way if we also care about the performance (@dblaikie 's comment on D57949 made me think so)
> 2. Make heavier refactoring to the `RelocAddrEntry` call sites. Make `RelocationResolver<typename ELFT::Rela>::Resolve` a member of the class where `RelocAddrEntry` is used to resolve the relocation.
> 
> The two steps require some efforts but we probably should do that to make the code cleaner. Note, I believe the lld side DWARF code is simple enough and the amount of code added by this patch is also reasonable.
This works for me.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D57940