Fix -Wthread-safety-analysis warnings. Compile with -Wthread-safety-analysis if available.

practicalswift commented at 2:49 pm on July 18, 2017: contributor

Add mutex requirement for AddToCompactExtraTransactions(…).
Use -Wthread-safety-analysis if available.
Rebased on top of https://github.com/TheBlueMatt/bitcoin/commits/2017-08-test-10923 - now includes: Fix -Wthread-safety-analysis warnings. Change the sync.h primitives to std from boost.

TheBlueMatt commented at 8:57 pm on July 18, 2017: member

Do we actually enforce that on Travis? Its nice to have the auto-checker stuff, but AFAIR its (barely) hooked up anywhere?

practicalswift commented at 9:24 pm on July 18, 2017: contributor

@TheBlueMatt Travis is currently not compiling with Clang Thread Safety Analysis warnings (-Wthread-safety-analysis) enabled. (Do we want to change that?)

Personally I build daily using clang with -Wthread-safety-analysis enabled.

In the general case we want to fix all warnings emitted when compiling with -Wthread-safety-analysis, don’t we? :-)

sipa commented at 0:39 am on July 19, 2017: member

If we’re going to revive this safety checking infrastructure (it was introduced in #2003, and I don’t think updated much since), there are probably many other places to add annotations.

jonasschnelli commented at 9:49 am on July 19, 2017: contributor

Agree with @sipa. @practicalswift whats the reason of only add the safety checking infrastructure to AddToCompactExtraTransactions?

jonasschnelli added the label Refactoring on Jul 19, 2017

practicalswift commented at 2:50 pm on July 19, 2017: contributor

The reason I’m starting with AddToCompactExtraTransactions(…) is simply to fix the only current warning when building with clang’s -Wthread-safety-analysis enabled:

0net_processing.cpp:587:10: warning: reading variable 'vExtraTxnForCompact' requires holding mutex 'cs_main' [-Wthread-safety-analysis]

The reason for the warning is that vExtraTxnForCompact is marked as guarded by the cs_main-mutex:

0static std::vector<std::pair<uint256, CTransactionRef>> vExtraTxnForCompact 
1    GUARDED_BY(cs_main);

clang detects that vExtraTxnForCompact is accessed in AddToCompactExtraTransactions(…) and that this function lacks the EXCLUSIVE_LOCKS_REQUIRED(cs_main) annotation that logically should follow from such access (if intentional).

This PR fixes that and after applying this PR the code base is free from -Wthread-safety-analysis warnings.

I’d be glad to extend the usage of GUARDED_BY(…) annotations (and the corresponding EXCLUSIVE_LOCKS_REQUIRED(…) annotations) in the code base, but that is a larger undertaking that probably belongs in a follow-up PR.

I guess one reason not to fix the current warning is if we for some reason instead would want to remove this thread-safety infrastructure. Let me know if that is the case :-)

practicalswift commented at 3:06 pm on July 19, 2017: contributor

Additional reading: The “C/C++ Thread Safety Analysis” paper (Hutchins, Ballman & Sutherland, 2014) paper describing clang thread-safety analysis and how it is used for the Google C++ codebase:

They essentially provide a static type system for threads, and can detect potential race conditions and deadlocks. This paper describes Clang Thread Safety Analysis, a tool which uses annotations to declare and enforce thread safety policies in C and C++ programs. […] It has been deployed on a large scale at Google; all C++ code at Google is now compiled with thread safety analysis enabled by default. […] The GUARDED_BY attribute declares that a thread must lock mu before it can read or write to balance, thus ensuring that the increment and decrement operations are atomic. Similarly, REQUIRES declares that the calling thread must lock mu before calling withdrawImpl. Because the caller is assumed to have locked mu, it is safe to modify balance within the body of the method.

TheBlueMatt commented at 10:12 pm on July 19, 2017: member

Ah! Didn’t realize its to fix a warning, yea, lets a) do this and b) can you add a -Werror=thread-safety-analysis here, too? I was informed travis already does a clang build for OSX, so it should be able to catch errors here :).

practicalswift renamed this:
~~Add mutex requirement for AddToCompactExtraTransactions(…)~~
Fix -Wthread-safety-analysis warning: Add mutex requirement for AddToCompactExtraTransactions(…)
on Jul 19, 2017

practicalswift commented at 10:26 pm on July 19, 2017: contributor

@TheBlueMatt Sure! I’ve updated the PR title to make things more clear :-)

TheBlueMatt commented at 3:31 pm on July 24, 2017: member

I think we really should make travis enforce this. It shouldnt be hard cause we already have –enable-werror by default, just need to hook up -Werror=thread-safety-analysis if the compiler supports it (which our clang-osx build should).

practicalswift commented at 3:56 pm on July 24, 2017: contributor

@TheBlueMatt Would something along the lines of the following do the trick for Travis?

 0diff --git a/.travis.yml b/.travis.yml
 1index a79428f..423b04e 100644
 2--- a/.travis.yml
 3+++ b/.travis.yml
 4@@ -34,7 +34,7 @@ env:
 5 # No wallet
 6     - HOST=x86_64-unknown-linux-gnu PACKAGES="python3" DEP_OPTS="NO_WALLET=1" RUN_TESTS=true GOAL="install" BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports"
 7 # Cross-Mac
 8-    - HOST=x86_64-apple-darwin11 PACKAGES="cmake imagemagick libcap-dev librsvg2-bin libz-dev libbz2-dev libtiff-tools python-dev" BITCOIN_CONFIG="--enable-gui --enable-reduce-exports" OSX_SDK=10.11 GOAL="deploy"
 9+    - HOST=x86_64-apple-darwin11 PACKAGES="cmake imagemagick libcap-dev librsvg2-bin libz-dev libbz2-dev libtiff-tools python-dev" BITCOIN_CONFIG="--enable-gui --enable-reduce-exports" OSX_SDK=10.11 GOAL="deploy" CPPFLAGS="-Werror=thread-safety-analysis"
10
11 before_install:
12     - export PATH=$(echo $PATH | tr ':' "\n" | sed '/\/opt\/python/d' | tr "\n" ":" | sed "s|::|:|g")

Or did you mean always enabling that warning if the compiler supports it?

TheBlueMatt commented at 8:57 pm on July 24, 2017: member

@practicalswift I was referring to the -Werror support we already have in configure.ac. Best to use that, I’d think.

practicalswift commented at 9:57 pm on July 24, 2017: contributor

@TheBlueMatt Got it! What about this:

 0diff --git a/configure.ac b/configure.ac
 1index aea5d71..ec3877d 100644
 2--- a/configure.ac
 3+++ b/configure.ac
 4@@ -241,6 +241,7 @@ if test "x$enable_werror" = "xyes"; then
 5     AC_MSG_ERROR("enable-werror set but -Werror is not usable")
 6   fi
 7   AX_CHECK_COMPILE_FLAG([-Werror=vla],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=vla"],,[[$CXXFLAG_WERROR]])
 8+  AX_CHECK_COMPILE_FLAG([-Werror=thread-safety-analysis],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=thread-safety-analysis"],,[[$CXXFLAG_WERROR]])
 9 fi
10
11 if test "x$CXXFLAGS_overridden" = "xno"; then
12@@ -249,6 +250,7 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
13   AX_CHECK_COMPILE_FLAG([-Wformat],[CXXFLAGS="$CXXFLAGS -Wformat"],,[[$CXXFLAG_WERROR]])
14   AX_CHECK_COMPILE_FLAG([-Wvla],[CXXFLAGS="$CXXFLAGS -Wvla"],,[[$CXXFLAG_WERROR]])
15   AX_CHECK_COMPILE_FLAG([-Wformat-security],[CXXFLAGS="$CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]])
16+  AX_CHECK_COMPILE_FLAG([-Wthread-safety-analysis],[CXXFLAGS="$CXXFLAGS -Wthread-safety-analysis"],,[[$CXXFLAG_WERROR]])
17
18   ## Some compilers (gcc) ignore unknown -Wno-* options, but warn about all
19   ## unknown options if any other warning is produced. Test the -Wfoo case, and

Looks good? :-)

TheBlueMatt commented at 8:00 pm on July 25, 2017: member

Heh, I figured you’d incldue it in this PR. Anyway, utACK this pr by itself.

theuni commented at 2:18 pm on July 31, 2017: member

utACK ef2aca6c12efc48ce557a87c90ecf2d96ad4885e

TheBlueMatt commented at 3:01 pm on August 1, 2017: member

utACK ef2aca6c12efc48ce557a87c90ecf2d96ad4885e, though its useful to hold off merging this until #10923 is ready as a test for #10923.

practicalswift force-pushed on Aug 18, 2017

practicalswift commented at 1:50 pm on August 18, 2017: contributor

Cherry-picked 9df85380aae4219920ccca80efac2bdf408519d8 into this PR as suggested by @TheBlueMatt in #10923! @TheBlueMatt, looks good? :-)

practicalswift renamed this:
~~Fix -Wthread-safety-analysis warning: Add mutex requirement for AddToCompactExtraTransactions(…)~~
Fix -Wthread-safety-analysis warning: Add mutex requirement for AddToCompactExtraTransactions(…) + enable --enable-werror for OS X Travis
on Aug 18, 2017

TheBlueMatt commented at 9:40 pm on August 18, 2017: member

@ptracticalswift maybe rebase on #10923 and take the patches from https://github.com/TheBlueMatt/bitcoin/commits/2017-08-test-10923 (squashed, of course)?

practicalswift force-pushed on Aug 19, 2017

practicalswift renamed this:
~~Fix -Wthread-safety-analysis warning: Add mutex requirement for AddToCompactExtraTransactions(…) + enable --enable-werror for OS X Travis~~
Fix -Wthread-safety-analysis warning: Add mutex requirement for AddToCompactExtraTransactions(…). Use -Wthread-safety-analysis if available.
on Aug 19, 2017

practicalswift force-pushed on Aug 19, 2017

practicalswift renamed this:
~~Fix -Wthread-safety-analysis warning: Add mutex requirement for AddToCompactExtraTransactions(…). Use -Wthread-safety-analysis if available.~~
Fix -Wthread-safety-analysis warnings and enable if available. Build with --enable-werror under OS X on Travis.
on Aug 19, 2017

practicalswift commented at 5:07 pm on August 19, 2017: contributor

@TheBlueMatt Does this PR look good now? Please check my attempt to summarize your changes in the commit message for https://github.com/bitcoin/bitcoin/pull/10866/commits/695fe315e8c4191bd5e9e98a8ce6d2be8c6aff96. Let me know if anything should be changed or if you’d rather squash your commits in your branch and have me cherry pick the resulting commit as-is :-)

laanwj referenced this in commit 2c9f5ecf3f on Aug 23, 2017

in .travis.yml:39 in eeee7c12d4 outdated

33@@ -34,7 +34,7 @@ env:
34 # x86_64 Linux, No wallet
35     - HOST=x86_64-unknown-linux-gnu PACKAGES="python3" DEP_OPTS="NO_WALLET=1" RUN_TESTS=true GOAL="install" BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports"
36 # Cross-Mac
37-    - HOST=x86_64-apple-darwin11 PACKAGES="cmake imagemagick libcap-dev librsvg2-bin libz-dev libbz2-dev libtiff-tools python-dev" BITCOIN_CONFIG="--enable-gui --enable-reduce-exports" OSX_SDK=10.11 GOAL="deploy"
38+    - HOST=x86_64-apple-darwin11 PACKAGES="cmake imagemagick libcap-dev librsvg2-bin libz-dev libbz2-dev libtiff-tools python-dev" BITCOIN_CONFIG="--enable-gui --enable-reduce-exports --enable-werror" OSX_SDK=10.11 GOAL="deploy"

MarcoFalke commented at 1:10 pm on August 23, 2017:

This change is already in master

practicalswift force-pushed on Aug 23, 2017

practicalswift renamed this:
~~Fix -Wthread-safety-analysis warnings and enable if available. Build with --enable-werror under OS X on Travis.~~
Fix -Wthread-safety-analysis warnings. Build with -Wthread-safety-analysis if available.
on Aug 23, 2017

practicalswift commented at 3:30 pm on August 23, 2017: contributor

@MarcoFalke Thanks! Rebased on top of master, so the already merged commit is now excluded from this PR :-)

practicalswift renamed this:
~~Fix -Wthread-safety-analysis warnings. Build with -Wthread-safety-analysis if available.~~
Fix -Wthread-safety-analysis warnings. Compile with -Wthread-safety-analysis if available.
on Aug 23, 2017

danra commented at 7:06 pm on August 30, 2017: contributor

@practicalswift NACK

Transaction time check was changed:

       checktxtime = boost::get_system_time() + boost::posix_time::minutes(1);

       checktxtime = std::chrono::steady_clock::now() + std::chrono::seconds(1);

IMO there are too many things going on in the first commit at once, making it hard to review and bug-prone. Switching the primitives in the sync module from boost to std should be done in an atomic commit, without doing any other change.

I didn’t know about this pull request before opening issue #11166 (from which you referred me here), I’m pretty close to having that ready to open a pull request, (just changing boost to std). Will update here when it’s ready. I think it’d be better if that were reviewed and merged, and only then follow up with other changes. EDIT: Opened pull request #11199

practicalswift commented at 7:42 am on September 1, 2017: contributor

@danra Are you referring to @TheBlueMatt:s commit f01ea9326bc69d4f731a5904881740308c5e4271?

This PR contains three commits. Please consider reviewing d77d9ccd1ad35f550c8dcdc3b83e65bad8c51c48 and 0c7115c56fdbe25257aee23ae002dc5af3238bb7 too :-)

practicalswift commented at 7:46 am on September 1, 2017: contributor

@sipa @TheBlueMatt @jonasschnelli @theuni I have some follow-up work which depends on this PR. Do you have time reviewing this PR?

The follow-up work is a PR adding GUARDED_BY(…) and EXCLUSIVE_LOCKS_REQUIRED(…) annotations where appropriate.

danra commented at 9:26 am on September 1, 2017: contributor

@practicalswift Yes, referring to f01ea93

practicalswift commented at 9:29 am on September 1, 2017: contributor

@TheBlueMatt Do you have any input on @danra:s review?

practicalswift commented at 5:04 pm on September 3, 2017: contributor

Please see #11226 for the follow-up PR which adds GUARDED_BY(…) and EXCLUSIVE_LOCKS_REQUIRED(…) annotations.

TheBlueMatt commented at 10:16 pm on September 4, 2017: member

@danra Indeed, the minutes -> seconds change was a mistake, should probably be fixed. Indeed, the first commit was originally several different commits, but they werent logically split up so it was easier to squash them than to manually re-split them. Personally I find the first commit not-too-hard to review cause the changes are pretty clearly along file boundaries, but then I also wrote it, so I cant speak….

utACK 0c7115c56fdbe25257aee23ae002dc5af3238bb7 (I went back and re-checked that nothing currently needing boost interruption support should be effected by this change).

practicalswift force-pushed on Sep 5, 2017

practicalswift commented at 6:17 am on September 5, 2017: contributor

@TheBlueMatt I’ve now added a commit fixing the minutes/seconds mix-up. Looks good? :-)

TheBlueMatt commented at 0:50 am on September 6, 2017: member

Yup, needs a squash though.

practicalswift force-pushed on Sep 6, 2017

practicalswift commented at 7:06 am on September 6, 2017: contributor

@TheBlueMatt Squashed! Looks good? :-)

practicalswift commented at 12:51 pm on September 15, 2017: contributor

Anyone willing to review? :-)

Having this PR merged would simplify the work on follow-up PR:s such as #11226.

TheBlueMatt commented at 7:18 pm on September 15, 2017: member

@practicalswift generally, when squashing, please try to avoid rebasing as it makes it much easier for reviewers to track which changes you made between two commits of a PR.

re-utACK b22210337da62047290fcb2658b2c45117c7efa9

in src/rpc/mining.cpp:484 in b22210337d outdated

478-            boost::unique_lock<boost::mutex> lock(csBestBlock);
479+            CWaitableLock lock(csBestBlock);
480             while (chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning())
481             {
482-                if (!cvBlockChange.timed_wait(lock, checktxtime))
483+                if (cvBlockChange.wait_until(lock, checktxtime) == std::cv_status::timeout)

theuni commented at 9:19 pm on September 15, 2017:

cvBlockChange needs to be notified in Interrupt() in init, otherwise we’ll hang here at shutdown.

practicalswift commented at 6:51 pm on September 19, 2017:

@theuni Something like this? :-)

 0diff --git a/src/init.cpp b/src/init.cpp
 1index 25b8caf..73f4860 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -167,6 +167,7 @@ void Interrupt(boost::thread_group& threadGroup)
 5     if (g_connman)
 6         g_connman->Interrupt();
 7     threadGroup.interrupt_all();
 8+    cvBlockChange.notify_all();
 9 }
10
11 void Shutdown()

theuni commented at 7:06 pm on September 19, 2017:

Looking deeper, I think this is actually ok (though circuitous) as-is.

0StopRPC() -> g_rpcSignals.Stopped() -> OnRPCStopped() -> cvBlockChange.notify_all()

So, nevermind my comment.

danra commented at 9:28 pm on September 15, 2017: contributor

@TheBlueMatt

Personally I find the first commit not-too-hard to review cause the changes are pretty clearly along file boundaries, but then I also wrote it, so I cant speak….

IMHO the fact that there were a few utACKs before fixing the minutes-seconds bug demonstrates that the commit was doing too many things at once to review easily.

So it’s still a NACK from me until it’s split to smaller commits. Of course, you’re welcome to use my own PR #11199 which does just the move from boost to std locks part.

theuni commented at 9:37 pm on September 15, 2017: member

utACK ~~other than my comment above~~. I think the commits here are ok. Either way, with #11199 or this one, we’re going to need some follow-ups to tidy up afterwards.

practicalswift commented at 9:29 pm on September 19, 2017: contributor

@TheBlueMatt Thanks for the utACK. I’ll avoid rebasing when squashing going forward. Thanks for the suggestion! @theuni Thanks for the utACK.

Anyone else who has time to review? Having this merged would help my annotations work a lot :-)

in src/sync.h:110 in f3c4005aa8 outdated

114 #ifdef DEBUG_LOCKCONTENTION
115 void PrintLockContention(const char* pszName, const char* pszFile, int nLine);
116 #endif
117 
118 /** Wrapper around boost::unique_lock<Mutex> */
119-template <typename Mutex>

ryanofsky commented at 9:01 pm on October 6, 2017:

Comment above still mentions boost.

in src/sync.h:118 in f3c4005aa8 outdated

120-class SCOPED_LOCKABLE CMutexLock
121+class SCOPED_LOCKABLE CCriticalBlock
122 {
123 private:
124-    boost::unique_lock<Mutex> lock;
125+    std::unique_lock<CCriticalSection> lock;

ryanofsky commented at 9:02 pm on October 6, 2017:

In commit “Fix -Wthread-safety-analysis warnings.”

Comment above still mentions boost.

in src/sync.h:115 in f3c4005aa8 outdated

116 #endif
117 
118 /** Wrapper around boost::unique_lock<Mutex> */
119-template <typename Mutex>
120-class SCOPED_LOCKABLE CMutexLock
121+class SCOPED_LOCKABLE CCriticalBlock

ryanofsky commented at 9:17 pm on October 6, 2017:

In commit “Fix -Wthread-safety-analysis warnings.”

The naming here doesn’t seem very consistent:

Recursive mutex: CCriticalSection (unchanged) Recursive lock: CCriticalBlock (previously CMutexLock) Non-recursive mutex: CWaitableCriticalSection (unchanged) Non-recursive lock: CWaitableLock (previously didn’t exist)

I don’t see why we wouldn’t just follow the standard naming and our current coding convention and use:

Recursive mutex: RecursiveMutex Recursive lock: RecursiveLock Non-recursive mutex: Mutex Non-recursive lock: Lock

Maybe defining backwards compatible CCriticalSection and CWaitableCriticalSection typedefs to avoid increasing the size of the diff.

ryanofsky commented at 9:33 pm on October 6, 2017: member

utACK b22210337da62047290fcb2658b2c45117c7efa9. I agree with others in not seeing a pressing need to split up the first commit since it is only ~20 lines, even though it would be a little nicer split up.

practicalswift commented at 2:27 pm on October 9, 2017: contributor

@ryanofsky Thanks for the review. I’ve now added two commits addressing the two issues raised. Additionally I found a boost include that is no longer needed. Would you mind re-reviewing and confirming that your feedback was properly addressed? :-)

ryanofsky commented at 3:24 pm on October 9, 2017: member

utACK 7ab7662dc067951fc9fd1364bd6d1446c8770305

Thanks for cleanup. You could consider combining “Change the sync.h primitives” and “Use consistent naming” commits because the combined diff would be smaller, and having two renames sets of in one PR is a little confusing.

practicalswift force-pushed on Oct 9, 2017

practicalswift commented at 7:23 pm on October 9, 2017: contributor

@ryanofsky Good point regarding combining the related commits. Could you please re-review? :-)

theuni commented at 7:36 pm on October 9, 2017: member

utACK 7ab7662dc067951fc9fd1364bd6d1446c8770305. I suspect that we may run into symbol collisions at some point by using the generic “Mutex” name, but it’ll be obvious if that happens.

ryanofsky commented at 8:38 pm on October 9, 2017: member

utACK 0a5c7596567d3d1d0296cf831057c79454f7851d (just squashed 7ab7662dc067951fc9fd1364bd6d1446c8770305)

in src/init.cpp:547 in b9011c9f6d outdated

544@@ -545,14 +545,14 @@ static void BlockNotifyCallback(bool initialSync, const CBlockIndex *pBlockIndex
545 }
546 
547 static bool fHaveGenesis = false;
548-static boost::mutex cs_GenesisWait;
549+static CWaitableCriticalSection cs_GenesisWait;

ryanofsky commented at 8:41 pm on October 9, 2017:

Could s/CWaitableCriticalSection/Mutex

TheBlueMatt commented at 5:09 pm on November 1, 2017:

Hmm, that feels weird? We now have CCriticalSection and Mutex which mean two different things, but its not clear /what/ they mean without looking at what they’re typedef’d to? Can we just do this as a scripted diff in a separate commit?

ryanofsky commented at 12:19 pm on November 3, 2017:

Hmm, that feels weird? We now have CCriticalSection and Mutex which mean two different things, but its not clear /what/ they mean without looking at what they’re typedef’d to? Can we just do this as a scripted diff in a separate commit?

Scripted-diff renaming followup is at https://github.com/bitcoin/bitcoin/pull/11599

in src/sync.h:114 in b9011c9f6d outdated

119 #endif
120 
121-/** Wrapper around boost::unique_lock<Mutex> */
122-template <typename Mutex>
123-class SCOPED_LOCKABLE CMutexLock
124+/** Wrapper around std::unique_lock<Mutex> */

ryanofsky commented at 8:42 pm on October 9, 2017:

Comment is a little misleading since it’s a wrapper around std::unique_lock<RecursiveMutex>

ryanofsky commented at 8:45 pm on October 9, 2017: member

Two minor comments (feel free to ignore)

practicalswift force-pushed on Oct 9, 2017

practicalswift commented at 8:56 pm on October 9, 2017: contributor

@ryanofsky Thanks for re-reviewing! Feedback address. Would you mind re-reviewing again? :-)

ryanofsky commented at 9:02 pm on October 9, 2017: member

It looks like you accidentally squashed the two changes suggested above into the second commit (“Add mutex requirement…”) instead of the first commit.

Anyway, utACK 0f5341f644796ff88234fa1179012f1cedae5a76. Only difference is the two changes.

practicalswift force-pushed on Oct 9, 2017

practicalswift commented at 9:09 pm on October 9, 2017: contributor

@ryanofsky Sorry, fixed. Would you mind re-reviewing? :-)

ryanofsky commented at 9:12 pm on October 9, 2017: member

utACK 94568c8f22e5969fd3abfca1c2afa7b80a05cf9a. No changes other than rearranged commits

fanquake added this to the "In progress" column in a project

TheBlueMatt commented at 5:12 pm on November 1, 2017: member

Not sure how I feel about renaming CMutexLock type in the same commit that changes a bunch of stuff from boost -> std, let alone in a commit people were complaining was already too large.

practicalswift commented at 9:42 am on November 2, 2017: contributor

@TheBlueMatt Should I revert it by doing a s/RecursiveLock/CMutexLock/ and squash, or should I keep the rename but put it in a separate commit? Please advice :-)

ryanofsky commented at 9:58 am on November 2, 2017: member

CMutexLock is only mentioned in 5 lines of code, all of them in sync.h. Sometimes it’s nice to make changes in separate commits, but I think this particular rename makes more sense in context, and doesn’t seem make the PR more complicated to review. But maybe I’m missing something because this entire change is only 47 lines and seems pretty straightforward to me.

practicalswift commented at 5:10 pm on November 2, 2017: contributor

@TheBlueMatt @ryanofsky I’ll let you guys find consensus. I’ll happily adjust :-)

TheBlueMatt commented at 7:57 pm on November 2, 2017: member

@ryanofsky in this case the PR adds a compatibility typedef to allow places to keep using CCriticalSection, renaming the underlying type and then mapping the old typename to the new one. I agree it should be renamed, but we should actually rename it instead of halfway-renaming it (using scripted-diff =D).

ryanofsky commented at 8:12 pm on November 2, 2017: member

Agree scripted diff would improve upon the typedef.

TheBlueMatt commented at 8:15 pm on November 2, 2017: member

In the interest of getting this very-important PR merged sooner rather than later, can we skip it for now?

ryanofsky commented at 8:17 pm on November 2, 2017: member

SGTM

theuni commented at 8:25 pm on November 2, 2017: member

+1

practicalswift force-pushed on Nov 2, 2017

practicalswift commented at 10:10 pm on November 2, 2017: contributor

Please review the latest commit added - does that revert the rename as suggested? Everyone happy? :-)

ryanofsky commented at 10:20 pm on November 2, 2017: member

@practicalswift, ~~~I think you can drop the “Revert CMutexLock rename” commit.~~~ (EDIT: Actually should be kept and squashed)

What Matt is asking you to do is remove the two typedefs:

0+typedef RecursiveMutex CCriticalSection;
1+typedef Mutex CWaitableCriticalSection

Which will require replacing RecursiveMutex to CCriticalSection and Mutex to CWaitableCriticalSection. You can squash these changes into the “Fix -Wthread-safety-analysis warnings” commit, which should make it smaller.

practicalswift force-pushed on Nov 2, 2017

practicalswift commented at 10:56 pm on November 2, 2017: contributor

@ryanofsky Could you take a look at Revert [wip]? :-)

ryanofsky commented at 11:07 pm on November 2, 2017: member

That looks close. Here is another commit that builds on top of yours and gets rid of more unnecessary renames & diffs: 17ebad7b47a2567a75e5a701780a261bf1a3275d (I was wrong above when I asked you to drop CMutexLock commit). If you squash both of these into “Fix -Wthread-safety-analysis warnings” and edit the commit message to remove mention of renames, this should be good to go.

ryanofsky commented at 11:19 pm on November 2, 2017: member

Here is a combined branch: https://github.com/ryanofsky/bitcoin/commits/pr/locks

You can fetch & apply it with

0git fetch -n https://github.com/ryanofsky/bitcoin pr/locks:ryanofsky-locks
1git reset --hard ryanofsky-locks

practicalswift force-pushed on Nov 3, 2017

practicalswift commented at 0:12 am on November 3, 2017: contributor

@ryanofsky Looks good now? :-)

practicalswift force-pushed on Nov 3, 2017

ryanofsky commented at 0:55 am on November 3, 2017: member

Looks good! utACK 1864bea78ae306e91dde46fcc2b85a01999efd84

in src/sync.h:108 in 1864bea78a outdated

109-typedef boost::condition_variable CConditionVariable;
110+/** Just a typedef for std::condition_variable, can be wrapped later if desired */
111+typedef std::condition_variable CConditionVariable;
112+
113+/** Just a typedef for std::unique_lock, can be wrapped later if desired */
114+typedef std::unique_lock<std::mutex> Lock;

TheBlueMatt commented at 9:17 pm on November 3, 2017:

I’d really rather we not call this a “Lock” - its not intended to be the thing used across the codebase as any lock (though we could maybe do so if we add DEBUG_LOCKORDER support here).

ryanofsky commented at 9:27 pm on November 3, 2017:

You always say what you don’t want instead of saying what you do want ;). What would you like instead? If you’ve seen #11599, the name should make more sense in this context. I think Lock and Mutex are the right names because they match the standard. As you mentioned, you can implement DEBUG_LOCKORDER with them and also do other things useful for debugging, like printing differences in a data structure between time a particular mutex is locked & unlocked.

TheBlueMatt commented at 9:36 pm on November 3, 2017:

The original name (CWaitableCriticalSection) was nice and specific - I’m happy to make it more in-line with the more-accepted “lock” terminology, but pointing out that it has only a narrow use-case in our codebase is important.

ryanofsky commented at 9:48 pm on November 3, 2017:

Ok, but I don’t think non-recursive locks should have a narrow usecase in our codebase. I thought everybody agreed non-recursivelocks should be preferred to recursive locks.

ryanofsky commented at 9:49 pm on November 3, 2017:

Also CWaitableCriticalSection was not the original name for this. There was no original name for this. Previously code just used boost::unique_lock directly.

TheBlueMatt commented at 9:51 pm on November 3, 2017:

Heh, I meant in the original patch in this PR. I mean we use locks in lots of different ways, for new subsystems that have very careful locking models, obviously non-recursive locks are preferred, but I’m skeptical every new thing we add will use them.

practicalswift commented at 9:53 pm on November 3, 2017:

@ryanofsky @TheBlueMatt Could you provide a diff with your suggested change?

ryanofsky commented at 9:59 pm on November 3, 2017:

I think Matt basically wants the typedef to be called ScaryLock instead of Lock. But I’m fine with whatever is proposed here given there is already a PR open for renaming.

practicalswift commented at 11:07 am on November 4, 2017:

OK, I’ll fix. @TheBlueMatt, what is your suggested name instead of Lock? :-)

TheBlueMatt commented at 6:58 pm on November 4, 2017:

I was fine with the WaitableCriticalSection (or WaitableLock or whatever) version that was there originally.

practicalswift commented at 12:26 pm on November 6, 2017:

@TheBlueMatt @ryanofsky The following diff would work?

 0diff --git a/src/init.cpp b/src/init.cpp
 1index dd64602..9e3eb6b 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -551,7 +551,7 @@ static void BlockNotifyGenesisWait(bool, const CBlockIndex *pBlockIndex)
 5 {
 6     if (pBlockIndex != nullptr) {
 7         {
 8-            Lock lock_GenesisWait(cs_GenesisWait);
 9+            WaitableLock lock_GenesisWait(cs_GenesisWait);
10             fHaveGenesis = true;
11         }
12         condvar_GenesisWait.notify_all();
13@@ -1634,7 +1634,7 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
14
15     // Wait for genesis block to be processed
16     {
17-        Lock lock(cs_GenesisWait);
18+        WaitableLock lock(cs_GenesisWait);
19         while (!fHaveGenesis) {
20             condvar_GenesisWait.wait(lock);
21         }
22diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
23index 8fae01b..0ba0e96 100644
24--- a/src/rpc/mining.cpp
25+++ b/src/rpc/mining.cpp
26@@ -478,7 +478,7 @@ UniValue getblocktemplate(const JSONRPCRequest& request)
27         {
28             checktxtime = std::chrono::steady_clock::now() + std::chrono::minutes(1);
29
30-            Lock lock(csBestBlock);
31+            WaitableLock lock(csBestBlock);
32             while (chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning())
33             {
34                 if (cvBlockChange.wait_until(lock, checktxtime) == std::cv_status::timeout)
35diff --git a/src/sync.h b/src/sync.h
36index 66b6d96..20556af 100644
37--- a/src/sync.h
38+++ b/src/sync.h
39@@ -105,7 +105,7 @@ typedef AnnotatedMixin<std::mutex> CWaitableCriticalSection;
40 typedef std::condition_variable CConditionVariable;
41
42 /** Just a typedef for std::unique_lock, can be wrapped later if desired */
43-typedef std::unique_lock<std::mutex> Lock;
44+typedef std::unique_lock<std::mutex> WaitableLock;
45
46 #ifdef DEBUG_LOCKCONTENTION
47 void PrintLockContention(const char* pszName, const char* pszFile, int nLine);

ryanofsky commented at 12:58 pm on November 6, 2017:

@TheBlueMatt @ryanofsky The following diff would work?

Yes for me. (As I mentioned in my last comment I am happy with any naming in this pr since #11599 can clean up any inconsistencies.)

practicalswift commented at 1:02 pm on November 6, 2017:

@ryanofsky Excellent! OK with you @TheBlueMatt? :-)

TheBlueMatt commented at 4:38 pm on November 6, 2017:

Yea, looks good, thanks!

Fix -Wthread-safety-analysis warnings. Change the sync.h primitives to std from boost.

Commit 1.

This code was written by @TheBlueMatt in the following branch:
* https://github.com/TheBlueMatt/bitcoin/commits/2017-08-test-10923

This commit message was written by me (@practicalswift) who also squashed
@TheBlueMatt's commits into one and tried to summarize the changes made.

Commit 2.

Remove boost include. Remove boost mentions in comments.

7e319d6393

Use -Wthread-safety-analysis if available (+ -Werror=thread-safety-analysis if --enable-werror) 4616c825a4

Add mutex requirement for AddToCompactExtraTransactions(…)

The vector `vExtraTxnForCompact`, which is guarded by the mutex
`cs_main`, is accessed in `AddToCompactExtraTransactions(…)`.

76ea17c796

practicalswift force-pushed on Nov 6, 2017

practicalswift commented at 4:43 pm on November 6, 2017: contributor

@ryanofsky @TheBlueMatt Name change patch applied. Squashed and pushed. Could you please re-review? :-)

TheBlueMatt commented at 4:52 pm on November 6, 2017: member

utACK 76ea17c7964c15dd90e10c2c257cdeb5847b3d69

ryanofsky commented at 4:52 pm on November 6, 2017: member

utACK 76ea17c7964c15dd90e10c2c257cdeb5847b3d69, confirmed only change since last review is name change.

sipa commented at 6:20 pm on November 7, 2017: member

utACK 76ea17c7964c15dd90e10c2c257cdeb5847b3d69. Nice to see some further unboostification.

sipa merged this on Nov 7, 2017

sipa closed this on Nov 7, 2017

sipa referenced this in commit ef8a634358 on Nov 7, 2017

fanquake moved this from the "In progress" to the "Done" column in a project

PastaPastaPasta referenced this in commit fe6057f91e on Jan 31, 2020

PastaPastaPasta referenced this in commit 6a8ad24e8c on Jan 31, 2020

PastaPastaPasta referenced this in commit 9c33eef310 on Feb 4, 2020

PastaPastaPasta referenced this in commit 06c90e5a83 on Feb 9, 2020

MarcoFalke referenced this in commit 7027c67cac on Jul 2, 2020

ckti referenced this in commit 91d6640b44 on Mar 28, 2021

practicalswift deleted the branch on Apr 10, 2021

gades referenced this in commit 383c9c9c33 on Jun 28, 2021

vijaydasmp referenced this in commit dfa262c93f on Oct 4, 2021

gades referenced this in commit ebc82c18e6 on Feb 23, 2022

DrahtBot locked this on Aug 16, 2022

Fix -Wthread-safety-analysis warnings. Compile with -Wthread-safety-analysis if available. #10866