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

Fri Apr 15 15:39:19 PDT 2022

aganea added a comment.

In D122922#3446555 <https://reviews.llvm.org/D122922#3446555>, @mstorsjo wrote:

> In D122922#3446489 <https://reviews.llvm.org/D122922#3446489>, @oontvoo wrote:
>
>> 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.
> [...]
> 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?

There are two goals:

1. The first is the "library-ification", to allow executing any number of sequential "link sessions" from a third-party app (or even from within `lld.exe`).  See discussions in D108850 <https://reviews.llvm.org/D108850>, D110450 <https://reviews.llvm.org/D110450> and D119049 <https://reviews.llvm.org/D119049>. We need all state related to a "link session" to be contained into a "context", the `CommonLinkerContext`. Each LLD driver would then implement its own derived class, see `COFFLinkerContext` for example. This means any state cannot be global, including `thread_local` (but `llvm::sys::ThreadLocal` is fine).

2. The second is related to multithreaded cooperation, when several "link sessions" will be running in parallel in a single process. The is mainly for D86351 <https://reviews.llvm.org/D86351>, but it could be useful for a LLD-as-a-daemon server. We can discuss all this later, but I think the containing application should host a single `ThreadPool` and pass it as a reference to `lld::safeLldMain()`. Tasks from any "link session" would then be then queued on the same global `ThreadPool`.

>>> 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.

This will not satisfy point 2 above. This would prevent concurrent `CompilerLinkerContext`s, `LLVM_THREAD_LOCAL` acts like a per-thread `static`. Calls to `make<>()` would add on the same thread-local `BumpPtrAllocator`, regardless of the `CompilerLinkerContext`. The `llvm::sys::ThreadLocal` solves that by allocating & freeing a separate TLS slot for each `CompilerLinkerContext`. The code in `~CommonLinkerContext()` will otherwise not work. We can fix this later if you wish, but it all defeats the purpose of `CompilerLinkerContext`.

The 1% divergence that @MaskRay mentionned could be fixed if we passed `AllocContext` where necessary?

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

>> ! In D122922#3446376 <https://reviews.llvm.org/D122922#3446376>, @mstorsjo wrote:
>>  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.

Worth noting that `ThreadLocal` is holding a dynamically-allocated index into a runtime TLS table, whereas `thread_local` effectively adds space into the PE's static TLS table. `llvm::sys::ThreadLocal` does not suffer from the issue mentioned by @mstorsjo.

================
Comment at: lld/include/lld/Common/CommonLinkerContext.h:50
+  // The AllocContext shared by all threads.
+  AllocContext allocContext;

----------------
Change to `globalContext`, as opposed to `perThreadContext`?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122922