[PATCH] D122922: [lld][common][lld-macho][lld-coff] Support per-thread allocators and StringSavers
Alexandre Ganea via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 7 10:03:19 PDT 2022
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(¤tThreadCtxt);
>
> // 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
More information about the llvm-commits
mailing list