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

Tue Apr 12 06:35:43 PDT 2022

oontvoo added inline comments.

================
Comment at: lld/CMakeLists.txt:228
+option(LLD_THREAD_SAFE_MEMORY
+    "Make the bump pointer allocator and StringSaver thread safe." OFF)
+if (LLD_THREAD_SAFE_MEMORY)
----------------
int3 wrote:
> I'm generally not a fan of adding compile-time options as I think it increases the number of potentially poorly-tested code paths. Maybe we could just use `LLD_ENABLE_THREADS` to gate the code currently protected by `LLVM_THREAD_SAFE_MEMORY`.
> 
> But I guess having things behind a compile-time flag for now will make this diff easier to land. It's conceivable that some buildbots may be unhappy with this depending on e.g. the target-specific support for TLS. Can we follow up with a diff that makes the thread-safe behavior the default? If that passes all the buildbots, I think we should make it the default behavior.
(sorry - missed this comment previously)

Good idea on using the `LLD_ENABLE_THREADS`! The reason  I added this additional variable was because I wanted to give other ports an option to opt-out of this if they choose to, for whatever reasons.
So if no one objects to enabling this unconditionally (or via the LLD_ENABLE_THREADS), then happy to remove this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122922