[PATCH] Change base mutex implementations to use STL-provided mutexes

Zachary Turner zturner at google.com
Fri Jun 6 12:57:28 PDT 2014


The motivation is just that we should be relying on standard
implementations as they're more likely to be robust, battle tested, and
higher performance than our own hand-rolled implementations.  For example,
there are many locations in the code that roll a custom version of
once-only initialization.  As a future task, I would like to convert all of
them to std::call_once.  Just as a general principle of not re-inventing
the wheel.

As for the platform specific header includes, I guess that's ultimately a
separate discussion, and this change can be made without introducing
platform-specific header includes into Mutex.h.  I am currently working on
a revision to this patch which removes the windows.h include from mutex.h,
so I will upload that later.


On Fri, Jun 6, 2014 at 12:32 PM, Alp Toker <alp at nuanti.com> wrote:

> Hi Zachary,
>
> In LLVM platform-specific code goes in the implementation files (.inc) and
> never in the portable headers. Certainly for a high-level class like Mutex
> I wouldn't expect to see platform-specific conditionals exposed.
>
> A couple of exceptions have slipped in like Support/FileSystem.h and
> Support/Process.h with ifdef WIN32 but those are meant to get fixed at some
> point.
>
> On a more practical level it's not a good idea to include windows.h
> globally throughout the public headers. These headers need to get included
> in portable software that embeds clang for tooling etc. and windows.h is
> known to be problematic.
>
> For thread primitives, the previous implementation looks broadly correct.
> You've explained the mechanics of your changes in detail but can you
> explain the motivation here? We need to find a better way to achieve that
> without introducing all these OS-specific low-level headers globally into
> embedding applications.
>
> Alp.
>
>
>
>
> On 05/06/2014 20:06, Zachary Turner wrote:
>
>> Hi rnk, aaron.ballman, chandlerc,
>>
>> This is a re-submission of an earlier broken patch.
>>
>> Change llvm::sys::Mutex implementation to use std::mutex and
>> std::recursive_mutex.
>>
>> This patch deletes the platform specific mutex implementations, and
>> delegates to the underlying STL implementation. Because STL mutex and
>> recursive_mutex are implemented as separate classes, it is no longer
>> possible to specify as a runtime param whether a recursive mutex is
>> desired, and instead the decision must be made at compile time.
>>
>> As part of this refactor, the ScopedLock class is deleted in favor of
>> MutexGuard, which provides equivalent functionality.
>>
>> To make reviewing easier, you can refer to the following table of
>> equivalences in the format (old_type -> new_type)
>>
>> SmartMutex<true>([true]) -> RecursiveDebugMutex
>>
>> SmartMutex<false>([true]) -> RecursiveMutex
>> sys::Mutex -> SmartMutex<false>() -> RecursiveMutex
>>
>> SmartMutex<true>(false) -> NonRecursiveDebugMutex
>> SmartMutex<false>(false) -> NonRecursiveMutex
>>
>> One caveat of note is that on Windows std::*mutex cannot be used during
>> an atexit handler.  To get around this, a new type AtexitSafeMutex is
>> defined, and on Windows this is typedefed to a critical section.
>>
>> http://reviews.llvm.org/D4033
>>
>> Files:
>>    include/llvm/ExecutionEngine/ExecutionEngine.h
>>    include/llvm/IR/ValueMap.h
>>    include/llvm/Support/Mutex.h
>>    include/llvm/Support/MutexGuard.h
>>    lib/CodeGen/PseudoSourceValue.cpp
>>    lib/CodeGen/SelectionDAG/SelectionDAG.cpp
>>    lib/ExecutionEngine/ExecutionEngine.cpp
>>    lib/ExecutionEngine/Interpreter/ExternalFunctions.cpp
>>    lib/ExecutionEngine/JIT/JIT.cpp
>>    lib/ExecutionEngine/JIT/JITEmitter.cpp
>>    lib/ExecutionEngine/OProfileJIT/OProfileWrapper.cpp
>>    lib/ExecutionEngine/RuntimeDyld/GDBRegistrar.cpp
>>    lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h
>>    lib/IR/LeakDetector.cpp
>>    lib/IR/LegacyPassManager.cpp
>>    lib/Support/CMakeLists.txt
>>    lib/Support/CrashRecoveryContext.cpp
>>    lib/Support/DynamicLibrary.cpp
>>    lib/Support/Mutex.cpp
>>    lib/Support/PluginLoader.cpp
>>    lib/Support/Statistic.cpp
>>    lib/Support/Threading.cpp
>>    lib/Support/Timer.cpp
>>    lib/Support/Unix/Mutex.inc
>>    lib/Support/Unix/Process.inc
>>    lib/Support/Unix/Signals.inc
>>    lib/Support/Windows/DynamicLibrary.inc
>>    lib/Support/Windows/Mutex.inc
>>    lib/Target/NVPTX/NVPTXUtilities.cpp
>>    unittests/IR/ValueMapTest.cpp
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140606/7c8a4af6/attachment.html>


More information about the llvm-commits mailing list