Merge bitcoin/bitcoin#30146: Add clang-tidy check for thread_local vars

34c9cee380 clang-tidy: add check for non-trivial thread_local vars (Cory Fields)

Pull request description:

  Forbid thread_local vars with non-trivial destructors.

  This is a follow-up from: https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608423170

ACKs for top commit:
  maflcko:
    ACK 34c9cee380
  TheCharlatan:
    Re-ACK 34c9cee380

Tree-SHA512: 3a798607fb189a5bbc714ed6e86dea462fe29d366b790e96d10a7b4ffcf1f194da9b8f4cd0b82154408709b8e3c58d3f613d6311903bd65a76d8b556ab230d21

contrib/devtools/bitcoin-tidy/CMakeLists.txt

@@ -25,7 +25,7 @@ find_program(CLANG_TIDY_EXE NAMES "clang-tidy-${LLVM_VERSION_MAJOR}" "clang-tidy
 message(STATUS "Found LLVM ${LLVM_PACKAGE_VERSION}")
 message(STATUS "Found clang-tidy: ${CLANG_TIDY_EXE}")
 
-add_library(bitcoin-tidy MODULE bitcoin-tidy.cpp logprintf.cpp)
+add_library(bitcoin-tidy MODULE bitcoin-tidy.cpp logprintf.cpp nontrivial-threadlocal.cpp)
 target_include_directories(bitcoin-tidy SYSTEM PRIVATE ${LLVM_INCLUDE_DIRS})
 
 # Disable RTTI and exceptions as necessary
@@ -58,7 +58,7 @@ else()
 endif()
 
 # Create a dummy library that runs clang-tidy tests as a side-effect of building
-add_library(bitcoin-tidy-tests OBJECT EXCLUDE_FROM_ALL example_logprintf.cpp)
+add_library(bitcoin-tidy-tests OBJECT EXCLUDE_FROM_ALL example_logprintf.cpp example_nontrivial-threadlocal.cpp)
 add_dependencies(bitcoin-tidy-tests bitcoin-tidy)
 
 set_target_properties(bitcoin-tidy-tests PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_COMMAND}")

contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp

@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include "logprintf.h"
+#include "nontrivial-threadlocal.h"
 
 #include <clang-tidy/ClangTidyModule.h>
 #include <clang-tidy/ClangTidyModuleRegistry.h>
@@ -13,6 +14,7 @@ public:
     void addCheckFactories(clang::tidy::ClangTidyCheckFactories& CheckFactories) override
     {
         CheckFactories.registerCheck<bitcoin::LogPrintfCheck>("bitcoin-unterminated-logprintf");
+        CheckFactories.registerCheck<bitcoin::NonTrivialThreadLocal>("bitcoin-nontrivial-threadlocal");
     }
 };
 

contrib/devtools/bitcoin-tidy/example_nontrivial-threadlocal.cpp

@@ -0,0 +1,2 @@
+#include <string>
+thread_local std::string foo;

contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.cpp

@@ -0,0 +1,44 @@
+// Copyright (c) 2023 Bitcoin Developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include "nontrivial-threadlocal.h"
+
+#include <clang/AST/ASTContext.h>
+#include <clang/ASTMatchers/ASTMatchFinder.h>
+
+
+// Copied from clang-tidy's UnusedRaiiCheck
+namespace {
+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();
+}
+} // namespace
+
+namespace bitcoin {
+
+void NonTrivialThreadLocal::registerMatchers(clang::ast_matchers::MatchFinder* finder)
+{
+    using namespace clang::ast_matchers;
+
+    /*
+      thread_local std::string foo;
+    */
+
+    finder->addMatcher(
+        varDecl(
+            hasThreadStorageDuration(),
+            hasType(hasCanonicalType(recordType(hasDeclaration(cxxRecordDecl(hasNonTrivialDestructor())))))
+        ).bind("nontrivial_threadlocal"),
+        this);
+}
+
+void NonTrivialThreadLocal::check(const clang::ast_matchers::MatchFinder::MatchResult& Result)
+{
+    if (const clang::VarDecl* var = Result.Nodes.getNodeAs<clang::VarDecl>("nontrivial_threadlocal")) {
+        const auto user_diag = diag(var->getBeginLoc(), "Variable with non-trivial destructor cannot be thread_local.");
+    }
+}
+
+} // namespace bitcoin

contrib/devtools/bitcoin-tidy/nontrivial-threadlocal.h

@@ -0,0 +1,29 @@
+// Copyright (c) 2023 Bitcoin Developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef NONTRIVIAL_THREADLOCAL_CHECK_H
+#define NONTRIVIAL_THREADLOCAL_CHECK_H
+
+#include <clang-tidy/ClangTidyCheck.h>
+
+namespace bitcoin {
+
+// Warn about any thread_local variable with a non-trivial destructor.
+class NonTrivialThreadLocal final : public clang::tidy::ClangTidyCheck
+{
+public:
+    NonTrivialThreadLocal(clang::StringRef Name, clang::tidy::ClangTidyContext* Context)
+        : clang::tidy::ClangTidyCheck(Name, Context) {}
+
+    bool isLanguageVersionSupported(const clang::LangOptions& LangOpts) const override
+    {
+        return LangOpts.CPlusPlus;
+    }
+    void registerMatchers(clang::ast_matchers::MatchFinder* Finder) override;
+    void check(const clang::ast_matchers::MatchFinder::MatchResult& Result) override;
+};
+
+} // namespace bitcoin
+
+#endif // NONTRIVIAL_THREADLOCAL_CHECK_H