Using Mutex provides stronger guarantee than GlobalMutex wrt Clang's
thread safety analysis. Thus it is better to reduce the usage of
GlobalMutex in favor of Mutex.
Using Mutex for g_sqlite_mutex is ok because its usage is limited in
wallet/sqlite.cpp and it does not require propagating the negative
annotations to not relevant code.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hebasto, TheCharlatan, achow101 |
| Stale ACK | w0xlt |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
I tried to see if this made any difference in practice, but it didn't. Annotations are disabled for con-/de-structors:
diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp
index 5d81af2fbf..c8a9262150 100644
--- a/src/wallet/sqlite.cpp
+++ b/src/wallet/sqlite.cpp
@@ -113,7 +113,6 @@ SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_pa
if (ret != SQLITE_OK) {
throw std::runtime_error(strprintf("SQLiteDatabase: Failed to initialize SQLite: %s\n", sqlite3_errstr(ret)));
}
- }
try {
Open();
@@ -122,6 +121,7 @@ SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_pa
Cleanup();
throw;
}
+ }
}
void SQLiteBatch::SetupSQLStatements()
Also, it is annotated in the implementation only (not the definition), (for obvious reasons, though).
However, it does add a hint for developer reading the code, so lgtm.
22 | @@ -23,7 +23,13 @@ 23 | namespace wallet { 24 | static constexpr int32_t WALLET_SCHEMA_VERSION = 0; 25 | 26 | -static GlobalMutex g_sqlite_mutex; 27 | +/** 28 | + * This mutex protects SQLite initialization and shutdown. 29 | + * sqlite3_config() and sqlite3_shutdown() are not threadsafe (sqlite3_initialize() is). 30 | + * Concurrent threads that execute SQLiteDatabase::SQLiteDatabase() should have just one
I'm not sure what you meant with this sentence. g_sqlite_count can be higher than one.
"one" refers to "concurrent threads"
Changed "one" to "one of them".
149 | @@ -144,7 +150,7 @@ SQLiteDatabase::~SQLiteDatabase() 150 | Cleanup(); 151 | } 152 | 153 | -void SQLiteDatabase::Cleanup() noexcept 154 | +void SQLiteDatabase::Cleanup() noexcept EXCLUSIVE_LOCKS_REQUIRED(!g_sqlite_mutex)
from developer-notes: "In functions that are declared separately from where they are defined, the thread safety annotations should be added exclusively to the function declaration."
Could move g_sqlite_mutex and g_sqlite_count to be static members of SQLiteDatabase` though I think.
Moved to SQLiteDatabase. This way if some other method calls Cleanup() from the .h file they will be enforced to not hold the mutex. This is impossible though, because the mutex was defined solely in the .cpp file. I think either way is fine.
Unrelated, the developer-notes read:
... should be added exclusively to the function declaration. Annotations on the definition could lead to false positives (lack of compile failure) at call sites between the two.
Should that not be "false negatives"?
What is "the two"?
72 | + * of them do the init and the rest wait for it to complete before all can proceed. 73 | + */ 74 | + static Mutex g_sqlite_mutex; 75 | + static int g_sqlite_count GUARDED_BY(g_sqlite_mutex); 76 | + 77 | + void Cleanup() noexcept EXCLUSIVE_LOCKS_REQUIRED(!g_sqlite_mutex);
Not sure about this annotation as @MarcoFalke pointed:
Annotations are disabled for con-/de-structors
Nevertheless, it would be nice to add AssertLockNotHeld(g_sqlite_mutex); to the function implemetation.
Added AssertLockNotHeld().
I kept the annotation on the Cleanup() method, even though it is called only from the constructor and the destructor in case it gets called from elsewhere in the future.
This looks a bit strange:
Mutex m1;
int global_counter GUARDED_BY(m1){0};
void f() EXCLUSIVE_LOCKS_REQUIRED(!m1)
{
AssertLockNotHeld(m1);
LOCK(m1);
++global_counter;
}
SQLiteDatabase::SQLiteDatabase()
{
LOCK(m1);
// ok!?
f();
void SQLiteDatabase::Open() // any other method
{
LOCK(m1);
// error: cannot call function 'f' while mutex 'm1' is held [-Werror,-Wthread-safety-analysis]
f();
I understand annotations on the constructor are disabled, but the above is more than that. Are constructors treated like NO_THREAD_SAFETY_ANALYSIS?
Are constructors treated like NO_THREAD_SAFETY_ANALYSIS?
Approach ACK 0be7de52f2d0d7c101afdf1b4e315a395015867b.
Built on Ubuntu 22.04 using clang 14.0 with -Wthread-safety-negative.
Using `Mutex` provides stronger guarantee than `GlobalMutex` wrt Clang's
thread safety analysis. Thus it is better to reduce the usage of
`GlobalMutex` in favor of `Mutex`.
Using `Mutex` for `g_sqlite_mutex` is ok because its usage is limited in
`wallet/sqlite.cpp` and it does not require propagating the negative
annotations to not relevant code.
0be7de52f2...4163093d63: rebase and add AssertLockNotHeld() as suggested
re-ACK 4163093d6374e865dc57e33dbea0e7e359062e0a
ACK 4163093d6374e865dc57e33dbea0e7e359062e0a
ACK 4163093d6374e865dc57e33dbea0e7e359062e0a