[PATCH] D135523: Adds absolute and pc relative relocation support for ELF/i386

Mon Oct 17 02:04:42 PDT 2022

sgraenitz added inline comments.

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELFLinkGraphBuilder.h:505
+Error ELFLinkGraphBuilder<ELFT>::forEachRelaRelocation(
+    const typename ELFT::Shdr &RelSect, RelocHandlerMethod &&Func,
     bool ProcessDebugSections) {
----------------
jain98 wrote:
> sgraenitz wrote:
> > Nit: We should keep typename Function here as well as for forEachRelRelocation()
> I can change it back to "RelocHandlerFunction". I purposely changed it to "RelocHandlerMethod" because "Func" argument was a member function (which at least in Rust are just referred to as methods. Perhaps they're not called the same thing in C++). Is there a reason why you named the template parameter "RelocHandlerFunction"?
Yes, they are not the same thing. Methods can be subject to dynamic dispatch. Just to clarify, there are two overloads and the typenames are chosen on purpose:
(1) forEachRelocation<RelocHandlerFunction> takes a function (static member or freestanding function)
(2) forEachRelocation<ClassT, RelocHandlerMethod> takes an object and a method (regular member function)

The one we talk about here is (1) and I think it should remain unchanged. forEachRelRelocation() should follow this pattern.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135523