[PATCH] D108850: [LLD] Remove global state in lldCommon

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

Thanks for taking a look!

In D108850#3099397 <https://reviews.llvm.org/D108850#3099397>, @smeenai wrote:

> I see a bunch of discussions about thread-locals; was that for an earlier version of this patch, or am I just missing something? (I don't see anything thread-local related in the current version of the patch.)

Yes, please take a look at "Diff 4" and before, `lctx` used to be a `THREAD_LOCAL` to accommodate for several instances of LLD running in the same process. But @rnk suggested to go in steps, and first move all scattered statics into a global static context.

> To make sure I'm understanding this correctly, can the calls to `freeArena` be removed because the destructor for the context will take care of that now?

Yes.



================
Comment at: lld/COFF/InputFiles.cpp:1069
 void BitcodeFile::parse() {
+  llvm::StringSaver &saver = lld::saver();
   std::vector<std::pair<Symbol *, bool>> comdat(obj->getComdatTable().size());
----------------
smeenai wrote:
> What was the motivation for saving to a local variable here? Is it the use of `saver` in the loop below? (It makes sense to me; I just wanted to confirm my understanding.)
Yes, precisely, to clarify the intention that `saver` shouldn't change in that loop.


================
Comment at: lld/Common/CommonLinkingContext.cpp:22
+
+CommonLinkingContext &lld::commonContext() {
+  assert(lctx);
----------------
smeenai wrote:
> It'd be nice if there was a way to put this in the header, so that it could be inlined even in non-LTO builds, but that'd prevent `lctx` from being file-static, which seems undesirable. I imagine LTO or ThinLTO would be happy to inline this (and I'll confirm when I do performance measurements), so it shouldn't matter too much in the end.
Did you have the chance to run any tests?


================
Comment at: lld/Common/CommonLinkingContext.cpp:23
+CommonLinkingContext &lld::commonContext() {
+  assert(lctx);
+  return *lctx;
----------------
smeenai wrote:
> I'm a bit concerned about the potential performance implications of this `assert`, but I'll measure first.
Since this would only trigger in `LLVM_ENABLE_ASSERTIONS=ON` builds, the performance shouldn't matter that much? Or should it? Otherwise we could only have it in `LLVM_ENABLE_EXPENSIVE_CHECKS` builds?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108850