Add clang-tidy check for thread

theuni commented at 4:08 pm on May 21, 2024: member

Forbid thread_local vars with non-trivial destructors.

This is a follow-up from: #30095 (review)

DrahtBot commented at 4:08 pm on May 21, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, maflcko
Concept ACK	laanwj, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

in src/init.cpp:723 in d580e88d30 outdated

719@@ -720,6 +720,7 @@ static bool AppInitServers(NodeContext& node)
720 // Parameter interaction based on rules
721 void InitParameterInteraction(ArgsManager& args)
722 {
723+    thread_local std:string bad_var;

maflcko commented at 4:19 pm on May 21, 2024:

0init.cpp:723:21: error: unexpected ':' in nested name specifier; did you mean '::'?

theuni commented at 4:22 pm on May 21, 2024:

Hah

theuni force-pushed on May 21, 2024

laanwj commented at 5:31 pm on May 21, 2024: member

Concept ACK

theuni commented at 5:41 pm on May 21, 2024: member

This PR originally contained a commit which purposely added a violation in init.cpp:

0thread_local std::string bad_var;

From the tidy c-i task:

init.cpp:723:5: error: Variable with non-trivial destructor cannot be thread_local. [bitcoin-nontrivial-threadlocal,-warnings-as-errors] 723 | thread_local std::string bad_var;

Since it was correctly detected, I’ll now remove that commit.

theuni force-pushed on May 21, 2024

DrahtBot added the label CI failed on May 21, 2024

DrahtBot commented at 5:41 pm on May 21, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/25236805741

hebasto commented at 7:14 pm on May 21, 2024: member

Concept ACK.

DrahtBot removed the label CI failed on May 21, 2024

in contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.cpp:14 in ace5b35fb4 outdated

 9+
10+
11+// Copied from clang-tidy's bugprone check
12+namespace {
13+AST_MATCHER(clang::CXXRecordDecl, hasNonTrivialDestructor) {
14+  // TODO: If the dtor is there but empty we don't want to warn either.

TheCharlatan commented at 9:21 am on May 22, 2024:

How about:

 0diff --git a/contrib/devtools/bitcoin-tidy/example_nontrivial-threadlocal.cpp b/contrib/devtools/bitcoin-tidy/example_nontrivial-threadlocal.cpp
 1index 2b74df5d0e..b002712baf 100644
 2--- a/contrib/devtools/bitcoin-tidy/example_nontrivial-threadlocal.cpp
 3+++ b/contrib/devtools/bitcoin-tidy/example_nontrivial-threadlocal.cpp
 4@@ -2,0 +3,30 @@ thread_local std::string foo;
 5+thread_local char* bar;
 6+
 7+struct Test_Ok_1 {
 8+    int member;
 9+    Test_Ok_1() {
10+        member = 1;
11+    }
12+    ~Test_Ok_1() {}
13+};
14+
15+struct Test_Ok_2 {
16+    int member;
17+    Test_Ok_2() {
18+        member = 1;
19+    }
20+};
21+
22+struct Test_Bad {
23+    int member;
24+    Test_Bad() {
25+        member = 1;
26+    }
27+    ~Test_Bad() {
28+        member = 0;
29+    }
30+};
31+
32+thread_local Test_Ok_1 test_1;
33+thread_local Test_Ok_2 test_2;
34+thread_local Test_Bad test_3;
35diff --git a/contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.cpp b/contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.cpp
36index bce1a99165..3f5a85e444 100644
37--- a/contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.cpp
38+++ b/contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.cpp
39@@ -14,2 +14,19 @@ AST_MATCHER(clang::CXXRecordDecl, hasNonTrivialDestructor) {
40-  // TODO: If the dtor is there but empty we don't want to warn either.
41-  return Node.hasDefinition() && Node.hasNonTrivialDestructor();
42+  if (!Node.hasDefinition()) {
43+    return false;
44+  }
45+  if (Node.hasTrivialDestructor()) {
46+    return false;
47+  }
48+  const clang::CXXDestructorDecl* dtor = Node.getDestructor();
49+  if (!dtor) {
50+    return false;
51+  }
52+
53+  if (dtor->hasBody()) {
54+    const clang::Stmt* body = dtor->getBody();
55+    if (const clang::CompoundStmt* comp_stmt = clang::dyn_cast<clang::CompoundStmt>(body)) {
56+      return !comp_stmt->body_empty();
57+    }
58+  }
59+
60+  return false;

maflcko commented at 9:47 am on May 22, 2024:

I don’t have freeBSD to test this, but are you sure it doesn’t crash when the dtor is there but empty?

In any case, what is the point of an empty dtor? Might as well remove it, or modernize-use-equals-default it?

theuni commented at 1:06 pm on May 22, 2024:

@TheCharlatan Thanks for playing around with it :)

I don’t have the internal clang knowledge to know if that’s correct or not. Looks fine to me, but I agree with @maflcko that we could just remove the empty dtor in that case.

Also, to be clear, that TODO is an upstream comment, not one from me: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.cpp#L18

TheCharlatan commented at 1:33 pm on May 22, 2024:

I don’t have freeBSD to test this, but are you sure it doesn’t crash when the dtor is there but empty?

It looks like it should work to me. If a method has a body it should also contain a compound statement, or at least this book seems to suggest as much.

Also, to be clear, that TODO is an upstream comment, not one from me: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.cpp#L18

…but if it is an upstream TODO I might well be missing something.

maflcko commented at 2:02 pm on May 22, 2024:

I don’t have freeBSD to test this, but are you sure it doesn’t crash when the dtor is there but empty?

It looks like it should work to me. If a method has a body it should also contain a compound statement, or at least this book seems to suggest as much.

Oh, I didn’t mean clang-tidy to crash, but the program that uses thread_local in reference to https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701

maflcko commented at 9:28 am on July 8, 2024:

modernize-use-equals-default

See https://github.com/bitcoin/bitcoin/pull/30406

in contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.cpp:15 in ace5b35fb4 outdated

10+
11+// Copied from clang-tidy's bugprone check
12+namespace {
13+AST_MATCHER(clang::CXXRecordDecl, hasNonTrivialDestructor) {
14+  // TODO: If the dtor is there but empty we don't want to warn either.
15+  return Node.hasDefinition() && Node.hasNonTrivialDestructor();

TheCharlatan commented at 1:37 pm on May 22, 2024:

Nit: Four spaces for indents like in the rest of the code?

TheCharlatan approved

TheCharlatan commented at 1:38 pm on May 22, 2024: contributor

ACK ace5b35fb435ca2646bb8738e6578187ae2e8720

DrahtBot requested review from laanwj on May 22, 2024

DrahtBot requested review from hebasto on May 22, 2024

clang-tidy: add check for non-trivial thread_local vars

Do not allow thread_local vars with non-trivial destructors

34c9cee380

theuni force-pushed on May 22, 2024

theuni commented at 1:48 pm on May 22, 2024: member

Updated to resolve @TheCharlatan’s nit, and also specified where hasNonTrivialDestructor came from.

TheCharlatan approved

TheCharlatan commented at 1:49 pm on May 22, 2024: contributor

Re-ACK 34c9cee380e7276cf3f85f2ce56a293e57afd676

DrahtBot added the label CI failed on Jul 7, 2024

maflcko commented at 8:41 am on July 8, 2024: member

ACK 34c9cee380e7276cf3f85f2ce56a293e57afd676

DrahtBot removed the label CI failed on Jul 9, 2024

fanquake merged this on Jul 11, 2024

fanquake closed this on Jul 11, 2024

Add clang-tidy check for thread_local vars #30146

Code Coverage

Reviews