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

Sun Sep 5 15:23:40 PDT 2021

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

In D105466#2969076 <https://reviews.llvm.org/D105466#2969076>, @MoritzS wrote:

> In D105466#2968873 <https://reviews.llvm.org/D105466#2968873>, @lhames wrote:
>
>> 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.
>
> All relocations that are not implemented will eventually lead to `report_fatal_error("Relocation type not implemented yet!");` in `resolveX86_64Relocation()`. Whereas with my changes it will reach `report_fatal_error("allocation of TLS not implemented");` in `MemoryManager::allocateTLSSection()`. So in both cases the execution crashes, it just prints a different error message now.

Ok. I've just realized that there's also a danger on the flip-side too: code containing TLVs will be accepted, but this only support accessing them on one thread, which will be surprising to some clients. I think it would be worth adding an "enablePartialTLVSupport" method to the memory manager (which should default to false), and gating this new feature on that. If you're happy to do that before the code lands then that's great, otherwise I'm happy to do it in-tree after it lands.

Thank you again for all your work on this Moritz -- it's really appreciated!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105466