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
