[PATCH] D113228: [RFC][ELF] Refactor relocation processing

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

On a first look I like the way this is going. There is more information in a single place. I've made a few suggestions but none are particularly important for an RFC. If I get time I'll keep thinking to see if I can think of any other suggestions.



================
Comment at: lld/ELF/Relocations.cpp:224
 // dynamic relocation so that the relocation will be fixed at load-time.
 static bool isStaticLinkTimeConstant(RelExpr e, RelType type, const Symbol &sym,
                                      InputSectionBase &s, uint64_t relOff) {
----------------
Is it worth moving closer to the only caller processRelocAux. Perhaps worth emphasisng that this only handles relocations that are expected in processRelocaAux. Could do with a comment or perhaps renaming to something like `customRelocIsStaticLinkTimeConstant`

If there were a way to know if a particular expr code could be CUSTOM outside of scanReloc then we could assert that which would make it clear, but I'm not sure if there is an easily maintainable way to do that.


================
Comment at: lld/ELF/Relocations.cpp:227
   // These expressions always compute a constant
-  if (oneof<R_GOTPLT, R_GOT_OFF, R_MIPS_GOT_LOCAL_PAGE, R_MIPS_GOTREL,
-            R_MIPS_GOT_OFF, R_MIPS_GOT_OFF32, R_MIPS_GOT_GP_PC,
-            R_AARCH64_GOT_PAGE, R_AARCH64_GOT_PAGE_PC, R_GOT_PC, R_GOTONLY_PC,
+  if (oneof<R_TLSLD_GOT_OFF, R_AARCH64_GOT_PAGE_PC, R_GOT_PC, R_GOTONLY_PC,
             R_GOTPLTONLY_PC, R_PLT_PC, R_PLT_GOTPLT, R_PPC32_PLTREL,
----------------
I'm guessing that some of the entries have been taken out as the relocations were not CUSTOM so they would never expect to be called?

Perhaps worth changing the comment
// The following expressions always compute a constant. The list does not contain expressions marked RELOCATE in scanReloc.


================
Comment at: lld/ELF/Relocations.cpp:952
 
+static void cantUseRelocation(InputSectionBase &sec, uint64_t offset,
+                              RelType type, Symbol &sym) {
----------------
suggest `cantUseRelocationAgainstLocal` or `invalidRelocToLocal`


================
Comment at: lld/ELF/Relocations.cpp:1161
 static unsigned
 handleTlsRelocation(RelType type, Symbol &sym, InputSectionBase &c,
                     typename ELFT::uint offset, int64_t addend, RelExpr expr) {
----------------
Perhaps worth renaming to handleTlsNonIE


================
Comment at: lld/ELF/Relocations.cpp:1371
+#define OPTIMIZE_PLT (!sym.isPreemptible && !sym.isGnuIFunc())
+#define toExecRelax                                                            \
+  (!config->shared && config->emachine != EM_ARM &&                            \
----------------
This is also in handleTlsRelocation, worth making an inline function? Something like `toExecRelax()`


================
Comment at: lld/ELF/Relocations.cpp:1444
-  } else {
-    // Handle a reference to a non-preemptible ifunc. These are special in a
-    // few ways:
----------------
Would be a shame to lose the information in the comment, even if it had to be moved/split up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113228