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

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

This is a larger change than I anticipated.  I was surprised by the need to have the pointer be thread local (and then ensure that the thread local pointers for new threads are set up, too).  If I'm understanding correctly, the idea is that a single program could have n threads doing n links at the same time.  Is that right?  I'm not arguing against that.  I'm just trying to make sure I understand why I was surprised.

It also introduces more indirection than I expected.  I believe Rui (and others) favored globals in order to go as fast as possible.  Are there lld benchmarks to give us an idea of the cost of this?  If there's pushback from the performance die-hards, it would be nice to have an actual measurement or two to show (I hope) that the cost for this flexibility is low.

Anyway, this looks pretty good to me.  I'm not well-versed in lld, and--honestly--I didn't scrutinize all the changes that looked mostly mechanical.



================
Comment at: lld/COFF/Chunks.cpp:12
+#include "SymbolTable.h"
 #include "Symbols.h"
 #include "Writer.h"
----------------
I guess we're using ASCIIbetical rather than alphabetical ordering. :-)


================
Comment at: lld/MachO/DriverUtils.cpp:263
       if (exists)
-        return saver.save(location.str());
+        return saver(&cg).save(location.str());
     }
----------------
In most cases, it's just `saver()`.  Is there something special about these instances where you explicitly pass a pointer to the common globals?


================
Comment at: lld/include/lld/Common/Globals.h:67
+// Detach the global state to prevent accidental re-use betwen tasks scheduled
+// on the same thread. The thread pool could possibly be shared beween LLD
+// instances in the future.
----------------
s/beween/between/


================
Comment at: lld/tools/lld/lld.cpp:190
+  // Delete global state.
+  delete cg;
+  return !r ? 1 : 0;
----------------
Given that this is the not-exiting-early path, should this thread detach from the global state before deleting it?  That might help catch instances where something in the chain of destructors inadvertently tries to reference a global.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108850