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

Tue Apr 12 12:44:38 PDT 2022

oontvoo added a comment.

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

> (So far I would expect that lld doesn't do any allocations from threaded code?

why not?  can you expand on this?

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

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122922