@ajtowns @practicalswift @ryanofsky Mind reviewing this PR?

Concept ACK. Please adjust the minimum clang version to https://releases.llvm.org/3.6.0/tools/clang/docs/ThreadSafetyAnalysis.html#negative

This should be uncontroversial, because even debian oldoldstable comes with clang-4 https://packages.debian.org/jessie/clang-4.0

Concept ACK, but we'll have to opt-in to -Wthread-safety-negative (+ -Werror=thread-safety-negative in Travis) to get the benefit of those annotations? :)

Can you add a temporary test case to verify that Travis catches an example incorrect lock?

@MarcoFalke

Concept ACK. Please adjust the minimum clang version to https://releases.llvm.org/3.6.0/tools/clang/docs/ThreadSafetyAnalysis.html#negative

This should be uncontroversial, because even debian oldoldstable comes with clang-4 https://packages.debian.org/jessie/clang-4.0

This feature lives since the Clang version 3.5.0: llvm/llvm-project@3efd049

Please note that the Clang version checking is required not only for this PR changes but also for all subsequent adding of EXCLUSIVE_LOCKS_REQUIRED(!mutex) (see #19238).

Would it be a rational decision to bump the minimum required Clang version from the current 3.3 to 3.5?

Yes, as I said this should be uncontroversial, as all supported operating systems come with at least clang-4.0 (oldoldstable debian)

@practicalswift

Concept ACK, but we'll have to opt-in to -Wthread-safety-negative (+ -Werror=thread-safety-negative in Travis) to get the benefit of those annotations? :)

Not exactly :)

-Wthread-safety-negative warns about absent negative annotations. It will be quite noisy in the current state of the code base. But it is our goal.

Without -Wthread-safety-negative the already added negative annotations work as expected (see: #19238).

Can you add a temporary test case to verify that Travis catches an example incorrect lock?

Done in #19251.

@practicalswift
The first error catch: https://travis-ci.org/github/bitcoin/bitcoin/jobs/697279768#L3640

@MarcoFalke

Yes, as I said this should be uncontroversial, as all supported operating systems come with at least clang-4.0 (oldoldstable debian)

https://packages.debian.org/jessie/clang:

Package: clang (1:3.5-25)

https://packages.debian.org/jessie/clang-4.0

Also, I agree with @practicalswift that this should be added to the default flags. Otherwise it misses the whole point of helping developers write better code. There should not be a single negative annotation in our code, so there can't be any warnings. What am I missing?

Also, I agree with @practicalswift that this should be added to the default flags. Otherwise it misses the whole point of helping developers write better code. There should not be a single negative annotation in our code, so there can't be any warnings. What am I missing?

All classes and free functions that use mutexes are required to be modernized as it done with the CAddrMan class in #19238. Only after that the -Wthread-safety-negative flag will be usable and useful without spammy warnings.

At the end of this way there is a hope to get rid of RecursiveMutexs completely.

@MarcoFalke @practicalswift
On the first stage, I see Negative Capabilities as a tool to refactor RecursiveMutexs into Mutexs with deadlock-free guaranties, and to verify the refactoring is done correctly.

The next stage is as @practicalswift said:

Concept ACK, but we'll have to opt-in to -Wthread-safety-negative (+ -Werror=thread-safety-negative in Travis) to get the benefit of those annotations? :)

Just to clarify with an example why EXCLUSIVE_LOCKS_REQUIRED(!m) makes sense without -Wthread-safety-negative (I added a buggy function f() which calls size() while holding the mutex):

556     size_t size() const EXCLUSIVE_LOCKS_REQUIRED(!m_addrman_mutex)
557     {
558         LOCK(m_addrman_mutex); // TODO: Cache this in an atomic to avoid this overhead
559         return sizeNonLockerHelper();
560     }
561 
562     void f()
563     {
564         LOCK(m_addrman_mutex);
565         size();
566     }

When this code is compiled with just -Wthread-safety (as in current master) then it produces:

src/addrman.h:565:9: error: cannot call function 'size' while mutex 'm_addrman_mutex' is held [-Werror,-Wthread-safety-analysis]
        size();
        ^

-Wthread-safety enables -Wthread-safety-analysis but not -Wthread-safety-negative.

So what does -Wthread-safety-negative do then? It warns whenever some function LOCK()s something without having EXCLUSIVE_LOCKS_REQUIRED(!thatthing):

src/addrman.h:564:9: error: acquiring mutex 'm_addrman_mutex' requires negative capability '!m_addrman_mutex' [-Werror,-Wthread-safety-negative]
        LOCK(m_addrman_mutex);
        ^

Does it warn even for recursive mutexes?

Approach ACK f8213c0

This simply adds a member function and doesn't need to bump the minimum compiler version. I don't see a reason to disallow this annotation for temporary testing by not having the member function.

If we're even slightly worried about supporting old compilers, why not add a new macro to threadsafety.h?

#if defined(__clang__) && __clang_major__ >= 4
#define EXCLUSIVE_LOCKS_FORBIDDEN(a) __attribute__((exclusive_locks_required(!a)))
#else
#define EXCLUSIVE_LOCKS_FORBIDDEN(a)
#endif // clang 4

class CAddrMan {
    ...
    size_t size() const EXCLUSIVE_LOCKS_FORBIDDEN(m_addrman_mutex);
    ...
};

It seems if you add member functions in CAddrMan that call things that forbid holding m_addrman_mutex, then you have to also mark those member functions as forbidding holding m_addrman_mutex, but for functions outside of those modules, you don't have to. If I do:

+void wtf(CConnman& c, const CService &addr, ServiceFlags nServices)
+{
+    LOCK(c.addrman.m_addrman_mutex);
+    c.SetServices(addr, nServices);
+}
+
 void CConnman::SetServices(const CService &addr, ServiceFlags nServices)
 {
     addrman.SetServices(addr, nServices);
 }

(and make addrman and m_addrman_mutex public members) then I don't get a compile time warning.

Based on the google paper I think the what's happening is "The analyzer assumes that it holds a negative capability for any object that is not defined within the current lexical scope" -- so for mutexes that are private class members, it should be fine, I think; and also for module-specific globals. But I think it will not work right for cross-file globals like cs_main?

@MarcoFalke

Does it warn even for recursive mutexes?

Yes, it does. See:

• https://travis-ci.org/github/bitcoin/bitcoin/jobs/697279768#L3640
• https://travis-ci.org/github/bitcoin/bitcoin/jobs/697279769#L2114

@MarcoFalke

This simply adds a member function and doesn't need to bump the minimum compiler version.

Agree to postpone the minimum compiler version bumping until C++17.

In any case, Clang just ignores unknown annotations.

FWIW, I've made some retro tests:

• Clang 3.3

$ cat /etc/system-release
Fedora release 20 (Heisenbug)

$ clang++ --version
clang version 3.3 (tags/RELEASE_33/rc3)
Target: x86_64-redhat-linux-gnu
Thread model: posix

$ ./autogen.sh
$ ./configure --without-gui --disable-wallet CC=clang CXX=clang++
$ make
...
In file included from crypto/sha256.cpp:11:
./compat/cpuid.h:17:5: error: use of undeclared identifier '__cpuid_count'
    __cpuid_count(leaf, subleaf, a, b, c, d);
    ^
1 error generated.

In file included from init.cpp:35:
In file included from ./policy/policy.h:12:
In file included from ./script/standard.h:12:
In file included from /usr/include/boost/variant.hpp:17:
/usr/include/boost/variant/variant.hpp:1751:9: error: call to member function 'convert_construct' is ambiguous
        convert_construct( detail::variant::move(operand), 1L);
        ^~~~~~~~~~~~~~~~~
1 error generated.

• Clang 3.5

$ cat /etc/system-release
Fedora release 22 (Twenty Two)

$ clang++ --version
clang version 3.5.0 (tags/RELEASE_350/final)
Target: x86_64-redhat-linux-gnu
Thread model: posix

$ ./autogen.sh
$ ./configure --without-gui --disable-wallet CC=clang CXX=clang++
$ make
...
In file included from banman.cpp:8:
In file included from ./netaddress.h:12:
In file included from ./compat.h:32:
In file included from /usr/include/fcntl.h:302:
/usr/include/bits/fcntl2.h:80:39: error: use of undeclared identifier '__builtin_va_arg_pack_len'
      if ((__oflag & O_CREAT) != 0 && __va_arg_pack_len () < 1)
                                      ^
/usr/include/sys/cdefs.h:350:30: note: expanded from macro '__va_arg_pack_len'
# define __va_arg_pack_len() __builtin_va_arg_pack_len ()
                             ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.

@ajtowns

Based on the google paper I think the what's happening is "The analyzer assumes that it holds a negative capability for any object that is not defined within the current lexical scope" -- so for mutexes that are private class members, it should be fine, I think; and also for module-specific globals. But I think it will not work right for cross-file globals like cs_main?

Agree with you.

Currently all thread-safety macros map to attributes with identical names:

#define FOO() __attribute__((foo))

That is good because somebody who is familiar with the clang attributes, but not with bitcoin core specifics does not have to look up what a macro does in src/threadsafety.h. So, maybe refrain from adding EXCLUSIVE_LOCKS_FORBIDDEN(). Also, the current EXCLUSIVE_LOCKS_REQUIRED() allows to list more than one mutex: EXCLUSIVE_LOCKS_REQUIRED(m1, !m2, m3), whereas the mentioned EXCLUSIVE_LOCKS_FORBIDDEN() would break in a strange way if passed something like m1, !m2.

This looks like an uncontroversial addition of a member function with no risk of breaking anything, so I am planning to merge this in the coming days. To ask more c++ experienced people, @ryanofsky do you think this could break anything?