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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

In D108850#3000699 <https://reviews.llvm.org/D108850#3000699>, @mehdi_amini wrote:

> The mechanism introduced in Parallel.h in this revision (wrapThreadState) seems mostly like some sort of "workaround" to be able to continue to rely on some global way to pass state around in `lld` instead of having something more explicit, which we favored in MLIR (where like in LLVM, the Context is fairly centric and not accessed through a global).

It was totally a temporary workaround, until we explicitly pass contexts around. I've tried passing contexts around, but it is a big change and I'm not sure it's worth it. Removed this code for now.

In D108850#3000527 <https://reviews.llvm.org/D108850#3000527>, @rnk wrote:

> I think this should be staged into two patches:
>
> 1. LLD-only: includes all of the Context changes, `saver()`, etc, but the context is still kept in a global pointer.

Updated the patch to only do 1. like you suggested.



================
Comment at: lld/Common/ErrorHandler.cpp:109
     lld::outs().flush();
     lld::errs().flush();
   }
----------------
mehdi_amini wrote:
> I'm curious about this change: you're protecting what seems to be global methods with a non-global mutex. The code comment for the mutex says ```
>   // The functions defined in this file can be called from multiple threads,
>   // but lld::outs() or lld::errs() are not thread-safe. We protect them using a
>   // mutex.
> ```
> 
> This made sense when the mutex was global, less so now. Can you clarify this? (and update the comment)
Before, the mutex and the functions were global. They both have the same lifetime as the call to `lldMain()`. I was planning on supporting several concurrent calls to `lldMain()` in the future. Updated the comment.


================
Comment at: llvm/lib/Support/Parallel.cpp:87
 
+  std::function<void(std::function<void()> &)> ThreadStateFn;
+
----------------
mehdi_amini wrote:
> Seems like worth documenting.
Removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108850