[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/CommonLinkerContext.cpp:38
+
+  for (PerThreadContext **context : perThreadContexts) {
+    for (auto &instance : (*context)->instances)
----------------
It is best if we had `llvm::sys::ScopedWriter lock(contextMutex);` here too.


================
Comment at: lld/Common/Memory.cpp:19
+// Pointer to this thread's context.
+thread_local PerThreadContext *threadContext = nullptr;
+} // namespace
----------------
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;
  }
  ...
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122922