-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add clang-tidy check for thread_local vars #30146
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
d580e88
to
c72ac30
Compare
Concept ACK |
This PR originally contained a commit which purposely added a violation in init.cpp: thread_local std::string bad_var; From the tidy c-i task:
Since it was correctly detected, I'll now remove that commit. |
c72ac30
to
ace5b35
Compare
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
Concept ACK. |
// Copied from clang-tidy's bugprone check | ||
namespace { | ||
AST_MATCHER(clang::CXXRecordDecl, hasNonTrivialDestructor) { | ||
// TODO: If the dtor is there but empty we don't want to warn either. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
diff --git a/contrib/devtools/bitcoin-tidy/example_nontrivial-threadlocal.cpp b/contrib/devtools/bitcoin-tidy/example_nontrivial-threadlocal.cpp
index 2b74df5d0e..b002712baf 100644
--- a/contrib/devtools/bitcoin-tidy/example_nontrivial-threadlocal.cpp
+++ b/contrib/devtools/bitcoin-tidy/example_nontrivial-threadlocal.cpp
@@ -2,0 +3,30 @@ thread_local std::string foo;
+thread_local char* bar;
+
+struct Test_Ok_1 {
+ int member;
+ Test_Ok_1() {
+ member = 1;
+ }
+ ~Test_Ok_1() {}
+};
+
+struct Test_Ok_2 {
+ int member;
+ Test_Ok_2() {
+ member = 1;
+ }
+};
+
+struct Test_Bad {
+ int member;
+ Test_Bad() {
+ member = 1;
+ }
+ ~Test_Bad() {
+ member = 0;
+ }
+};
+
+thread_local Test_Ok_1 test_1;
+thread_local Test_Ok_2 test_2;
+thread_local Test_Bad test_3;
diff --git a/contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.cpp b/contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.cpp
index bce1a99165..3f5a85e444 100644
--- a/contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.cpp
+++ b/contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.cpp
@@ -14,2 +14,19 @@ AST_MATCHER(clang::CXXRecordDecl, hasNonTrivialDestructor) {
- // TODO: If the dtor is there but empty we don't want to warn either.
- return Node.hasDefinition() && Node.hasNonTrivialDestructor();
+ if (!Node.hasDefinition()) {
+ return false;
+ }
+ if (Node.hasTrivialDestructor()) {
+ return false;
+ }
+ const clang::CXXDestructorDecl* dtor = Node.getDestructor();
+ if (!dtor) {
+ return false;
+ }
+
+ if (dtor->hasBody()) {
+ const clang::Stmt* body = dtor->getBody();
+ if (const clang::CompoundStmt* comp_stmt = clang::dyn_cast<clang::CompoundStmt>(body)) {
+ return !comp_stmt->body_empty();
+ }
+ }
+
+ return false;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modernize-use-equals-default
See #30406
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK ace5b35
Do not allow thread_local vars with non-trivial destructors
ace5b35
to
34c9cee
Compare
Updated to resolve @TheCharlatan's nit, and also specified where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-ACK 34c9cee
ACK 34c9cee |
Forbid thread_local vars with non-trivial destructors.
This is a follow-up from: #30095 (comment)