Do not hide compile-time thread safety warnings

hebasto commented at 12:43 pm on August 5, 2020: member

On the way of transit from RecursiveMutex to Mutex (see #19303) it is crucial to have run-time AssertLockHeld() assertion that does not hide compile-time Clang Thread Safety Analysis warnings.

On master (65e4ecabd5b4252154640c7bac38c92a3f3a7018) using AssertLockHeld() could hide Clang Thread Safety Analysis warnings, e.g., with the following patch applied:

 0--- a/src/txmempool.h
 1+++ b/src/txmempool.h
 2@@ -607,7 +607,7 @@ public:
 3     void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
 4 
 5     void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs);
 6-    void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
 7+    void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 8     void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs);
 9     void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs);
10

Clang compiles the code without any thread safety warnings.

See “Add missed thread safety annotations” commit for the actual thread safety warnings that are fixed in this PR.

hebasto commented at 12:44 pm on August 5, 2020: member

Friendly ping @ajtowns @vasild

laanwj added the label Build system on Aug 5, 2020

in src/net.h:224 in 5d64c6b18f outdated

221+                func(node);
222+        }
223+    };
224+
225+    template <typename Callable>
226+    void ForEachNode(Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)

MarcoFalke commented at 1:47 pm on August 5, 2020:

It seems a layer-violation to import the validation lock cs_main into the net.h header to annotate this function. Also, this approach doesn’t seem like it scales, since it has to be done for each mutex that is held during a call to ForEachNode.

hebasto commented at 2:43 pm on August 5, 2020:

Updated.

MarcoFalke commented at 2:06 pm on August 5, 2020: member

weak Concept ACK. Will review some more later

hebasto force-pushed on Aug 5, 2020

hebasto commented at 2:38 pm on August 5, 2020: member

Updated fa9157916514903cd7b526375597b1de0d8df8f0 -> 3f00515e92e7b6aed97c126592ad94b8a52a0e18 (pr19668.01 -> pr19668.02, diff).

This is an alternative approach without issues mentioned by @MarcoFalke:

It seems a layer-violation to import the validation lock cs_main into the net.h header to annotate this function. Also, this approach doesn’t seem like it scales, since it has to be done for each mutex that is held during a call to ForEachNode.

hebasto commented at 2:42 pm on August 5, 2020: member

@MarcoFalke

weak Concept ACK. Will review some more later

Updated motivation in the OP.

ajtowns commented at 5:01 pm on August 5, 2020: member

Concept ACK.

I’d suggest going further and annotating the AssertLock(Not)Held calls with EXCLUSIVE_LOCKS_REQUIRED(cs) (or !cs) too. This catches existing missing annotations in validation.cpp:CheckSequenceLocks and wallet/scriptpubkeyman.cpp:DescriptorScriptPubKeyMan::AddDescriptKeyWithDb so seems worthwhile. Patch at https://github.com/ajtowns/bitcoin/commits/202008-assertlockheld

hebasto commented at 5:30 pm on August 5, 2020: member

@ajtowns

I’d suggest going further and annotating the AssertLock(Not)Held calls with EXCLUSIVE_LOCKS_REQUIRED(cs) (or !cs) too. This catches existing missing annotations in validation.cpp:CheckSequenceLocks and wallet/scriptpubkeyman.cpp:DescriptorScriptPubKeyMan::AddDescriptKeyWithDb so seems worthwhile. Patch at https://github.com/ajtowns/bitcoin/commits/202008-assertlockheld

Thanks! Done.

ajtowns commented at 6:01 am on August 6, 2020: member

Might be good to fix up the lock annotations before removing the ASSERT annotation on AssertLockHeldInternal so you can compile successfully at each commit? Otherwise looks good to me.

hebasto force-pushed on Aug 6, 2020

hebasto commented at 6:07 am on August 6, 2020: member

@ajtowns

Might be good to fix up the lock annotations before removing the ASSERT annotation on AssertLockHeldInternal so you can compile successfully at each commit? Otherwise looks good to me.

Done, commits are reordered now.

jnewbery commented at 10:56 am on August 7, 2020: member

Concept ACK. Perhaps the Locking/mutex usage notes section of developer-notes.md should be updated with the current best practice for annotating/asserting locks.

hebasto commented at 11:18 am on August 7, 2020: member

@jnewbery

Concept ACK. Perhaps the Locking/mutex usage notes section of developer-notes.md should be updated with the current best practice for annotating/asserting locks.

You read my mind :)

Do you want me adding a doc commit to this PR?

jnewbery commented at 12:09 pm on August 7, 2020: member

Do you want me adding a doc commit to this PR?

I think it’s fine to add it to this PR.

in src/sync.h:58 in fcf6569d2e outdated

52@@ -53,8 +53,9 @@ void LeaveCritical();
53 void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line);
54 std::string LocksHeld();
55 template <typename MutexType>
56-void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs);
57-void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs);
58+void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs);
59+template <typename MutexType>
60+void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs);

vasild commented at 1:38 pm on August 7, 2020:

These can also be tagged with ASSERT_EXCLUSIVE_LOCK.

0void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs) EXCLUSIVE_LOCKS_REQUIRED(cs);
1template <typename MutexType>
2void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs) EXCLUSIVE_LOCKS_REQUIRED(!cs);

vasild commented at 8:04 pm on August 7, 2020:

I tried the following with AssertLockHeldInternal() EXCLUSIVE_LOCKS_REQUIRED(cs) ASSERT_EXCLUSIVE_LOCK(cs);:

0void f()
1{
2    // The following line produces a warning due to the EXCLUSIVE_LOCKS_REQUIRED attribute:
3    // Calling function 'AssertLockHeldInternal<AnnotatedMixin<std:: __1::recursive_mutex>>' requires holding mutex 'cs_main' exclusively [-Wthread-safety-analysis]
4    AssertLockHeld(cs_main);
5
6    // The following line produces a warning due to the ASSERT_EXCLUSIVE_LOCK attribute:
7    // Acquiring mutex 'cs_main' that is already held
8    LOCK(cs_main);
9}

However if the assert attribute is defined first: ASSERT_EXCLUSIVE_LOCK(cs) EXCLUSIVE_LOCKS_REQUIRED(cs) then only the second warning is generated (EXCLUSIVE_LOCKS_REQUIRED is ignored in this case).

hebasto commented at 8:47 pm on August 7, 2020:

These can also be tagged with ASSERT_EXCLUSIVE_LOCK.

What are expected benefits?

hebasto commented at 8:38 am on August 8, 2020:

@vasild Here are branches to test your suggestion:

Compiling the branch with suggestion miss the following warnings:

0wallet/scriptpubkeyman.cpp:1861:9: warning: reading variable 'm_map_crypted_keys' requires holding mutex 'cs_desc_man' [-Wthread-safety-analysis]
1        m_map_crypted_keys[pubkey.GetID()] = make_pair(pubkey, crypted_secret);
2        ^
3wallet/scriptpubkeyman.cpp:1864:9: warning: reading variable 'm_map_keys' requires holding mutex 'cs_desc_man' [-Wthread-safety-analysis]
4        m_map_keys[pubkey.GetID()] = key;

and

0wallet/wallet.cpp:4429:54: warning: passing variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Wthread-safety-reference]
1                if (!spk_manager->CheckDecryptionKey(vMasterKey) && !spk_manager->Encrypt(vMasterKey, nullptr)) {
2                                                     ^
3wallet/wallet.cpp:4429:91: warning: passing variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Wthread-safety-reference]
4                if (!spk_manager->CheckDecryptionKey(vMasterKey) && !spk_manager->Encrypt(vMasterKey, nullptr)) {

vasild commented at 11:50 am on August 10, 2020:

Ok, I confirm the above. The presence of ASSERT_EXCLUSIVE_LOCK() causes some warnings to be silenced.

aaronpuchert commented at 11:53 pm on September 16, 2020:

However if the assert attribute is defined first: ASSERT_EXCLUSIVE_LOCK(cs) EXCLUSIVE_LOCKS_REQUIRED(cs) then only the second warning is generated (EXCLUSIVE_LOCKS_REQUIRED is ignored in this case).

This is somewhat expected. If you assert cs, it’s assumed to be held afterwards. If you require it to be held and then assert it, we warn if it isn’t initially held, and then assume it to be held afterwards.

Generally though you shouldn’t expect attributes to be treated in the order they are given, and this particular combination of attributes just doesn’t make sense. (Either you’re assuming a capability that you’ve just asserted, which is redundant, or you assert a capability that you’ve previously assumed, which is pointless.)

vasild commented at 9:41 am on September 18, 2020:

@aaronpuchert,

This is somewhat expected. If you assert cs, it’s assumed to be held afterwards. If you require it to be held and then assert it, we warn if it isn’t initially held, and then assume it to be held afterwards.

I agree with that if they were two distinct functions:

0func_tagged_with_assert_exclusive_lock();
1...
2func_tagged_with_exclusive_lock_required();
3...

But for a single function: void f() ATTR1 ATTR2 I wouldn’t expect the behavior to differ if defined like void f() ATTR2 ATTR1 instead.

If the combination of ATTR1 and ATTR2 is meaningless then maybe it would be good to produce some warning or error instead of silently assuming some behavior. Like for signed unsigned int x;.

aaronpuchert commented at 11:31 am on September 18, 2020:

If the combination of ATTR1 and ATTR2 is meaningless then maybe it would be good to produce some warning or error instead of silently assuming some behavior.

Makes sense, but there are many attributes and I’m not sure whether we can exhaustively figure out which combinations make sense and which don’t.

hebasto commented at 10:05 am on August 8, 2020: member

Do you want me adding a doc commit to this PR?

I think it’s fine to add it to this PR.

Done. @jnewbery @jonatack Mind checking wording in the doc commit?

in doc/developer-notes.md:761 in 7666823661 outdated

756+// txmempool.h
757+class CTxMemPool
758+{
759+public:
760+    ...
761+    mutable RecursiveMutex cs;

vasild commented at 11:56 am on August 10, 2020:

nit: Maybe use an example that does not involve RecursiveMutex since we recommend against using it and if we manage to obliterate it from the code, then this example will require adjusting.

hebasto commented at 4:23 pm on August 29, 2020:

Oh, I’ve failed to find an example in our code base that embraces a Mutex instance, an annotation in function member declaration, and an assertion in function member definition.

vasild approved

vasild commented at 11:57 am on August 10, 2020: member

ACK 766682366

vasild commented at 12:01 pm on August 10, 2020: member

This PR basically replaces the attribute ASSERT_EXCLUSIVE_LOCK() of AssertLockHeldInternal() with EXCLUSIVE_LOCKS_REQUIRED() and fixes some of the exposed warnings.

It is unfortunate that we can’t have both attributes.

jnewbery commented at 11:35 am on August 13, 2020: member

This all seems reasonable and the updated developer docs are clear and make sense. @ajtowns : what does your :-1: mean here #19668 (comment)? Are you saying that we can have both attributes, or that it’s not unfortunate, or that we can’t have both and that this PR is going in the wrong direction? As someone who doesn’t know very much about the safety annotations, it’s difficult for me to interpret what your objection is.

DrahtBot commented at 8:58 pm on August 20, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17785 (p2p: Unify Send and Receive protocol versions by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Needs rebase on Aug 24, 2020

hebasto force-pushed on Aug 24, 2020

hebasto commented at 5:32 pm on August 24, 2020: member

Rebased 76668236610198f61bf60c42a40c2826585515b7 -> 9ee46539e2c65e4ac53149fcdf1badeb35279013 (pr19668.05 -> pr19668.06) due to the conflict with #19704.

DrahtBot removed the label Needs rebase on Aug 24, 2020

in src/net_processing.cpp:631 in 00e537d140 outdated

627@@ -628,13 +628,12 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman& connma
628             }
629         }
630         connman.ForNode(nodeid, [&connman](CNode* pfrom){
631-            AssertLockHeld(cs_main);
632+            LockAssertion lock(::cs_main);

MarcoFalke commented at 9:38 am on August 29, 2020:

LockAssertion’s __attribute__ is EXCLUSIVE_LOCK_FUNCTION aka ACQUIRE. This goes against the clang documentation, which states that the capability is not held. https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#acquire-acquire-shared-release-release-shared

ACQUIRE is an attribute on functions or methods, which declares that the function acquires a capability, but does not release it. The caller must not hold the given capability on entry, and it will hold the capability on exit. ACQUIRE_SHARED is similar.

hebasto commented at 4:00 pm on August 29, 2020:

https://github.com/bitcoin/bitcoin/blob/4631dc5c578475fd3ca7a91676f7daf788a11192/src/sync.h#L353-L355

when it [the fact that mutex is locked] couldn’t be determined otherwise [by Thread Safety Analysis]

Such lambda expression is that case.

MarcoFalke commented at 4:44 pm on August 29, 2020:

But in that case the AssertLockHeld (version as in current master) should be used. That would also comply with the clang docs: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#assert-capability-and-assert-shared-capability

These are attributes on a function or method that does a run-time test to see whether the calling thread holds the given capability. The function is assumed to fail (no return) if the capability is not held.

I think that the AssertLockHeld is currently overused, as we use it as an run-time backup of the compile-time check. It might be good to add an AssertLockHeld version that only does the runtime check without influencing compile-time warnings.

MarcoFalke commented at 4:45 pm on August 29, 2020:

The alternative version could even be tagged with EXCLUSIVE_LOCKS_REQUIRED, so that correct usage of the run-time-only check would be enforced at compile time.

hebasto commented at 4:46 pm on August 29, 2020:

This PR goal is decouple compile-time check from run-time one.

hebasto commented at 4:55 pm on August 29, 2020:

From the point of Thread Safety Analysis the AssertLockHeld() is a run-time check now. That hides compile-time checks.

This PR makes AssertLockHeld() a compile-time check (again from the point of Thread Safety Analysis).

It might be good to add an AssertLockHeld version that only does the runtime check without influencing compile-time warnings.

This PR makes (not adds) “an AssertLockHeld version that only does the runtime check without influencing compile-time warnings.”

MarcoFalke commented at 5:12 pm on August 29, 2020:

Ok, but why are you not adding the annotation that the documentation suggests for this use case?

 0diff --git a/src/sync.h b/src/sync.h
 1index 7b397a8003..4a222442a9 100644
 2--- a/src/sync.h
 3+++ b/src/sync.h
 4@@ -357,7 +357,7 @@ public:
 5 struct SCOPED_LOCKABLE LockAssertion
 6 {
 7     template <typename Mutex>
 8-    explicit LockAssertion(Mutex& mutex) EXCLUSIVE_LOCK_FUNCTION(mutex)
 9+    explicit LockAssertion(Mutex& mutex) ASSERT_EXCLUSIVE_LOCK(mutex)
10     {
11 #ifdef DEBUG_LOCKORDER
12         AssertLockHeld(mutex);

https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#assert-capability-and-assert-shared-capability

These are attributes on a function or method that does a run-time test to see whether the calling thread holds the given capability. The function is assumed to fail (no return) if the capability is not held.

hebasto commented at 5:32 pm on August 29, 2020:

Ok, but why are you not adding the annotation that the documentation suggests for this use case?

It seems a bit out of this PR scope. Nevertheless, ASSERT_EXCLUSIVE_LOCK “does a run-time test”. But LockAssertion provides compile-time TSA check. See: https://github.com/bitcoin/bitcoin/pull/14437/files#diff-df95fc819b237793cb462142a31b1871

MarcoFalke commented at 5:55 pm on August 29, 2020:

The run time check was added when it was renamed from “annotation” to “assertion”: #16034 (comment)

hebasto commented at 6:06 pm on August 29, 2020:

Our crafted run time check AssertLockHeld() is preferred over TSA run time check ASSERT_EXCLUSIVE_LOCK(), no?

ajtowns commented at 4:10 pm on August 31, 2020:

I think the only actual differences between EXCLUSIVE_LOCK_FUNCTION and ASSERT_EXCLUSIVE_LOCK are that the latter doesn’t warn you when it sees you attempt to lock the same thing twice (so whether a redundant assertion like LOCK(x); LockAssertion a(x); gives an error or not) and whether it warns you if you don’t release the lock when you leave the scope – but since we’ve got an unlocking destructor here I don’t think that matters much. Ref

So I think it’s fine to mark this function with either EXCLUSIVE_LOCK_FUNCTION or ASSERT_EXCLUSIVE_LOCK. E_L_F has the advantage that it discourages using LockAssertion where it’s not necessary; otoh A_E_L has the advantage that it won’t cause problems if someday clang becomes smarter about when locks are held, and we need the assertion for old clang versions but not new ones. Given there’s not many docs about how the ASSERT variant works and the example code doesn’t actually exercise it, probably safer to stick with EXCLUSIVE_LOCK_FUNCTION?

MarcoFalke commented at 4:13 pm on August 31, 2020:

Sure. Lets leave it as is. Resolving discussion.

aaronpuchert commented at 0:21 am on September 17, 2020:

ASSERT_EXCLUSIVE_LOCK was created for exactly this purpose, and it’s conceptually the right attribute to use here. EXCLUSIVE_LOCK_FUNCTION isn’t right for the very simple reason that you don’t actually lock. Also this shouldn’t be a SCOPED_LOCKABLE because assertions don’t go out of scope.

E_L_F has the advantage that it discourages using LockAssertion where it’s not necessary

It’s redundant but obviously safe, so it’s deliberate that we don’t warn about that.

Given there’s not many docs about how the ASSERT variant works

Feel free to suggest changes. We obviously want to keep the docs short so that you don’t have to read a novel, but they could be a bit more verbose.

and the example code doesn’t actually exercise it,

You can have a look at the test.

MarcoFalke commented at 9:39 am on August 29, 2020: member

It seems to be doing the exact opposite of what the clang docs say, so it would be good to explain why this needs to be different

in src/sync.h:56 in 5971934eac outdated

52@@ -53,7 +53,7 @@ void LeaveCritical();
53 void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line);
54 std::string LocksHeld();
55 template <typename MutexType>
56-void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) ASSERT_EXCLUSIVE_LOCK(cs);
57+void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs);

MarcoFalke commented at 4:56 pm on August 29, 2020:

in commit 5971934eac13809143c763aa515b1960a7368d74:

why are you removing this? This should come with the appropriate annotation to enforce correct usage and warn about missing compile-time annotations

0void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs);

MarcoFalke commented at 4:56 pm on August 29, 2020:

In fact, adding this annotation and compiling the commit shows one instance of a missing annotation.

MarcoFalke commented at 5:01 pm on August 29, 2020:

Ah, it looks like you already did that in the next commit :man_facepalming: 653c8d065d90642ffb2d8d4d49474b4abc67a811

hebasto commented at 5:03 pm on August 29, 2020:

Should I reorder/squash commits?

MarcoFalke commented at 5:31 pm on August 29, 2020:

No need to reorder, but if you want you can describe the two preparatory commits. Suggested git range-diff for the first two commits:

 01:  00e537d140 ! 1:  efeaeaec78 Use LockAssertion utility class instead of AssertLockHeld()
 1    @@ Metadata
 2      ## Commit message ##
 3         Use LockAssertion utility class instead of AssertLockHeld()
 4     
 5    +    First, add the proper attribute to LockAssertion, according to
 6    +    https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#assert-capability-and-assert-shared-capability
 7    +
 8    +    Then, to prepare for commit "Do not hide compile-time thread safety
 9    +    warnings", replace AssertLockHeld with LockAssertion where needed.
10    +
11      ## src/net_processing.cpp ##
12     @@ src/net_processing.cpp: static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman& connma
13                  }
14    @@ src/net_processing.cpp: void PeerLogicValidation::EvictExtraOutboundPeers(int64_
15      
16                      // Only disconnect a peer that has been connected to us for
17                      // some reasonable fraction of our check-frequency, to give
18    +
19    + ## src/sync.h ##
20    +@@ src/sync.h: public:
21    + struct SCOPED_LOCKABLE LockAssertion
22    + {
23    +     template <typename Mutex>
24    +-    explicit LockAssertion(Mutex& mutex) EXCLUSIVE_LOCK_FUNCTION(mutex)
25    ++    explicit LockAssertion(Mutex& mutex) ASSERT_EXCLUSIVE_LOCK(mutex)
26    +     {
27    + #ifdef DEBUG_LOCKORDER
28    +         AssertLockHeld(mutex);
292:  0ed94d8f86 ! 2:  f0e821b7ef Add missed thread safety annotations
30    @@ Metadata
31      ## Commit message ##
32         Add missed thread safety annotations
33     
34    +    This is needed for commit "sync.h: make runtime lock checks require
35    +    compile-time lock checks" to pass
36    +
37      ## src/wallet/scriptpubkeyman.h ##
38     @@ src/wallet/scriptpubkeyman.h: private:
39          //! keeps track of whether Unlock has run a thorough check before

hebasto commented at 5:47 pm on August 29, 2020:

Thanks! Commit messages have been updated.

MarcoFalke commented at 5:32 pm on August 29, 2020: member

Just one question: #19668 (review)

Otherwise, almost ACK 984ea63cb6 🔘

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3almost ACK 984ea63cb6 🔘
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgRiQv+OzT2oaGMTBdl/bL/KCX2vXdlLOYUopemBZbi3N+ZZxDTGcYwPBWDsjoF
 81BAKpoRQfy18UvI+TwftS4hccqt2676r332uahJ3TD0SwT5LGe4IyR6w2QAnB86o
 9Wv7eLRJIMoAb1b3UfL2wTNCEWXouxqoAVeEsucq4PyfI5lDQKpilJSSvJjRmvScK
10+wuNfvhmXeMX+RzYBTBilG0SELfjhnsNSn9l+rkmMgbD7kAzHatUFJ+FeSRHDJPb
11Lwr0XBI6hsYYCn/d7vH9o2PVXcULJ4PFpjg5HU4/f40dF+7vmva38CA9cKlJyVEU
122McPNT8h99/JAonUFMTFAzroAjAKyD1SVDRl2vyKAArT84HGMnKH0cr4e5WKaHuw
13oa4DXt0fyz9PxZayq9hd3SPOJB4ukxrY8rWI2QFZHgOU5AUAKR0L8B1fJA+KKFgU
14zVGxPz38xUO2UahyHqzN6Nm/ELyXgBq9lZG0xM5kFzL1Xha/dsooqvu0M8faSzSM
15+0SGxDw8
16=jU5f
17-----END PGP SIGNATURE-----

Timestamp of file with hash 7b19e59265341027936672d6d3e2e75fba6bee9ec10e1cd33960b42ec4416ed8 -

Use LockAssertion utility class instead of AssertLockHeld()

This change prepares for upcoming commit "Do not hide compile-time
thread safety warnings" by replacing AssertLockHeld() with
LockAssertion() where needed.

af9ea55a72

Add missed thread safety annotations

This is needed for upcoming commit "sync.h: Make runtime lock checks
require compile-time lock checks" to pass.

3ddc150857

Do not hide compile-time thread safety warnings 23d71d171e

sync.h: Make runtime lock checks require compile-time lock checks 2ee7743fe7

doc: Add best practice for annotating/asserting locks ea74e10acf

hebasto force-pushed on Aug 29, 2020

hebasto commented at 5:51 pm on August 29, 2020: member

Updated 9ee46539e2c65e4ac53149fcdf1badeb35279013 -> ea74e10acf17903e44c85e3678853414653dd4e1 (pr19668.06 -> pr19668.07, diff):

addressed @MarcoFalke’s comment:

No need to reorder, but if you want you can describe the two preparatory commits.

MarcoFalke commented at 5:58 pm on August 29, 2020: member

ACK ea74e10acf 🎙

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK ea74e10acf 🎙
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjduAwAvJQy8WxET79bR7T5ZL/UoKzl6VDAiNEM5pbrfO0xCRrsD1bM+s+RABtR
 8nxkeaGOrOrUNAvbGun0GKtT7voqp3x3RwXtcUdeNRCSJXp5zOIzMP8V87FCzMZ9v
 9WI9jJh2E6JngRXTzJayY7R4enioFcCoLK415AwlEZaa06jycBP3TpAPOsjUczaIC
108EjDku/22miUzPcwnHAw1XR9bhXZ3oyl6b7Lwx4mktiGFCNtvr54MLopFls//+bs
11ZxFQ3Wqma5wLm8aBPCfHkP1ZkTtLzEZnH9ho2fphW986fDjvd+G0+H00/zV1P8ey
12QeY+fTU0zzxoUMXUQsoGvXAxcfRnV1/EtzWtqraTYs+ebQx1kN8PPilkBcIioyI/
13llP9i7SP/QJEH74bxQPD6IaC8NlyY9is2RI99DzBiGYawB0zZj65ri9hWIocD+XD
14LYefVtHoAjaEflzHGnYWNiEGBzSOEm/RUUONBT1JBFmkvFQC6LNWoYGbZe+MfuYT
15/at0I08D
16=0zQn
17-----END PGP SIGNATURE-----

Timestamp of file with hash 45b97e7100d59a8015d51f47d4403a42ca4a6457ae8da97a2cbcf721bf8f5f59 -

hebasto closed this on Aug 30, 2020

hebasto reopened this on Aug 30, 2020

hebasto commented at 4:52 pm on August 30, 2020: member

@ajtowns @jnewbery @vasild

Begging for another review round :)

hebasto closed this on Aug 30, 2020

hebasto reopened this on Aug 30, 2020

jnewbery commented at 10:18 am on August 31, 2020: member

ACK ea74e10acf17903e44c85e3678853414653dd4e1

ajtowns commented at 4:21 pm on August 31, 2020: member

ACK ea74e10acf17903e44c85e3678853414653dd4e1

MarcoFalke merged this on Sep 1, 2020

MarcoFalke closed this on Sep 1, 2020

hebasto deleted the branch on Sep 1, 2020

sidhujag referenced this in commit ed409a3f99 on Sep 3, 2020

in src/net_processing.cpp:1329 in ea74e10acf

1325@@ -1327,7 +1326,7 @@ void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std:
1326     }
1327 
1328     m_connman.ForEachNode([this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) {
1329-        AssertLockHeld(cs_main);
1330+        LockAssertion lock(::cs_main);

ryanofsky commented at 5:25 pm on September 3, 2020:

What’s the reason to prefer LockAssertion to AssertLockHeld if AssertLockHeld works at compile time and runtime and LockAssertion only works at compile time? Maybe this could be clarified https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization

hebasto commented at 6:00 pm on September 3, 2020:

When TSA fails to detect that a mutex is locked we do want to use LockAssertion as using AssertLockHeld no longer helps:

 0$ git diff
 1diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 2index 0e049bd66..90bd626fc 100644
 3--- a/src/net_processing.cpp
 4+++ b/src/net_processing.cpp
 5@@ -1370,7 +1370,7 @@ void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std:
 6     }
 7 
 8     m_connman.ForEachNode([this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) {
 9-        LockAssertion lock(::cs_main);
10+        AssertLockHeld(::cs_main);
11 
12         // TODO: Avoid the repeated-serialization here
13         if (pnode->nVersion < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect)
14$ make
15net_processing.cpp:1373:9: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex> >' requires holding mutex 'cs_main' exclusively [-Wthread-safety-analysis]
16        AssertLockHeld(::cs_main);
17        ^
18./sync.h:79:28: note: expanded from macro 'AssertLockHeld'
19#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
20                           ^
21net_processing.cpp:1378:9: warning: calling function 'ProcessBlockAvailability' requires holding mutex 'cs_main' exclusively [-Wthread-safety-analysis]
22        ProcessBlockAvailability(pnode->GetId());
23        ^
24net_processing.cpp:1379:30: warning: calling function 'State' requires holding mutex 'cs_main' exclusively [-Wthread-safety-analysis]
25        CNodeState &state = *State(pnode->GetId());
26                             ^
27net_processing.cpp:1383:18: warning: calling function 'PeerHasHeader' requires holding mutex 'cs_main' exclusively [-Wthread-safety-analysis]
28                !PeerHasHeader(&state, pindex) && PeerHasHeader(&state, pindex->pprev)) {
29                 ^
30net_processing.cpp:1383:51: warning: calling function 'PeerHasHeader' requires holding mutex 'cs_main' exclusively [-Wthread-safety-analysis]
31                !PeerHasHeader(&state, pindex) && PeerHasHeader(&state, pindex->pprev)) {
32                                                  ^
335 warnings generated.

hebasto commented at 6:03 pm on September 3, 2020:

And this LockAssertion application is documented: https://github.com/bitcoin/bitcoin/blob/a0a422c34cfd6514d0cc445bd784d3ee1a2d1749/src/sync.h#L355-L357

hebasto commented at 6:04 pm on September 3, 2020:

Maybe this could be clarified https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization

Mind suggesting the clarification as my English is so poor?

ryanofsky commented at 9:45 pm on September 3, 2020:

Oh, I wasn’t aware AssertLockHeld annotations changed from ASSERT_EXCLUSIVE_LOCK to EXCLUSIVE_LOCKS_REQUIRED in #19668. This seems like a questionable decision to me, because now instead of being able to use AssertLockHeld everywhere, we have separate confusing AssertLockHeld and LockAssertion calls.

I think I would want to clarify this by changing AssertLockHeld annotations back to ASSERT_EXCLUSIVE_LOCK, making it a weaker assertion that only causes errors at runtime, never compile time. Then we could drop LockAssertion and s/LockAssertion/AssertLockHeld/ and only have one type of assertion to think about.

If this doesn’t work because it interferes with work you are doing, another option might be to s/LockAssertion/WeaklyAssertLockHeld/ everywhere. (Also maybe simplify the implementation to be an ASSERT_EXCLUSIVE_LOCK function instead of a class instance that requires a variable declaration.) The new naming should make it clearer which call is preferred:

AssertLockHeld - stricter assertion which causes errors at compile time if the compiler supports annotations, or causes errors at runtime if the compiler doesn’t support annotations
WeaklyAssertLockHeld - less strict alternative which only triggers errors at runtime, never compile-time, and can be used when it’s not possible for compiler to determine that the mutex is locked.

ajtowns commented at 10:29 pm on September 3, 2020:

We have three types of assertions related for locking:

marking a function as EXCLUSIVE_LOCKS_REQUIRED has a compile time check with clang that the caller has already obtained the lock
declaring a LockAssertion instance overrides the clang compile time checks by saying that we’ve already somehow acquired the lock in a way that we can’t prove via clang thread safety annotations. This is useful for lambda functions (which could be annotated) that are called from some generic dispatcher like ForEach (that can’t be annotated).
adding a call to AssertLockHeld does a runtime check if DEBUG_LOCKORDER is enabled. it’s annotated with EXCLUSIVE_LOCKS_REQUIRED so is redundant with the compile time checks, but the compile time checks aren’t available outside of clang. it’s automatically called from LockAssertion to ensure that the compile time checks aren’t overridden incorrectly.

As a result:

using AssertLockHeld everywhere remains fine, and no longer needlessly disables the compile time checks
LockAssertion should only be used very rarely – and in fact it’s only used in net_processing in some ForEach/ForEachNode functions.

Maybe we should move the locking outside of the ForEach functions into the caller, and convert them into for ( x : nodes ) { ... } instead, in which case we could probably do away with LockAssertion entirely.

ryanofsky commented at 11:31 pm on September 3, 2020:

no longer needlessly disables the compile time checks

It’s funny the only reason I ever called it was to disable compile time checks, and now it doesn’t do that anymore! :smile: If really the only thing AssertLockHeld is doing is adding runtime checks on top of compile time checks that are already present, I think that is nearly useless. Anyway, can take it up in another PR.

ryanofsky commented at 11:51 pm on September 3, 2020:

Anyway, can take it up in another PR.

Created #19865 to experiment

ajtowns commented at 11:54 pm on September 3, 2020:

You use LockAssertion g(cs_foo); to tell the compiler that a lock is guaranteed to already be held within a code block even though the function annotations don’t guarantee it. AssertLockHeld is useful if you’re not compiling with clang.

ryanofsky commented at 0:12 am on September 4, 2020:

AssertLockHeld is useful if you’re not compiling with clang.

Yes, that’s what I mean by “nearly useless.” It can help developers who aren’t using clang, and who are compiling with DEBUG_LOCKORDER, and who want to wait for an assert to trigger at runtime. But it won’t make the codebase more thread safe because we already don’t merge changes that don’t compile with clang.

Any case, if you see problems with #19865, it would be good to comment there. I’m not an expert on lock annotations, so maybe the AssertLockHeld calls removed in #19865 are actually doing something useful besides haphazardly repeating compile time assertions at runtime.

hebasto commented at 8:06 am on September 4, 2020:

AssertLockHeld is useful if you’re not compiling with clang.

Yes, that’s what I mean by “nearly useless.” It can help developers who aren’t using clang, and who are compiling with DEBUG_LOCKORDER, and who want to wait for an assert to trigger at runtime. But it won’t make the codebase more thread safe because we already don’t merge changes that don’t compile with clang.

I disagree with statements about “useless” AssertLockHeld.

ryanofsky referenced this in commit d0ac4b97a8 on Sep 3, 2020

laanwj referenced this in commit 99a8eb6051 on Sep 4, 2020

sidhujag referenced this in commit 45b8840fd3 on Sep 9, 2020

ryanofsky referenced this in commit b62ef19d17 on Sep 14, 2020

ryanofsky referenced this in commit 2cc908d201 on Sep 14, 2020

deadalnix referenced this in commit 8ea2e27baa on Sep 22, 2021

deadalnix referenced this in commit 83cdc4fe2f on Sep 22, 2021

deadalnix referenced this in commit 52376cedea on Sep 22, 2021

deadalnix referenced this in commit 9c6e807163 on Sep 22, 2021

deadalnix referenced this in commit ac7014aca8 on Sep 22, 2021

DrahtBot locked this on Feb 15, 2022

Do not hide compile-time thread safety warnings #19668

Conflicts