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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added inline comments.


================
Comment at: lld/Common/Memory.cpp:19
+// Pointer to this thread's context.
+thread_local PerThreadContext *threadContext = nullptr;
+} // namespace
----------------
aganea wrote:
> oontvoo wrote:
> > aganea wrote:
> > > As things stand, you could also do `LLVM_THREAD_LOCAL PerThreadContext *threadContext = nullptr;`
> > > However since this is really just like a `static` it goes against the "*LinkerContext" concept, and since you already added some functionality in the context, you could just as well move this in `CommonLinkerContext`. You could do something like `ThreadLocal AllocContext;` (see llvm/include/llvm/Support/ThreadLocal.h). That would allocate a dynamic TLS slot on the first use, as opposed to `thread_local` which points to a pre-allocated slot in the PE static TLS table.
> > thanks! I've moved the variable to the LinkerContext class but wasn't able to use LLVM's ThreadLocal because I needed to hold on to the variable's address (to reset its value from another thread).
> > 
> > Would this "just work" with ThreadLocal?
> > 
> > ie:
> > ```
> > ThreadLocal<AllocationContext> currentThreadCtxt;
> > 
> > 
> > std::vector<ThreadLocal<AllocationContext>> allThreadContexts;
> > 
> > // collecting all the thread-local ctxt's addresses
> > allThreadContext.push_back(&currentThreadCtxt);
> > 
> > // in destroy(), reset them to sentinel value (nullptr)
> > for (auto& localCtxt : allThreadContexts) {
> >   delete localCtxt.get();
> >   localCtxt = nullptr;
> > }
> > 
> > ```
> > 
> It is not possible to take the address of the TLS slot -- one can only `.get()` or `.set()` the pointer in the `ThreadLocal` object. We needed that address to reset to sentinel in the destructor. But if moving `ThreadLocal<AllocationContext>` into `CommonLinkerContext`, we don't need to reset to a sentinel value anymore, since the TLS slot in all threads will die with `CommonLinkerContext`. If the TLS slot is reused later, it'll be reset to 0 by the system. So we could just use `std::vector<AllocationContext *> allThreadContexts` (plain pointer).
> ```
> struct CommonLinkerContext {
>   ThreadLocal<AllocationContext> currentThreadCtxt;
>   std::vector<AllocationContext *> allThreadContexts;
>   ...
> };
> 
> AllocationContext *CommonLinkerContext::allocCtxt() {
>   if (!threadContext.get()) {
>     // Context didn't exist yet for this thread, so create a new one.
>     auto *context = new AllocationContext;
>     threadContext.set(context);
> 
>     llvm::sys::ScopedWriter lock(contextMutex);
>     allThreadContexts.push_back(context);
>   }
>   return threadContext.get();
> }
> 
> CommonLinkerContext::~CommonLinkerContext() {
>   ...
>   llvm::sys::ScopedWriter lock(contextMutex);
>   for (AllocationContext *context : allThreadContexts) {
>     for (auto &instance : context->instances)
>       instance.second->~SpecificAllocBase();
>     delete context;
>   }
>   ...
> }
> ```
There's still a chance that an `AllocationContext` is in use when calling `~CommonLinkerContext()`, if all threads were not `.join`ed prior. But that should probably be solved at a higher level?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122922