Skip to content

Commit 400aa68

Browse files
seditedsedited
committed
Merge bitcoin/bitcoin#34809: threadsafety: Add STDLOCK() macro for StdMutex
8d2f068 sync: Use StdMutex for thread safety annotations (Anthony Towns) cbc231e scripted-diff: logging: Switch from StdLockGuard to STDLOCK (Anthony Towns) f808786 logging: Add missing thread safety annotations (Anthony Towns) e196cf2 util/stdmutex.h: Add STDLOCK() and improve annotation checking for StdMutex (Anthony Towns) Pull request description: Using `STDLOCK(mutex)` instead of `StdLockGuard guard(mutex)` allows clang to propagate missing lock annotations backwards (for global locks or locks in the same class, anyway), and also avoids declaring a dummy name. Use this in logging.h, and also use it in sync.cpp, adding annotations around the internal structure. ACKs for top commit: theuni: ACK 8d2f068 sedited: ACK 8d2f068 Tree-SHA512: ee23f6a7bcc62cc6d9ea88afa863a9018e53a0932272bb14241441fb69066c6633c4e19aadd3dd7599848d4742099d63756be4eff1969775293b728f3dd187aa
2 parents 16613c9 + 8d2f068 commit 400aa68

4 files changed

Lines changed: 46 additions & 37 deletions

File tree

‎src/logging.cpp‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ static int FileWriteStr(std::string_view str, FILE *fp)
5353

5454
bool BCLog::Logger::StartLogging()
5555
{
56-
StdLockGuard scoped_lock(m_cs);
56+
STDLOCK(m_cs);
5757

5858
assert(m_buffering);
5959
assert(m_fileout == nullptr);
@@ -97,7 +97,7 @@ bool BCLog::Logger::StartLogging()
9797

9898
void BCLog::Logger::DisconnectTestLogger()
9999
{
100-
StdLockGuard scoped_lock(m_cs);
100+
STDLOCK(m_cs);
101101
m_buffering = true;
102102
if (m_fileout != nullptr) fclose(m_fileout);
103103
m_fileout = nullptr;
@@ -111,7 +111,7 @@ void BCLog::Logger::DisconnectTestLogger()
111111
void BCLog::Logger::DisableLogging()
112112
{
113113
{
114-
StdLockGuard scoped_lock(m_cs);
114+
STDLOCK(m_cs);
115115
assert(m_buffering);
116116
assert(m_print_callbacks.empty());
117117
}
@@ -159,7 +159,7 @@ bool BCLog::Logger::WillLogCategoryLevel(BCLog::LogFlags category, BCLog::Level
159159

160160
if (!WillLogCategory(category)) return false;
161161

162-
StdLockGuard scoped_lock(m_cs);
162+
STDLOCK(m_cs);
163163
const auto it{m_category_log_levels.find(category)};
164164
return level >= (it == m_category_log_levels.end() ? LogLevel() : it->second);
165165
}
@@ -392,7 +392,7 @@ BCLog::LogRateLimiter::Status BCLog::LogRateLimiter::Consume(
392392
const SourceLocation& source_loc,
393393
const std::string& str)
394394
{
395-
StdLockGuard scoped_lock(m_mutex);
395+
STDLOCK(m_mutex);
396396
auto& stats{m_source_locations.try_emplace(source_loc, m_max_bytes).first->second};
397397
Status status{stats.m_dropped_bytes > 0 ? Status::STILL_SUPPRESSED : Status::UNSUPPRESSED};
398398

@@ -423,7 +423,7 @@ void BCLog::Logger::FormatLogStrInPlace(std::string& str, BCLog::LogFlags catego
423423

424424
void BCLog::Logger::LogPrintStr(std::string_view str, SourceLocation&& source_loc, BCLog::LogFlags category, BCLog::Level level, bool should_ratelimit)
425425
{
426-
StdLockGuard scoped_lock(m_cs);
426+
STDLOCK(m_cs);
427427
return LogPrintStr_(str, std::move(source_loc), category, level, should_ratelimit);
428428
}
429429

@@ -556,7 +556,7 @@ void BCLog::LogRateLimiter::Reset()
556556
{
557557
decltype(m_source_locations) source_locations;
558558
{
559-
StdLockGuard scoped_lock(m_mutex);
559+
STDLOCK(m_mutex);
560560
source_locations.swap(m_source_locations);
561561
m_suppression_active = false;
562562
}
@@ -598,7 +598,7 @@ bool BCLog::Logger::SetCategoryLogLevel(std::string_view category_str, std::stri
598598
const auto level = GetLogLevel(level_str);
599599
if (!level.has_value() || level.value() > MAX_USER_SETABLE_SEVERITY_LEVEL) return false;
600600

601-
StdLockGuard scoped_lock(m_cs);
601+
STDLOCK(m_cs);
602602
m_category_log_levels[flag] = level.value();
603603
return true;
604604
}

‎src/logging.h‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -192,28 +192,28 @@ namespace BCLog {
192192
/** Returns whether logs will be written to any output */
193193
bool Enabled() const EXCLUSIVE_LOCKS_REQUIRED(!m_cs)
194194
{
195-
StdLockGuard scoped_lock(m_cs);
195+
STDLOCK(m_cs);
196196
return m_buffering || m_print_to_console || m_print_to_file || !m_print_callbacks.empty();
197197
}
198198

199199
/** Connect a slot to the print signal and return the connection */
200200
std::list<std::function<void(const std::string&)>>::iterator PushBackCallback(std::function<void(const std::string&)> fun) EXCLUSIVE_LOCKS_REQUIRED(!m_cs)
201201
{
202-
StdLockGuard scoped_lock(m_cs);
202+
STDLOCK(m_cs);
203203
m_print_callbacks.push_back(std::move(fun));
204204
return --m_print_callbacks.end();
205205
}
206206

207207
/** Delete a connection */
208208
void DeleteCallback(std::list<std::function<void(const std::string&)>>::iterator it) EXCLUSIVE_LOCKS_REQUIRED(!m_cs)
209209
{
210-
StdLockGuard scoped_lock(m_cs);
210+
STDLOCK(m_cs);
211211
m_print_callbacks.erase(it);
212212
}
213213

214-
size_t NumConnections()
214+
size_t NumConnections() EXCLUSIVE_LOCKS_REQUIRED(!m_cs)
215215
{
216-
StdLockGuard scoped_lock(m_cs);
216+
STDLOCK(m_cs);
217217
return m_print_callbacks.size();
218218
}
219219

@@ -224,7 +224,7 @@ namespace BCLog {
224224

225225
void SetRateLimiting(std::shared_ptr<LogRateLimiter> limiter) EXCLUSIVE_LOCKS_REQUIRED(!m_cs)
226226
{
227-
StdLockGuard scoped_lock(m_cs);
227+
STDLOCK(m_cs);
228228
m_limiter = std::move(limiter);
229229
}
230230

@@ -240,17 +240,17 @@ namespace BCLog {
240240

241241
std::unordered_map<LogFlags, Level> CategoryLevels() const EXCLUSIVE_LOCKS_REQUIRED(!m_cs)
242242
{
243-
StdLockGuard scoped_lock(m_cs);
243+
STDLOCK(m_cs);
244244
return m_category_log_levels;
245245
}
246246
void SetCategoryLogLevel(const std::unordered_map<LogFlags, Level>& levels) EXCLUSIVE_LOCKS_REQUIRED(!m_cs)
247247
{
248-
StdLockGuard scoped_lock(m_cs);
248+
STDLOCK(m_cs);
249249
m_category_log_levels = levels;
250250
}
251-
void AddCategoryLogLevel(LogFlags category, Level level)
251+
void AddCategoryLogLevel(LogFlags category, Level level) EXCLUSIVE_LOCKS_REQUIRED(!m_cs)
252252
{
253-
StdLockGuard scoped_lock(m_cs);
253+
STDLOCK(m_cs);
254254
m_category_log_levels[category] = level;
255255
}
256256
bool SetCategoryLogLevel(std::string_view category_str, std::string_view level_str) EXCLUSIVE_LOCKS_REQUIRED(!m_cs);

‎src/sync.cpp‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#include <logging/timer.h>
88
#include <tinyformat.h>
99
#include <util/log.h>
10+
#include <util/stdmutex.h>
1011
#include <util/strencodings.h>
1112
#include <util/threadnames.h>
1213

@@ -87,10 +88,10 @@ using LockOrders = std::map<LockPair, LockStack>;
8788
using InvLockOrders = std::set<LockPair>;
8889

8990
struct LockData {
90-
LockStacks m_lock_stacks;
91-
LockOrders lockorders;
92-
InvLockOrders invlockorders;
93-
std::mutex dd_mutex;
91+
LockStacks m_lock_stacks GUARDED_BY(dd_mutex);
92+
LockOrders lockorders GUARDED_BY(dd_mutex);
93+
InvLockOrders invlockorders GUARDED_BY(dd_mutex);
94+
StdMutex dd_mutex;
9495
};
9596

9697
LockData& GetLockData() {
@@ -166,7 +167,7 @@ static void push_lock(MutexType* c, const CLockLocation& locklocation)
166167
std::is_base_of_v<std::recursive_mutex, MutexType>;
167168

168169
LockData& lockdata = GetLockData();
169-
std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
170+
STDLOCK(lockdata.dd_mutex);
170171

171172
LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()];
172173
lock_stack.emplace_back(c, locklocation);
@@ -206,7 +207,7 @@ static void push_lock(MutexType* c, const CLockLocation& locklocation)
206207
static void pop_lock()
207208
{
208209
LockData& lockdata = GetLockData();
209-
std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
210+
STDLOCK(lockdata.dd_mutex);
210211

211212
LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()];
212213
lock_stack.pop_back();
@@ -226,7 +227,7 @@ template void EnterCritical(const char*, const char*, int, std::recursive_mutex*
226227
void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line)
227228
{
228229
LockData& lockdata = GetLockData();
229-
std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
230+
STDLOCK(lockdata.dd_mutex);
230231

231232
const LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()];
232233
if (!lock_stack.empty()) {
@@ -257,7 +258,7 @@ void LeaveCritical()
257258
static std::string LocksHeld()
258259
{
259260
LockData& lockdata = GetLockData();
260-
std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
261+
STDLOCK(lockdata.dd_mutex);
261262

262263
const LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()];
263264
std::string result;
@@ -269,7 +270,7 @@ static std::string LocksHeld()
269270
static bool LockHeld(void* mutex)
270271
{
271272
LockData& lockdata = GetLockData();
272-
std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
273+
STDLOCK(lockdata.dd_mutex);
273274

274275
const LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()];
275276
for (const LockStackItem& i : lock_stack) {
@@ -302,7 +303,7 @@ template void AssertLockNotHeldInternal(const char*, const char*, int, Recursive
302303
void DeleteLock(void* cs)
303304
{
304305
LockData& lockdata = GetLockData();
305-
std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
306+
STDLOCK(lockdata.dd_mutex);
306307
const LockPair item = std::make_pair(cs, nullptr);
307308
LockOrders::iterator it = lockdata.lockorders.lower_bound(item);
308309
while (it != lockdata.lockorders.end() && it->first.first == cs) {
@@ -321,7 +322,7 @@ void DeleteLock(void* cs)
321322
bool LockStackEmpty()
322323
{
323324
LockData& lockdata = GetLockData();
324-
std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
325+
STDLOCK(lockdata.dd_mutex);
325326
const auto it = lockdata.m_lock_stacks.find(std::this_thread::get_id());
326327
if (it == lockdata.m_lock_stacks.end()) {
327328
return true;

‎src/util/stdmutex.h‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010
// Thread Safety Analysis and provides appropriate annotation macros.
1111
#include <threadsafety.h> // IWYU pragma: export
1212

13+
#include <util/macros.h>
14+
1315
#include <mutex>
1416

1517
// StdMutex provides an annotated version of std::mutex for us,
@@ -23,15 +25,21 @@ class LOCKABLE StdMutex : public std::mutex
2325
//! with the ! operator, to indicate that a mutex should not be held.
2426
const StdMutex& operator!() const { return *this; }
2527
#endif // __clang__
26-
};
2728

28-
// StdLockGuard provides an annotated version of std::lock_guard for us,
29-
// and should only be used when sync.h Mutex/LOCK/etc are not usable.
30-
class SCOPED_LOCKABLE StdLockGuard : public std::lock_guard<StdMutex>
31-
{
32-
public:
33-
explicit StdLockGuard(StdMutex& cs) EXCLUSIVE_LOCK_FUNCTION(cs) : std::lock_guard<StdMutex>(cs) {}
34-
~StdLockGuard() UNLOCK_FUNCTION() = default;
29+
// StdMutex::Guard provides an annotated version of std::lock_guard for us.
30+
class SCOPED_LOCKABLE Guard : public std::lock_guard<StdMutex>
31+
{
32+
public:
33+
explicit Guard(StdMutex& cs) EXCLUSIVE_LOCK_FUNCTION(cs) : std::lock_guard<StdMutex>(cs) {}
34+
~Guard() UNLOCK_FUNCTION() = default;
35+
};
36+
37+
static inline StdMutex& CheckNotHeld(StdMutex& cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; }
3538
};
3639

40+
// Provide STDLOCK(..) wrapper around StdMutex::Guard that checks the lock is not already held
41+
#define STDLOCK(cs) StdMutex::Guard UNIQUE_NAME(criticalblock){StdMutex::CheckNotHeld(cs)}
42+
43+
using StdLockGuard = StdMutex::Guard; // TODO: remove, provided for backwards compat only
44+
3745
#endif // BITCOIN_UTIL_STDMUTEX_H

0 commit comments

Comments
 (0)