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

Mon Apr 11 17:54:07 PDT 2022

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

lgtm. Let's update the commit message, in particular

> Usage: code that might be run in different threads should call the new perThreadSaver() or makePerThread()

instead of the current util (saver() and make())

also

> TLS support might not be available on all archs. (Darwin?)

Is this still an ongoing concern? (Why do you think TLS support might not be available on Darwin?)

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122922