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