[PATCH] D120073: [LLD] Fix for race condition.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/InputSection.cpp:122-123
   {
-    static std::mutex mu;
     std::lock_guard<std::mutex> lock(mu);
     uncompressedBuf = bAlloc().Allocate<char>(size);
----------------
ayermolo wrote:
> wenlei wrote:
> > ayermolo wrote:
> > > wenlei wrote:
> > > > IIUC, uncompress is currently in an inconsistent state between having thread safety and not. If we don't actually intend to make uncompress thread safe, perhaps this mutex can also be removed? 
> > > > 
> > > > Agree that downstream usage doesn't necessarily justify making the upstream implementation thread safe. Though it would be good to make it clear whether uncompress is supposed to be thread safe or not.
> > > Original diff that added the mutex: https://reviews.llvm.org/D59269
> > Ok, thanks for the pointer. Then it does look like the intention was to make it thread safe. @MaskRay forget about downstream usage, do we want this function to be thread safe or not? 
> @MaskRay can you comment on @wenlei question?
The original code doesn't allow one object to be concurrently uncompressed by two threads.
The new code wants to allow this possibility, which is unusual in the lld code base (generally we only require the objects to be thread-compatible).

Since achieving this unusual thing makes the code slightly more complex. I'll say the answer is no.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120073