[lld] r241420 - COFF: Make ArchiveFile::getMember thread-safe.

[Rui Ueyama]

On Sun, Jul 5, 2015 at 4:16 PM, Chandler Carruth <chandlerc at google.com>
wrote:

> On Sun, Jul 5, 2015 at 3:52 PM Rui Ueyama <ruiu at google.com> wrote:
>
>> Author: ruiu
>> Date: Sun Jul  5 17:50:00 2015
>> New Revision: 241420
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=241420&view=rev
>> Log:
>> COFF: Make ArchiveFile::getMember thread-safe.
>>
>> This function is called SymbolTable::readObjects, so in order to
>> parallelize that function, we have to make this function thread-safe.
>>
>
> I think it would be better to make an entire interface thread-safe, and
> document its thread safety guarantees than to do this one function at a
> time.
>

What do you mean by "entire interface"? Do you want to make all member
functions of all InputFile classes thread-safe? Most member functions don't
have to be thread-safe, and this is exceptional.

Even this function is not necessary to be thread-safe. We can make this
function to always return a memory buffer, and we can check for duplicates
on calling side.

Also, have you thought about a testing strategy for the concurrency
> functionality you're adding? I'm wondering if this would be a place where
> unittesting would be effective. There you could spawn a collection of
> threads with a lambda to query the thread safe API and ensure they all
> produce the same result. When run with ThreadSanitizer, this would provide
> a pretty useful way to ensure the APIs that are documented as thread safe
> are in fact safe.
>

I was thinking to test this with ThreadSanitizer and regular lit tests to
check for thread-unsafe bugs. Is this different from unit tests?


>
>> Modified:
>>     lld/trunk/COFF/InputFiles.cpp
>>
>> Modified: lld/trunk/COFF/InputFiles.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/InputFiles.cpp?rev=241420&r1=241419&r2=241420&view=diff
>>
>> ==============================================================================
>> --- lld/trunk/COFF/InputFiles.cpp (original)
>> +++ lld/trunk/COFF/InputFiles.cpp Sun Jul  5 17:50:00 2015
>> @@ -18,6 +18,7 @@
>>  #include "llvm/Support/Debug.h"
>>  #include "llvm/Support/Endian.h"
>>  #include "llvm/Support/raw_ostream.h"
>> +#include <mutex>
>>
>>  using namespace llvm::object;
>>  using namespace llvm::support::endian;
>> @@ -76,7 +77,9 @@ std::error_code ArchiveFile::parse() {
>>  }
>>
>>  // Returns a buffer pointing to a member file containing a given symbol.
>> +// This function is thread-safe.
>>  ErrorOr<MemoryBufferRef> ArchiveFile::getMember(const Archive::Symbol
>> *Sym) {
>> +  static std::mutex Mu;
>>
>
> A function-local static mutex is more expensive than necessary.
>
> 1) It is a global lock, which seems like it should be avoided.
>
> 2) It requires a thread-safe initialization check on each call through the
> function in addition to the mutex lock.
>
> It would seem more natural for the ArchiveFile object to carry the mutex
> used to establish thread safety for its member functions.
>
>
>>    auto ItOrErr = Sym->getMember();
>>    if (auto EC = ItOrErr.getError())
>>      return EC;
>> @@ -84,7 +87,9 @@ ErrorOr<MemoryBufferRef> ArchiveFile::ge
>>
>>    // Return an empty buffer if we have already returned the same buffer.
>>    const char *StartAddr = It->getBuffer().data();
>> +  Mu.lock();
>>    auto Pair = Seen.insert(StartAddr);
>> +  Mu.unlock();
>>    if (!Pair.second)
>>      return MemoryBufferRef();
>>    return It->getMemoryBufferRef();
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150705/7d0966d6/attachment.html>