refactor: Improve thread safety analysis by propagating some negative capabilities #25175

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:220520-nega changing 1 files +10 −7

hebasto commented at 11:40 AM on May 20, 2022: member

This PR is a follow-up for bitcoin/bitcoin#22778 and bitcoin/bitcoin#24062, and it seems required for bitcoin/bitcoin#24931.

See details in the commit messages.

refactor: Propagate negative `!m_most_recent_block_mutex` capability

Could be verified with
$ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative'
$ make clean
$ make 2>&1 | grep m_most_recent_block_mutex

5a6e3c1db3

refactor: Propagate negative `!m_tx_relay_mutex` capability

Could be verified with
$ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative'
$ make clean
$ make 2>&1 | grep m_tx_relay_mutex

2b3373c152

DrahtBot added the label P2P on May 20, 2022
DrahtBot added the label Refactoring on May 20, 2022
in src/net_processing.cpp:476 in 2b3373c152
```
 471 | @@ -472,15 +472,16 @@ class PeerManagerImpl final : public PeerManager
 472 |          EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 473 |      void BlockChecked(const CBlock& block, const BlockValidationState& state) override
 474 |          EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 475 | -    void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock) override;
 476 | +    void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock) override
 477 | +        EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex);
```
MarcoFalke commented at 12:47 PM on May 20, 2022:

I wonder how useful it is to annotate interface functions with member mutexes. The only allowed caller can't see the mutex, as it is private.

hebasto commented at 1:24 PM on May 20, 2022:

An annotation is not for a caller, rather for Clang Thread Safety Analysis C++ extension.

ajtowns commented at 3:33 PM on May 20, 2022:

(An external caller (accessing via the virtual/override) can't see the mutex, but an in-class caller could)

MarcoFalke commented at 5:43 PM on May 20, 2022:

Yeah, I was just thinking that it would be incredibly unlikely or even wrong to even have an in-class caller.
MarcoFalke approved
MarcoFalke commented at 12:48 PM on May 20, 2022: member

lgtm
jonatack commented at 1:33 PM on May 20, 2022: member

Concept ACK
ajtowns commented at 2:17 PM on May 20, 2022: member

ACK 2b3373c1520aa0b41277cd89956224e08cbd79dd
w0xlt approved
w0xlt commented at 3:24 PM on May 20, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/25175/commits/2b3373c1520aa0b41277cd89956224e08cbd79dd
MarcoFalke merged this on May 20, 2022
MarcoFalke closed this on May 20, 2022
hebasto deleted the branch on May 20, 2022
sidhujag referenced this in commit e1cec3b50d on May 28, 2022
DrahtBot locked this on May 20, 2023

Contributors

Labels

Linked (view graph)

#24931 Strengthen thread safety assertions