[PATCH] D109516: [JITLink] Factor out forEachRelocation() function from addRelocations() in ELF Aarch64 backend (NFC)

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

Thanks very much for this Stefan!

Is there a reason that `forEachRelocation` works one-section-at-a-time? I thought that `forEachRelocation` could embed the loop over the sections within it to simplify addRelocations further. I.e. in most cases it should reduce to something like:

  Error addRelocations() override {
    LLVM_DEBUG(dbgs() << "Processing relocations:\n");
    return forEachRelocation(RelSect, this, &ELFJITLinker_aarch64::addSingleRelocation);
  }

There may be a good reason not to do that in ELF though?



================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELFLinkGraphBuilder.h:113-118
+  using ProcessRelocationFuncT = Error(const typename ELFT::Rela &,
+                                       const typename ELFT::Shdr &, Section &);
+
+  Error forEachRelocation(const typename ELFT::Shdr &RelSect,
+                          std::function<ProcessRelocationFuncT> F,
+                          bool ProcessDebugSections = false);
----------------
I think forEachRelocation should be a template method here, rather than taking a std::function:

```
/// Func should be callable as:
///   Error(const typename ELFT::Rela &,
///         const typename ELFT::Shdr &, Section &)

template <typename RelocHandlerFunction>
Error forEachRelocation(const typename ELFT::Shdr &RelSect,
                        RelocHandlerFunction &&Func, bool ProcessDebugSections) {
  ...
}
```

It might be handy to provide a method version for convenience too:

```
template <typename ClassT, typename RelocHandlerMethod>
Error forEachRelocation(const typename ELFT::Shdr &RelSect,
                        ClassT *Instance, RelocHandlerMethod *Method,
                        bool ProcessDebugSections) {
  return forEachRelocation(
      RelSect,
      [Instance, Method](const typename ELFT::Rela &Rel,
                         const typename ELFT::Shdr &Target,
                         Section &GS) {
        return (Instance->*Method)(Rel, Target, GS);
      },
      ProcessDebugSections);
}
```

With that, I think the iteration below could be reduced to:

```
    using Base = ELFLinkGraphBuilder<ELFT>;
    for (const auto &RelSect : Base::Sections)
      if (Error Err =
            Base::forEachRelocation(RelSect, this,
                                    &ELFJITLinker_aarch64::addSingleRelocation))
        return Err;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109516