util: Add Clang thread safety annotations for variables guarded by cs_args #13126

practicalswift · 2018-04-30T09:32:24Z

Add missing cs_args locks
Add Clang thread safety annotations for variables guarded by cs_args

practicalswift · 2018-05-12T20:31:35Z

Now includes a fix for a missing cs_args lock introduced in the recently merged PR #10267.

practicalswift · 2018-06-06T06:07:31Z

Rebased!

DrahtBot · 2018-07-29T15:16:46Z

The last travis run for this pull request was 53 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

maflcko · 2018-07-29T15:32:52Z

src/util.cpp

-        if (m_override_args.count("-includeconf") == 0) {
+        bool emptyIncludeConf;
+        {
+            LOCK(cs_args);


ReadConfigFiles is done once on start. Couldn't you just take the lock at the beginning of the method and remove those scoped locks further down in the method?

This change caused the deadlock. Now reverted to previous version :-)

maflcko · 2018-07-29T15:34:45Z

src/util.cpp

@@ -387,6 +388,7 @@ void ArgsManager::WarnForSectionOnlyArgs()
    // if it's okay to use the default section for this network, don't worry
    if (m_network == CBaseChainParams::MAIN) return;

+    LOCK(cs_args);


Just take the lock in the very first line of this method, or is there a specific reason that m_network shouldn't be protected?

practicalswift · 2018-08-27T16:52:25Z

@MarcoFalke Thanks for reviewing! Feedback addressed. Please re-review :-)

practicalswift · 2018-08-27T17:09:02Z

Added GUARDED_BY(cs_args) to m_available_args and m_network_only_args too :-)

maflcko · 2018-08-27T18:51:01Z

src/util.cpp

@@ -304,6 +304,7 @@ class ArgsManagerHelper {
    static inline bool GetNetBoolArg(const ArgsManager &am, const std::string& net_arg)
    {
        std::pair<bool,std::string> found_result(false,std::string());
+        LOCK(am.cs_args);


I'd prefer to annotate this function and then take the lock where GetNetBoolArg is called (in GetChainName)

maflcko · 2018-08-27T18:52:05Z

src/util.cpp

@@ -886,7 +891,8 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
        }
        // if there is an -includeconf in the override args, but it is empty, that means the user
        // passed '-noincludeconf' on the command line, in which case we should not include anything
-        if (m_override_args.count("-includeconf") == 0) {
+        bool emptyIncludeConf = m_override_args.count("-includeconf") == 0;
+        if (emptyIncludeConf) {


Why this change?

Oh, that was likely a left-over from a previous commit - a separate variable was needed to limit the scope of the lock. When the lock was moved higher up that was no longer needed but the variable was accidentally left around.

maflcko · 2018-08-27T18:52:56Z

utACK, but could squash into two commits (1) add missing locks and (2) add lock annotations

practicalswift · 2018-08-27T21:23:58Z

@MarcoFalke Good points. PR adjusted accordingly. Please re-review :-)

maflcko · 2018-08-29T18:50:17Z

POTENTIAL DEADLOCK DETECTED
Previous lock order was:
 (1)cs_args  util.cpp:874 (2)csPathCached  util.cpp:767
Current lock order is:
 (2)csPathCached  util.cpp:767 (1)cs_args  util.cpp:508

practicalswift · 2018-08-29T18:56:57Z

@MarcoFalke Thanks! I'll investigate immediately!

maflcko · 2018-08-29T19:27:42Z

Note that I reproduced this with --enable-debug in ./configure

practicalswift · 2018-08-29T19:30:34Z

@MarcoFalke Strange! I always build with --enable-debug. Triggered by make check?

maflcko · 2018-08-29T19:36:36Z

No, triggered by bitcoind or bitcoin-qt. It happens early on when the args and data dir are read.

maflcko · 2018-08-29T20:20:19Z

src/util.cpp

-    m_config_args.clear();
+    {
+        LOCK(cs_args);
+        m_config_args.clear();


Couldn't you just move those two lines after the GetConfigFile(); call? (And remove all scopes and re-locks?)

practicalswift · 2018-08-29T20:40:38Z

@MarcoFalke Please re-review :-)

maflcko · 2018-08-29T21:23:27Z

This still fails travis for the same reason. Sorry, I guess my suggestion doesn't work.

practicalswift · 2018-08-30T08:17:43Z

@MarcoFalke Reverted again :-) Travis is now happy. Please re-review!

… guarded by cs_args 1e29379 Fix potential deadlock (practicalswift) d58dc9f Add lock annotations (cs_args) (practicalswift) db5e9d3 Add missing locks (cs_args) (practicalswift) Pull request description: * Add missing `cs_args` locks * Add Clang thread safety annotations for variables guarded by `cs_args` Tree-SHA512: bc562dbddf24a287bcf9bf902bc930f942f260a94e5c8ec4d190f7f2ddac448ed3d52acadaf9fc1c81a5cbff2c171c52c18ba0804eeb03f699d70394e1c977c5

… guarded by cs_args Summary: 1e29379d69 Fix potential deadlock (practicalswift) d58dc9f943 Add lock annotations (cs_args) (practicalswift) db5e9d3c88 Add missing locks (cs_args) (practicalswift) Pull request description: * Add missing `cs_args` locks * Add Clang thread safety annotations for variables guarded by `cs_args` Tree-SHA512: bc562dbddf24a287bcf9bf902bc930f942f260a94e5c8ec4d190f7f2ddac448ed3d52acadaf9fc1c81a5cbff2c171c52c18ba0804eeb03f699d70394e1c977c5 Backport of Core PR13126 bitcoin/bitcoin#13126 Completes T652 Test Plan: cmake -GNinja .. -DENABLE_WERROR=ON ninja check Also run `build-werror` and `build-tsan` builds. Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc, markblundeberg Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Subscribers: markblundeberg Differential Revision: https://reviews.bitcoinabc.org/D4965

…riables guarded by cs_args 1e29379 Fix potential deadlock (practicalswift) d58dc9f Add lock annotations (cs_args) (practicalswift) db5e9d3 Add missing locks (cs_args) (practicalswift) Pull request description: * Add missing `cs_args` locks * Add Clang thread safety annotations for variables guarded by `cs_args` Tree-SHA512: bc562dbddf24a287bcf9bf902bc930f942f260a94e5c8ec4d190f7f2ddac448ed3d52acadaf9fc1c81a5cbff2c171c52c18ba0804eeb03f699d70394e1c977c5 # Conflicts: # src/util/system.cpp

fanquake added the Utils/log/libs label Apr 30, 2018

practicalswift mentioned this pull request Apr 30, 2018

Meta-issue: Add Clang thread safety analysis annotations #13129

Closed

30 tasks

practicalswift force-pushed the guarded-by-cs_args branch from 05d6e62 to 83b6e7c Compare May 12, 2018 20:10

practicalswift mentioned this pull request May 12, 2018

New -includeconf argument for including external configuration files #10267

Merged

practicalswift force-pushed the guarded-by-cs_args branch from 83b6e7c to 02616c4 Compare May 14, 2018 12:18

maflcko added the Needs rebase label Jun 6, 2018

practicalswift force-pushed the guarded-by-cs_args branch from 02616c4 to 7f1cc8f Compare June 6, 2018 06:07

maflcko removed the Needs rebase label Jun 6, 2018

DrahtBot closed this Jul 29, 2018

DrahtBot reopened this Jul 29, 2018

maflcko reviewed Jul 29, 2018

View reviewed changes

practicalswift force-pushed the guarded-by-cs_args branch from 7f1cc8f to f4d0f5c Compare August 27, 2018 16:51

DrahtBot mentioned this pull request Aug 27, 2018

refactor: Fix the chainparamsbase -> util circular dependency #14045

Closed

maflcko reviewed Aug 27, 2018

View reviewed changes

practicalswift force-pushed the guarded-by-cs_args branch from d0d69b1 to 26add58 Compare August 27, 2018 21:22

Add missing locks (cs_args)

db5e9d3

practicalswift force-pushed the guarded-by-cs_args branch from 26add58 to a8fac9f Compare August 29, 2018 18:57

practicalswift force-pushed the guarded-by-cs_args branch from 0b1506a to 8c747a4 Compare August 29, 2018 20:16

maflcko reviewed Aug 29, 2018

View reviewed changes

practicalswift force-pushed the guarded-by-cs_args branch 2 times, most recently from b16fb00 to 2d326da Compare August 29, 2018 20:31

Add lock annotations (cs_args)

d58dc9f

practicalswift force-pushed the guarded-by-cs_args branch 3 times, most recently from 111354a to e492de8 Compare August 29, 2018 20:39

Fix potential deadlock

1e29379

practicalswift force-pushed the guarded-by-cs_args branch from e492de8 to 1e29379 Compare August 30, 2018 08:03

maflcko merged commit 1e29379 into bitcoin:master Aug 30, 2018

Bushstar mentioned this pull request Sep 4, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#359

Merged

practicalswift deleted the guarded-by-cs_args branch April 10, 2021 19:35

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

util: Add Clang thread safety annotations for variables guarded by cs_args #13126

util: Add Clang thread safety annotations for variables guarded by cs_args #13126

Uh oh!

Conversation

practicalswift commented Apr 30, 2018 • edited by DrahtBot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented May 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Jun 6, 2018

Uh oh!

DrahtBot commented Jul 29, 2018

Uh oh!

maflcko Jul 29, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift Aug 29, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 29, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Aug 27, 2018

Uh oh!

practicalswift commented Aug 27, 2018

Uh oh!

maflcko Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 27, 2018

Uh oh!

practicalswift commented Aug 27, 2018

Uh oh!

maflcko commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Aug 29, 2018

Uh oh!

maflcko commented Aug 29, 2018

Uh oh!

practicalswift commented Aug 29, 2018

Uh oh!

maflcko commented Aug 29, 2018

Uh oh!

maflcko Aug 29, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Aug 29, 2018

Uh oh!

maflcko commented Aug 29, 2018

Uh oh!

practicalswift commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Apr 30, 2018 •

edited by DrahtBot

Loading

practicalswift commented May 12, 2018 •

edited

Loading

maflcko commented Aug 29, 2018 •

edited

Loading

practicalswift commented Aug 30, 2018 •

edited

Loading