[webkit-dev] Clang Thread Safety Analysis

[Chris Dumez]

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




-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20210523/edb6bc3d/attachment.htm>