Hi,

Just a quick follow-up to let you know that both CheckedLock/CheckedCondition 
and UncheckedLock/UncheckedCondition have been removed from the tree.
The whole codebase has been ported to Lock/Condition, which have the thread 
safety analysis annotations.

--
 Chris Dumez




> On May 23, 2021, at 10:40 PM, Chris Dumez <cdu...@apple.com> wrote:
> 
> Clang Thread Safety Analysis
> WTF::Lock now supports Clang Thread Safety Analysis 
> <https://clang.llvm.org/docs/ThreadSafetyAnalysis.html>. It is a C++ language 
> extension which warns about potential race conditions in code, at compile 
> time. It is the same great Lock, but with some extra help from clang to make 
> our multi-threaded code safer by giving out compilation errors when thread 
> unsafe code is detected.
> 
> Annotations
> Just by using WTF::Lock, Clang will already be able to do some basic 
> validation. However, to take full advantage of it, there are a few 
> annotations you should know about. Note that you'll see those annotations 
> throughout the code base already as a few of us have been busy adopting them. 
> All these annotations are declared in wtf/ThreadSafetyAnalysis.h 
> <http://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/ThreadSafetyAnalysis.h>.
> 
> WTF_GUARDED_BY_LOCK()
> 
> WTF_GUARDED_BY_LOCK is an attribute on data members, which declares that the 
> data member is protected by the given lock. Read operations on the data 
> require shared access, while write operations require exclusive access.
> 
> WTF_POINTEE_GUARDED_BY_LOCK is similar, but is intended for use on pointers 
> and smart pointers. There is no constraint on the data member itself, but the 
> data that it points to is protected by the given lock.
> 
> class Foo {
>     Vector<String> m_strings WTF_GUARDED_BY_LOCK(m_stringsLock);
>     Lock m_stringsLock;
> };
> You will get a compiler error if you try to access or modify m_strings 
> without grabbing the m_stringsLock lock first.
> 
> WTF_REQUIRES_LOCK()
> 
> WTF_REQUIRES_LOCK is an attribute on functions or methods, which declares 
> that the calling thread must have exclusive access to the given locks. More 
> than one lock may be specified. The locks must be held on entry to the 
> function, and must still be held on exit.
> 
> class Foo {
>     void addString(String&&) WTF_REQUIRES_LOCK(m_stringsLock);
> 
>     Vector<String> m_strings WTF_GUARDED_BY_LOCK(m_stringsLock);
>     Lock m_stringsLock;
> };
> You will get a compiler error if you try to call addString() without grabbing 
> the m_stringsLock lock first. This also allows to compiler to know that the 
> addString() implementation can modify the m_strings data member without 
> grabbing the lock first.
> 
> Another good use case:
> 
> static Lock globalCacheLock;
> static HashMap<String, String>& globalCache() 
> WTF_REQUIRES_LOCK(globalCacheLock)
> {
>     static NeverDestroyed<HashMap<String, String>> globalCache;
>     return globalCache;
> }
> This will force all call sites to grab the globalCacheLock lock before 
> calling globalCache().
> 
> Previously, we may have passed a LockHolder& as parameter to try and convey 
> that. We have been updating code to stop passing such parameters though as it 
> is no longer useful.
> 
> WTF_ACQUIRES_LOCK() / WTF_RELEASES_LOCK()
> 
> WTF_ACQUIRES_LOCK is an attribute on functions or methods declaring that the 
> function acquires a lock, but does not release it. The given lock must not be 
> held on entry, and will be held on exit.
> 
> WTF_RELEASES_LOCK declares that the function releases the given lock. The 
> lock must be held on entry, and will no longer be held on exit.
> 
> class Foo {
> public:
>     void suspend() WTF_ACQUIRES_LOCK(m_suspensionLock)
>     {
>         m_suspensionLock.lock();
>         m_isSuspended = true;
>     }
>     void resume() WTF_RELEASE_LOCK(m_suspensionLock)
>     {
>         m_isSuspended = false;
>         m_suspensionLock.unlock();
>     }
> 
> private:
>     Lock m_suspensionLock;
>     bool m_isSuspended;
> };
> Without these annotations, you'd get a compiler error stating that you failed 
> to unlock m_suspensionLock before returning in suspend() and that you 
> unlocked m_suspensionLock in resume() even though it was not previously 
> locked.
> 
> WTF_RETURNS_LOCK()
> 
> WTF_RETURNS_LOCK is an attribute on functions or methods, which declares that 
> the function returns a reference to the given lock.
> 
> class Foo {
> public:
>     Lock& lock() WTF_RETURNS_LOCK(m_lock) { return m_lock; }
> private:
>     Lock m_lock;    
> };
> WTF_EXCLUDES_LOCK()
> 
> WTF_EXCLUDES_LOCK is an attribute on functions or methods, which declares 
> that the caller must not hold the given lock. This annotation is used to 
> prevent deadlock. Many mutex implementations are not re-entrant, so deadlock 
> can occur if the function acquires the mutex a second time.
> 
> class Foo {
>     void addString(String&& string) WTF_EXCLUDES_LOCK(m_stringsLock)
>     {
>         Locker locker { m_stringsLock };
>         m_string.append(WTFMove(string));
>     }
>     Vector<String> m_strings WTF_GUARDED_BY_LOCK(m_stringsLock);
>     Lock m_stringsLock;
> };
> WTF_IGNORES_THREAD_SAFETY_ANALYSIS
> 
> WTF_IGNORES_THREAD_SAFETY_ANALYSIS is an attribute on functions or methods, 
> which turns off thread safety checking for that method. It provides an escape 
> hatch for functions which are either (1) deliberately thread-unsafe, or (2) 
> are thread-safe, but too complicated for the analysis to understand. Reasons 
> for (2) are be described in the Known Limitations 
> <https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#limitations>.
> 
> class Foo {
>     void unsafeIncrement() WTF_IGNORES_THREAD_SAFETY_ANALYSIS
>     {
>         ++m_counter;
>     }
>     unsigned m_counter WTF_GUARDED_BY_LOCK(m_counterLock) { 0 };
>     Lock m_counterLock;
> };
> Unlike the other attributes, WTF_IGNORES_THREAD_SAFETY_ANALYSIS is not part 
> of the interface of a function, and should thus be placed on the function 
> definition (in the .cpp file) rather than on the function declaration (in the 
> header).
> 
> assertIsHeld()
> 
> These are attributes on a function or method which asserts the calling thread 
> already holds the given lock, for example by performing a run-time test and 
> terminating if the lock is not held. Presence of this annotation causes the 
> analysis to assume the lock is held after calls to the annotated function.
> 
> class Foo {
>     void waitForString()
>     {
>         Locker locker { m_stringsLock };
>         m_stringsCondition.wait(m_stringsLock, [&] {
>             assertIsHeld(m_stringsLock); // Needed as the compiler is not 
> able to tell we have the lock.
>             return !m_strings.isEmpty();
>         });
>     }
> 
>     void populateFromProviders()
>     {
>         Locker locker { m_stringsLock };
>         forEachProvider([&](Provider& provider) {
>             assertIsHeld(m_stringsLock); // Needed as the compiler is not 
> able to tell we have the lock.
>             m_strings.append(provider.pullStrings());
>         });
>     }
> 
>     Vector<String> m_strings WTF_GUARDED_BY_LOCK(m_stringsLock);
>     Lock m_stringsLock;
>     Condition m_stringsCondition;
> };
> The most common case in WebKit is when you hold a lock in a function then 
> have a lambda in this function that runs synchronously. Even though the 
> lambda runs synchronously and you're holding the lock, the compiler doesn't 
> know that. As a result, if the lambda tries to access a protected data 
> member, you will get a compiler error unless you use assertIsHeld() at the 
> beginning of the lambda.
> 
> Limitations
> There are a few things Clang thread safety analysis is not able to do. You 
> can find examples of these here 
> <https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations>. I 
> am also documenting below a few things that had to change in WebKit as a 
> result.
> 
> No more holdLock() / tryHoldLock()
> 
> holdLock() / tryHoldLock() were not compatible with Clang Thread Safety 
> Analysis so we removed them. Instead, the following patterns are now 
> recommended:
> 
>     void foo()
>     {
>         // Previously:
>         // auto locker = holdLock(m_lock);
>         Locker locker { m_lock }; // With C++17 the type of the Locker<T> is 
> deduced based on type of m_lock.
>     }
> 
>     void bar()
>     {
>         /*
>         Previously:
>         auto locker = tryHoldLock(m_lock);
>         if (!locker)
>             return;
>         */
>         if (!m_lock.tryLock())
>             return;
>         Locker locker { AdoptLock, m_lock };
>     }
> }
> In my opinion, the tryHoldLock() alternative is not as nice but it enables 
> the safety analysis and we don't use tryHoldLock()very often in the code 
> base. In the future, I believe we should be able to make something like this 
> work:
> 
>     void bar()
>     {
>         Locker locker { DeferLock, m_lock };
>         if (!locker.tryLock())
>             return;
>     }
> Locker<Lock> is no longer movable
> 
> To support Clang thread safety analysis, Locker<Lock> no longer has a move 
> constructor.
> 
> Locker<Lock> can no longer wrap a null Lock
> 
> This was used for conditional locking like so:
> 
> Locker locker { shouldLock ? &m_lock : nullptr };
> What's next?
> Please adopt annotations above in new code to improve thread safety in our 
> code base. If you run into trouble adopting them or modifying code that 
> already uses the annotations, please feel free to ask me or Kimmo for help as 
> we've been porting a lot of the existing code. Hopefully this email is a good 
> reference too.
> 
> It was a fairly large project to adopt Clang thread safety analysis in WebKit 
> and some things still need work / clean up:
> 
> WTF::CheckedLock is currently an alias to WTF::Lock and will go away very 
> soon once no code is referencing it.
> WTF::UncheckedLock is the previous version of the lock that did not support 
> thread safety analysis and with its previous Locker. It is still used in 
> WebKit in some places for now because code using it was either not trivial to 
> port or we just did not have time to update it. Please avoid avoid using 
> WTF::UncheckedLock in new code if you can. For now, the plan is to work 
> towards moving all code from WTF::UncheckedLock to WTF::Lock but it may take 
> some time, discussion and possibly some Locker<Lock> changes. The main issue 
> really has been the Locker API differences documented above. JavaScriptCore 
> is by far the biggest user of WTF::UncheckedLock right now.
> Even though we adopted annotations in many places already, more code can 
> adopt it to improve thread safety.
> Some code uses WTF_IGNORES_THREAD_SAFETY_ANALYSIS with FIXME comments 
> indicating that it is likely not thread-safe. We need to review those and 
> either make them thread-safe or clarify why it is OK as is.
> We will likely try and make the Locker<Lock> more flexible to support more 
> use cases. We are a bit limited by what clang supports but I believe there is 
> room of improvement.
> Credits
> Credits to Kimmo for introducing the checked Lock to WebKit and helping with 
> the adoption!
> Big thanks to everyone who helped with reviews and feedback as well.
> 
> --
>  Chris Dumez
> 
> 
> 
> 

_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Reply via email to