Add means to handle negative capabilities in the Clang Thread Safety annotations

hebasto commented at 2:06 pm on June 11, 2020: member

This commit is separated from #19238, and it adds support of Negative Capabilities in the Clang Thread Safety Analysis attributes.

Negative requirements are an alternative EXCLUDES [LOCKS_EXCLUDED] that provide a stronger safety guarantee. A negative requirement uses the REQUIRES [EXCLUSIVE_LOCKS_REQUIRED] attribute, in conjunction with the ! operator, to indicate that a capability should not be held.

Examples of usage:

#19238 (for a class)
https://github.com/hebasto/bitcoin/tree/200610-addrman-tsn (for the whole code base)

Add means to handle negative capabilities in thread safety annotations f8213c05f0

vasild approved

vasild commented at 2:28 pm on June 11, 2020: member

ACK f8213c05

hebasto commented at 2:34 pm on June 11, 2020: member

@ajtowns @practicalswift @ryanofsky Mind reviewing this PR?

MarcoFalke commented at 2:40 pm on June 11, 2020: member

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

practicalswift commented at 3:07 pm on June 11, 2020: contributor

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?

hebasto commented at 3:22 pm on June 11, 2020: member

@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: https://github.com/llvm/llvm-project/commit/3efd0495a0813896e1559e532c5d9e581dd48c0e

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?

MarcoFalke commented at 3:30 pm on June 11, 2020: member

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

hebasto commented at 3:43 pm on June 11, 2020: member

@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.

hebasto commented at 3:57 pm on June 11, 2020: member

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

hebasto commented at 4:38 pm on June 11, 2020: member

@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)

MarcoFalke commented at 4:51 pm on June 11, 2020: member

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

MarcoFalke commented at 5:01 pm on June 11, 2020: member

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?

hebasto commented at 5:14 pm on June 11, 2020: member

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.

hebasto commented at 5:24 pm on June 11, 2020: member

@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? :)

vasild commented at 7:48 pm on June 11, 2020: member

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):

 0556     size_t size() const EXCLUSIVE_LOCKS_REQUIRED(!m_addrman_mutex)
 1557     {
 2558         LOCK(m_addrman_mutex); // TODO: Cache this in an atomic to avoid this overhead
 3559         return sizeNonLockerHelper();
 4560     }
 5561 
 6562     void f()
 7563     {
 8564         LOCK(m_addrman_mutex);
 9565         size();
10566     }

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

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

-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):

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

MarcoFalke commented at 10:33 pm on June 11, 2020: member

Does it warn even for recursive mutexes?

MarcoFalke commented at 11:26 pm on June 11, 2020: member

Approach ACK f8213c05f087e5fbb5d92a291f766b0baebc798f

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.

MarcoFalke added the label Refactoring on Jun 11, 2020

ajtowns commented at 3:45 am on June 12, 2020: member

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

 0#if defined(__clang__) && __clang_major__ >= 4
 1#define EXCLUSIVE_LOCKS_FORBIDDEN(a) __attribute__((exclusive_locks_required(!a)))
 2#else
 3#define EXCLUSIVE_LOCKS_FORBIDDEN(a)
 4#endif // clang 4
 5
 6class CAddrMan {
 7    ...
 8    size_t size() const EXCLUSIVE_LOCKS_FORBIDDEN(m_addrman_mutex);
 9    ...
10};

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:

0+void wtf(CConnman& c, const CService &addr, ServiceFlags nServices)
1+{
2+    LOCK(c.addrman.m_addrman_mutex);
3+    c.SetServices(addr, nServices);
4+}
5+
6 void CConnman::SetServices(const CService &addr, ServiceFlags nServices)
7 {
8     addrman.SetServices(addr, nServices);
9 }

(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?

hebasto commented at 7:33 am on June 12, 2020: member

@MarcoFalke

Does it warn even for recursive mutexes?

Yes, it does. See:

hebasto commented at 4:14 pm on June 12, 2020: member

@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

 0$ cat /etc/system-release
 1Fedora release 20 (Heisenbug)
 2
 3$ clang++ --version
 4clang version 3.3 (tags/RELEASE_33/rc3)
 5Target: x86_64-redhat-linux-gnu
 6Thread model: posix
 7
 8$ ./autogen.sh
 9$ ./configure --without-gui --disable-wallet CC=clang CXX=clang++
10$ make
11...
12In file included from crypto/sha256.cpp:11:
13./compat/cpuid.h:17:5: error: use of undeclared identifier '__cpuid_count'
14    __cpuid_count(leaf, subleaf, a, b, c, d);
15    ^
161 error generated.
17
18In file included from init.cpp:35:
19In file included from ./policy/policy.h:12:
20In file included from ./script/standard.h:12:
21In file included from /usr/include/boost/variant.hpp:17:
22/usr/include/boost/variant/variant.hpp:1751:9: error: call to member function 'convert_construct' is ambiguous
23        convert_construct( detail::variant::move(operand), 1L);
24        ^~~~~~~~~~~~~~~~~
251 error generated.

Clang 3.5

 0$ cat /etc/system-release
 1Fedora release 22 (Twenty Two)
 2
 3$ clang++ --version
 4clang version 3.5.0 (tags/RELEASE_350/final)
 5Target: x86_64-redhat-linux-gnu
 6Thread model: posix
 7
 8$ ./autogen.sh
 9$ ./configure --without-gui --disable-wallet CC=clang CXX=clang++
10$ make
11...
12In file included from banman.cpp:8:
13In file included from ./netaddress.h:12:
14In file included from ./compat.h:32:
15In file included from /usr/include/fcntl.h:302:
16/usr/include/bits/fcntl2.h:80:39: error: use of undeclared identifier '__builtin_va_arg_pack_len'
17      if ((__oflag & O_CREAT) != 0 && __va_arg_pack_len () < 1)
18                                      ^
19/usr/include/sys/cdefs.h:350:30: note: expanded from macro '__va_arg_pack_len'
20# define __va_arg_pack_len() __builtin_va_arg_pack_len ()
21                             ^
22fatal error: too many errors emitted, stopping now [-ferror-limit=]
2320 errors generated.

hebasto commented at 6:47 am on June 14, 2020: member

@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.

vasild commented at 4:02 pm on June 15, 2020: member

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

0#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.

MarcoFalke commented at 11:58 pm on June 15, 2020: member

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?

MarcoFalke merged this on Jun 17, 2020

MarcoFalke closed this on Jun 17, 2020

hebasto deleted the branch on Jun 17, 2020

sidhujag referenced this in commit 09ab074c00 on Jul 7, 2020

kittywhiskers referenced this in commit 6673aee63e on Jun 5, 2021

kittywhiskers referenced this in commit fd1cbae2ee on Jun 5, 2021

kittywhiskers referenced this in commit 9940930e5f on Jun 6, 2021

kittywhiskers referenced this in commit 5a5673a63c on Jun 6, 2021

kittywhiskers referenced this in commit 5a3e7797ba on Jun 6, 2021

UdjinM6 referenced this in commit 4b3ba40dcd on Jun 6, 2021

UdjinM6 referenced this in commit eee6b322a5 on Jun 6, 2021

kittywhiskers referenced this in commit a473cb67f9 on Jun 7, 2021

kittywhiskers referenced this in commit 5e7cf89605 on Jun 8, 2021

kittywhiskers referenced this in commit 95ae337fe0 on Jun 9, 2021

kittywhiskers referenced this in commit cbc6186b64 on Jun 10, 2021

UdjinM6 referenced this in commit a8aee57447 on Jun 11, 2021

deadalnix referenced this in commit e13ab3ae69 on Aug 26, 2021

DrahtBot locked this on Feb 15, 2022

Add means to handle negative capabilities in the Clang Thread Safety annotations #19249