[PATCH] D105466: [RuntimeDyld] Implemented relocation of TLS symbols in ELF

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

What was the previous behavior when we encountered these relocations?

My only concern here is avoiding regressing existing users if we turn a previously unsupported (but silently accepted) relocation into a potential link-time error. It's perverse, but this kind of improvement to functionality and error checking has caused problems for clients before, and the main aim for MCJIT / RuntimeDyld right now is stability.

If the new support could cause a problem then I think the best solution would be to add a 'bool supportsTLV' method to the memory manager that we can query before running any of the TLV code.



================
Comment at: llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:1861
       processSimpleRelocation(SectionID, Offset, RelType, Value);
+    } else if (RelType == ELF::R_X86_64_GOTTPOFF) {
+      // Use the approach from "x86-64 Linker Optimizations" from the TLS spec
----------------
This function is pretty big already. Could the TLV code be split out into separate functions for readability?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105466