[PATCH] D73254: [ELF] Rename relocateOne() to relocate() and pass `Relocation` to it

Nick Desaulniers via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jan 23 09:34:05 PST 2020


nickdesaulniers added a comment.

Thanks for the patch!



================
Comment at: lld/ELF/Target.h:91
+    relocate(loc, rel, val);
+  }
 
----------------
It looks like there's just a few more callers of `relocateOne` in:
* ELF/AArch64ErrataFix.cpp
* ELF/ARMErrataFix.cpp
* ELF/InputSection.cpp

I think if you updated those too, you could drop this change to `relocateOne` and then this patch would be simply renaming `relocateOne` to `relocate` and taking a full `Relocation` object rather than a `RelType`.

You might even give `struct Relocation` an non-`explicit` constructor that way you could implicitly construct a `Relocation` via a `RelType`.  Then calls to `relocateOne` with a literal `RelType` would only have to be renamed, and you wouldn't have to add code to contruct `Relocations` at callsites.

(It looks like `struct Relocation` in llvm/tools/llvm-objcopy/COFF/Object.h already does something to this effect, for prior art). Oh man, so many different `Relocation` structs across the codebase...


================
Comment at: lld/ELF/Target.h:246
     error(getErrorLocation(loc) + "improper alignment for relocation " +
-          lld::toString(type) + ": 0x" + llvm::utohexstr(v) +
+          lld::toString(rel.type) + ": 0x" + llvm::utohexstr(v) +
           " is not aligned to " + Twine(n) + " bytes");
----------------
Are the changes to this file necessary? Is the full `Relocation` object used?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73254





More information about the llvm-commits mailing list