[lld] r248102 - COFF: Fix thread-safety bug.

[Peter Collingbourne via llvm-commits]

On October 5, 2015 9:03:36 PM GMT+01:00, David Blaikie <dblaikie at gmail.com> wrote:
>On Mon, Oct 5, 2015 at 6:15 AM, Peter Collingbourne <peter at pcc.me.uk>
>wrote:
>
>> LTOModule owns a Module under the covers,
>
>
>Does it need to/does it (need to) use it for LLD's uses? Perhaps we
>could
>expose that as an actual stateful object rather than global
>mutable/shared
>state?

I think it does need to own an actual module. LLD needs to do things like query the target triple and symbol list from the module even before running the IR linker. (I suppose we could write a specialized bitcode reader for this purpose but not sure if worth it.)

>Ah, I see - it gets the "global" LLVMContext - it doesn't even matter
>about
>the Module that LTOModule might have.
>
>Rui: Does this code need to create the Modules (that it's creating in
>parallel) all in the same LLVMContext?

It does; this is needed in order for the IR linker to work.

> I imagine it probably does, in
>which
>case I guess there's currently nothing to do but to lock/only build one
>Module at a time. Probably.
>
>- David
>
>
>> so any client that creates multiple
>> LTOModules in the same LLVMContext will be affected.
>>
>> Maybe it's time for something like an LLVMContext-wide mutex, but
>that
>> would
>> present its own design issues (e.g. we would need to define exactly
>where
>> guards should go) so I'd prefer to let clients worry about this until
>we
>> have a good sense of the right design.
>>
>> Peter
>>
>> On Sat, Sep 19, 2015 at 06:22:47PM -0700, Rui Ueyama via llvm-commits
>> wrote:
>> > I have no idea. CC'ing Peter since he may know about that.
>> >
>> > On Sat, Sep 19, 2015 at 5:31 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>> >
>> > >
>> > >
>> > > On Sat, Sep 19, 2015 at 4:14 PM, Rui Ueyama via llvm-commits <
>> > > llvm-commits at lists.llvm.org> wrote:
>> > >
>> > >> Author: ruiu
>> > >> Date: Sat Sep 19 18:14:51 2015
>> > >> New Revision: 248102
>> > >>
>> > >> URL: http://llvm.org/viewvc/llvm-project?rev=248102&view=rev
>> > >> Log:
>> > >> COFF: Fix thread-safety bug.
>> > >>
>> > >> LTOModule doesn't seem to be thread-safe, so guard that with
>mutex.
>> > >>
>> > >
>> > > Yeah, that seems unfortunate.
>> > >
>> > > Any idea where the thread unsafety comes from & whether we
>can/should
>> fix
>> > > it down the stack rather than in lld?
>> > >
>> > >
>> > >>
>> > >> Modified:
>> > >>     lld/trunk/COFF/InputFiles.cpp
>> > >>     lld/trunk/COFF/InputFiles.h
>> > >>
>> > >> Modified: lld/trunk/COFF/InputFiles.cpp
>> > >> URL:
>> > >>
>>
>http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/InputFiles.cpp?rev=248102&r1=248101&r2=248102&view=diff
>> > >>
>> > >>
>>
>==============================================================================
>> > >> --- lld/trunk/COFF/InputFiles.cpp (original)
>> > >> +++ lld/trunk/COFF/InputFiles.cpp Sat Sep 19 18:14:51 2015
>> > >> @@ -310,6 +310,9 @@ void ImportFile::parse() {
>> > >>  }
>> > >>
>> > >>  void BitcodeFile::parse() {
>> > >> +  // Usually parse() is thread-safe, but bitcode file is an
>> exception.
>> > >> +  std::lock_guard<std::mutex> Lock(Mu);
>> > >> +
>> > >>    std::string Err;
>> > >>    M.reset(LTOModule::createFromBuffer(MB.getBufferStart(),
>> > >>                                        MB.getBufferSize(),
>> > >> @@ -356,5 +359,7 @@ MachineTypes BitcodeFile::getMachineType
>> > >>    }
>> > >>  }
>> > >>
>> > >> +std::mutex BitcodeFile::Mu;
>> > >> +
>> > >>  } // namespace coff
>> > >>  } // namespace lld
>> > >>
>> > >> Modified: lld/trunk/COFF/InputFiles.h
>> > >> URL:
>> > >>
>>
>http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/InputFiles.h?rev=248102&r1=248101&r2=248102&view=diff
>> > >>
>> > >>
>>
>==============================================================================
>> > >> --- lld/trunk/COFF/InputFiles.h (original)
>> > >> +++ lld/trunk/COFF/InputFiles.h Sat Sep 19 18:14:51 2015
>> > >> @@ -17,6 +17,7 @@
>> > >>  #include "llvm/Object/COFF.h"
>> > >>  #include "llvm/Support/StringSaver.h"
>> > >>  #include <memory>
>> > >> +#include <mutex>
>> > >>  #include <set>
>> > >>  #include <vector>
>> > >>
>> > >> @@ -213,6 +214,7 @@ private:
>> > >>    std::vector<SymbolBody *> SymbolBodies;
>> > >>    llvm::BumpPtrAllocator Alloc;
>> > >>    std::unique_ptr<LTOModule> M;
>> > >> +  static std::mutex Mu;
>> > >>  };
>> > >>
>> > >>  } // namespace coff
>> > >>
>> > >>
>> > >> _______________________________________________
>> > >> llvm-commits mailing list
>> > >> llvm-commits at lists.llvm.org
>> > >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> > >>
>> > >
>> > >
>>
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
>> --
>> Peter
>>

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.