[PATCH] D122922: [lld][common][lld-macho][lld-coff] Support per-thread allocators and StringSavers

Tue Apr 12 13:25:44 PDT 2022

mstorsjo added a comment.

In D122922#3446489 <https://reviews.llvm.org/D122922#3446489>, @oontvoo wrote:

>>> ! In D122922#3446376 <https://reviews.llvm.org/D122922#3446376>, @mstorsjo wrote:
>>
>> Can someone summarize the high level roadmap of this feature? If threadsafe allocation is going to be optional/opt-in, lld can't generally rely on that, right? Or is it meant as a gradual way of merging the code and trying it out, and after it's ready to be enabled unconditionally, it can be relied upon in lld in general?
>
> Yes, as it stands, the threadsafety is controlled by the new `LLD_THREAD_SAFE_MEMORY`. The intention is for each LLD ports to decide whether they want threadsafe allocator/saver (and not for *users* of lld).
> For MachO, I think we'd like to enable it (already been hit by a few race-condition bugs). Can't speak for ELF/others ...

Right, but if the MachO part of lld does allocations from multiple threads, then it's essentially unreliable unless this option is enabled? So anyone building lld wanting to use the MachO part of it would need to enable it.

>> (So far I would expect that lld doesn't do any allocations from threaded code?
>
> why not?  can you expand on this?

I'm mostly familiar with the COFF parts of lld, and there, most of the linker logic is serial, and parallelism is only used for short blocks of `parallelForEach` or `parallelSort`, where no allocations are assumed to be done.

>> And for the recent/upcoming refactorings to make lld usable as a library, the per-session context should hold the allocator, right?)
>
> not super familiar with this effort. What is the scope of this "per-session" ? Is is per-process? If so, it doesn't solve the problem (here we have multiple threads using the same bptrAlloc, hence the race condition)

No, it's meant so that you can use lld as a library, so that one process can have multiple threads running in parallel, where more than one such thread can call e.g. `lld::coff::link()` at the same time. I haven't followed the state of the work on that in very close detail, but as far as I understand, the implementation strategy has been to gather everything that previously was global, into a per-invocation context, that is passed around (and/or stored as a thread local variable somewhere I think). So within each linker invocation, you'd only ever have allocations happening on one thread, the one where the user called `lld::*::link()` - each such linker job then would run multiple threads for occasional parallelism in the linking process though.

Does the MachO linker run longer lived threads within one linker invocation, where any of them can do allocations?

I think it'd be valuable to align these two thread safety efforts so we don't end up with two mechanisms for doing the same. I think @aganea has been involved in that effort though. @aganea - can you chime in here?

>> Secondly, about the choice of mechanism for the thread local data, I'm not familiar with the implications of `llvm::sys::ThreadLocal`, but for plain C++ `thread_local`, it's generally preferable if the thread local variable is made e.g. static within one single source file (with an accessor), instead of being declared in a header, accessed from any translation unit. If being accessed directly from multiple translation units, the use of the thread local variable incurs some amount of extra tls wrapper functions and weak symbols, which are occasionally broken in e.g. GCC on Windows. (See e.g. D111779 <https://reviews.llvm.org/D111779>, where a TLS variable in LLDB was moved this way, to fix GCC builds of it.)
>
> Right, the native TLS variable would be static (not in a header). Pls See updated diff.

Thanks, so `LLVM_THREAD_LOCAL AllocContext *threadContext = nullptr;` should indeed work just fine.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122922