`-Wthread-safety-negative` reported with seemingly correctly annotated code

[firewave]

#include <mutex>
#include <set>
#include <string>

class C
{
    void f(std::string s)
    {
        std::lock_guard<std::mutex> l(mSync);
        mObject.emplace(std::move(s));
    }

private:
    std::mutex mSync;
    std::set<std::string> mObject __attribute__((guarded_by(mSync)));
};

Building with -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS -stdlib=libc++ -Weverything gives the following:

<source>:9:37: warning: acquiring mutex 'mSync' requires negative capability '!mSync' [-Wthread-safety-negative]
    9 |         std::lock_guard<std::mutex> l(mSync);
      |                                     ^
<source>:8:5: note: thread warning in function 'f'
    8 |     {
      |     ^

https://godbolt.org/z/d7q7s9jK7

There is some discussion about this in the patch which introduced this: https://reviews.llvm.org/D84604.

CC @aaronpuchert @AaronBallman @LebedevRI

[aaronpuchert]

This looks like it's working as intended: if you enable -Wthread-safety-negative, you get a warning if you acquire a mutex and don't have the corresponding negative capability. In this case, adding __attribute__((requires_capability(!mSync))) on f makes the warning go away. (You need to declare std::mutex operator!(const std::mutex&); to make this work, but a definition is not required, since you're only using it in an unevaluated context.)

There is some discussion about this in the patch which introduced this: https://reviews.llvm.org/D84604.

This change should be unrelated: it was about global variables, but here the mutex is a non-static member. Those were always supported.

[firewave]

This looks like it's working as intended: if you enable -Wthread-safety-negative,

That might be the crux of this. I am adding the annotations and already use -Weverything so these warnings just show up. The documentation indicates this might be an opt-in feature and that is not how it works with warnings (unlike clang-tidy checks).

Negative requirements are an experimental feature which is off by default, because it will produce many warnings in existing code. It can be enabled by passing -Wthread-safety-negative.

you get a warning if you acquire a mutex and don't have the corresponding negative capability. In this case, adding __attribute__((requires_capability(!mSync))) on f makes the warning go away. (You need to declare std::mutex operator!(const std::mutex&); to make this work, but a definition is not required, since you're only using it in an unevaluated context.)

After reading the documentation I understand what this is all about. But I wonder if this should only be required if you are not using something like std::lock_guard i.e. explicitly locking and unlocking the mutex. It feels like it should imply the negative capability for the mutex being used.

I could see it making sense when multiple locks are being involved to statically detect deadlocks.

So I will disable the warning as it doesn't seem to be of any help in this case (beside it being experimental).

[aaronpuchert]

This looks like it's working as intended: if you enable -Wthread-safety-negative,

That might be the crux of this. I am adding the annotations and already use -Weverything so these warnings just show up. The documentation indicates this might be an opt-in feature and that is not how it works with warnings (unlike clang-tidy checks).

With -Weverything you opt into every warning, including experimental warnings.

But in this case there isn't even a bug in my opinion. It's working as it should.

But I wonder if this should only be required if you are not using something like std::lock_guard i.e. explicitly locking and unlocking the mutex.

How you acquire the lock inside of the function is irrelevant for -Wthread-safety-negative. The warning is not that you might forget to release it, the warning wants proof that the mutex was not acquired already.

It feels like it should imply the negative capability for the mutex being used.

My understanding of the lock_guard constructor being used here is that it simply locks the mutex. It does not magically imply that the mutex is previously unlocked. On the contrary, that is a precondition, and that's what the warning is about.

I could see it making sense when multiple locks are being involved to statically detect deadlocks.

That's pretty close to the point, except that you can also get deadlocks with a single mutex, if it's not reentrant/recursive. With -Wthread-safety-negative you need to annotate every function that acquires the lock as requiring the corresponding negative capability. This requires callers to prove that they don't have the lock, at least where the lock is in scope. In this case, a method of the class needs to have the negative capability, any function outside of the class does not. (Access specifiers don't matter at the moment, since I didn't continue working on https://reviews.llvm.org/D87194.)