ci: Integrate bitcoin-tidy clang-tidy plugin
#26296
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| cmake_minimum_required(VERSION 3.9) | ||
|
|
||
| project(bitcoin-tidy VERSION 1.0.0 DESCRIPTION "clang-tidy checks for Bitcoin Core") | ||
|
|
||
| include(GNUInstallDirs) | ||
|
|
||
| set(CMAKE_CXX_STANDARD 17) | ||
| set(CMAKE_CXX_STANDARD_REQUIRED True) | ||
| set(CMAKE_CXX_EXTENSIONS False) | ||
|
|
||
| # TODO: Figure out how to avoid the terminfo check | ||
| find_package(LLVM REQUIRED CONFIG) | ||
| find_program(CLANG_TIDY_EXE NAMES "clang-tidy-${LLVM_VERSION_MAJOR}" "clang-tidy" HINTS ${LLVM_TOOLS_BINARY_DIR}) | ||
| 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) | ||
| target_include_directories(bitcoin-tidy SYSTEM PRIVATE ${LLVM_INCLUDE_DIRS}) | ||
|
|
||
| # Disable RTTI and exceptions as necessary | ||
| if (MSVC) | ||
| target_compile_options(bitcoin-tidy PRIVATE /GR-) | ||
| else() | ||
| target_compile_options(bitcoin-tidy PRIVATE -fno-rtti) | ||
| target_compile_options(bitcoin-tidy PRIVATE -fno-exceptions) | ||
| endif() | ||
|
|
||
| # Add warnings | ||
| if (MSVC) | ||
| target_compile_options(bitcoin-tidy PRIVATE /W4) | ||
| else() | ||
| target_compile_options(bitcoin-tidy PRIVATE -Wall) | ||
| target_compile_options(bitcoin-tidy PRIVATE -Wextra) | ||
| endif() | ||
|
|
||
| set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "--load=${CMAKE_BINARY_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}bitcoin-tidy${CMAKE_SHARED_LIBRARY_SUFFIX}" "-checks=-*,bitcoin-*") | ||
|
|
||
| # 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_dependencies(bitcoin-tidy-tests bitcoin-tidy) | ||
|
|
||
| set_target_properties(bitcoin-tidy-tests PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_COMMAND}") | ||
|
|
||
|
|
||
| install(TARGETS bitcoin-tidy LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| # Bitcoin Tidy | ||
|
|
||
| Example Usage: | ||
|
|
||
| ```bash | ||
| cmake -S . -B build -DLLVM_DIR=/path/to/lib/cmake/llvm -DCMAKE_BUILD_TYPE=Release | ||
|
Member
There was a problem hiding this comment. Maybe also add an example for common systems, Ubuntu/Debian-based ones, and Fedora/CentOS-based ones, what |
||
| make -C build -j$(nproc) | ||
| ``` | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| // 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 "logprintf.h" | ||
|
|
||
| #include <clang-tidy/ClangTidyModule.h> | ||
| #include <clang-tidy/ClangTidyModuleRegistry.h> | ||
|
|
||
| class BitcoinModule final : public clang::tidy::ClangTidyModule | ||
| { | ||
| public: | ||
| void addCheckFactories(clang::tidy::ClangTidyCheckFactories& CheckFactories) override | ||
| { | ||
| CheckFactories.registerCheck<bitcoin::LogPrintfCheck>("bitcoin-unterminated-logprintf"); | ||
| } | ||
| }; | ||
|
|
||
| static clang::tidy::ClangTidyModuleRegistry::Add<BitcoinModule> | ||
| X("bitcoin-module", "Adds bitcoin checks."); | ||
|
|
||
| volatile int BitcoinModuleAnchorSource = 0; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| // 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. | ||
|
|
||
| // Warn about any use of LogPrintf that does not end with a newline. | ||
|
Member
There was a problem hiding this comment. I think the test isn't run in CI, is it?
Member
There was a problem hiding this comment. This used to be part of the default build, but now needs to be run manually: It's probably not a bad idea to have them run by c-i, that way we can see that they're actually catching something.
Member
There was a problem hiding this comment.
Right, and also to ensure it compiles in the first place.
Member
There was a problem hiding this comment. FWIW, I made this non-default because I don't think it's a good idea for a vanilla As @MarcoFalke points out though, we do want it run when we're in control of the environment and not worried about those warnings. In order to run them, I suggest: diff --git a/ci/test/06_script_b.sh b/ci/test/06_script_b.sh
index 75d5469267..02358db789 100755
--- a/ci/test/06_script_b.sh
+++ b/ci/test/06_script_b.sh
@@ -150,6 +150,7 @@ fi
if [ "${RUN_TIDY}" = "true" ]; then
cmake -B /tidy-build -DLLVM_DIR=/usr/lib/llvm-16/cmake -DCMAKE_BUILD_TYPE=Release -S "${BASE_ROOT_DIR}"/contrib/devtools/bitcoin-tidy
cmake --build /tidy-build "$MAKEJOBS"
+ cmake --build /tidy-build --target bitcoin-tidy-tests "$MAKEJOBS"
set -eo pipefail
cd "${BASE_BUILD_DIR}/bitcoin-$HOST/src/"
Member
Author
There was a problem hiding this comment. Added in the current push. |
||
| #include <string> | ||
|
|
||
| enum LogFlags { | ||
| NONE | ||
| }; | ||
|
|
||
| enum Level { | ||
| None | ||
| }; | ||
|
|
||
| template <typename... Args> | ||
| static inline void LogPrintf_(const std::string& logging_function, const std::string& source_file, const int source_line, const LogFlags flag, const Level level, const char* fmt, const Args&... args) | ||
| { | ||
| } | ||
|
|
||
| #define LogPrintLevel_(category, level, ...) LogPrintf_(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__) | ||
| #define LogPrintf(...) LogPrintLevel_(LogFlags::NONE, Level::None, __VA_ARGS__) | ||
|
|
||
| // Use a macro instead of a function for conditional logging to prevent | ||
| // evaluating arguments when logging for the category is not enabled. | ||
|
Member
There was a problem hiding this comment. nit: Can drop the comment? (The others don't have a comment either) |
||
| #define LogPrint(category, ...) \ | ||
| do { \ | ||
| LogPrintf(__VA_ARGS__); \ | ||
| } while (0) | ||
|
|
||
|
|
||
| class CWallet | ||
| { | ||
| std::string GetDisplayName() const | ||
| { | ||
| return "default wallet"; | ||
| } | ||
|
|
||
| public: | ||
| template <typename... Params> | ||
| void WalletLogPrintf(std::string fmt, Params... parameters) const | ||
| { | ||
| LogPrintf(("%s " + fmt).c_str(), GetDisplayName(), parameters...); | ||
| }; | ||
| }; | ||
|
|
||
| void good_func() | ||
| { | ||
| LogPrintf("hello world!\n"); | ||
| } | ||
| void good_func2() | ||
| { | ||
| CWallet wallet; | ||
| wallet.WalletLogPrintf("hi\n"); | ||
|
|
||
| const CWallet& walletref = wallet; | ||
| walletref.WalletLogPrintf("hi\n"); | ||
|
|
||
| auto* walletptr = new CWallet(); | ||
| walletptr->WalletLogPrintf("hi\n"); | ||
| delete walletptr; | ||
| } | ||
| void bad_func() | ||
| { | ||
| LogPrintf("hello world!"); | ||
| } | ||
| void bad_func2() | ||
| { | ||
| LogPrintf(""); | ||
| } | ||
| void bad_func3() | ||
| { | ||
| // Ending in "..." has no special meaning. | ||
| LogPrintf("hello world!..."); | ||
| } | ||
| void bad_func4_ignored() | ||
| { | ||
| LogPrintf("hello world!"); // NOLINT(bitcoin-unterminated-logprintf) | ||
| } | ||
| void bad_func5() | ||
| { | ||
| CWallet wallet; | ||
| wallet.WalletLogPrintf("hi"); | ||
|
|
||
| const CWallet& walletref = wallet; | ||
| walletref.WalletLogPrintf("hi"); | ||
|
|
||
| auto* walletptr = new CWallet(); | ||
| walletptr->WalletLogPrintf("hi"); | ||
| delete walletptr; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| // 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 "logprintf.h" | ||
|
|
||
| #include <clang/AST/ASTContext.h> | ||
| #include <clang/ASTMatchers/ASTMatchFinder.h> | ||
|
|
||
|
|
||
| namespace { | ||
| AST_MATCHER(clang::StringLiteral, unterminated) | ||
| { | ||
| size_t len = Node.getLength(); | ||
| if (len > 0 && Node.getCodeUnit(len - 1) == '\n') { | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
| } // namespace | ||
|
|
||
| namespace bitcoin { | ||
|
|
||
| void LogPrintfCheck::registerMatchers(clang::ast_matchers::MatchFinder* finder) | ||
| { | ||
| using namespace clang::ast_matchers; | ||
|
|
||
| /* | ||
| Logprintf(..., ..., ..., ..., ..., "foo", ...) | ||
| */ | ||
|
|
||
| finder->addMatcher( | ||
| callExpr( | ||
| callee(functionDecl(hasName("LogPrintf_"))), | ||
| hasArgument(5, stringLiteral(unterminated()).bind("logstring"))), | ||
| this); | ||
|
|
||
| /* | ||
| CWallet wallet; | ||
| auto walletptr = &wallet; | ||
| wallet.WalletLogPrintf("foo"); | ||
| wallet->WalletLogPrintf("foo"); | ||
| */ | ||
| finder->addMatcher( | ||
| cxxMemberCallExpr( | ||
| thisPointerType(qualType(hasDeclaration(cxxRecordDecl(hasName("CWallet"))))), | ||
|
Member
There was a problem hiding this comment. This is wrong, too. My recommendation would be to just remove this line completely, unless there is a reason to have it. The other lines should be exact enough to match everything that is needed, without under- or over-matching.
Member
There was a problem hiding this comment. Ugh, I was inclined to argue with you because I think the tests should be as tight as possible. But realistically, it's more likely for a class to be forgotten in the checks (as has happened here) than a false-positive in some future class. So I begrudgingly agree.
Member
There was a problem hiding this comment. Mind creating a pull with the outstanding feedback? :) If not, I'll try to do it next week. |
||
| callee(cxxMethodDecl(hasName("WalletLogPrintf"))), | ||
| hasArgument(0, stringLiteral(unterminated()).bind("logstring"))), | ||
|
Member
There was a problem hiding this comment. This is wrong. The argument of |
||
| this); | ||
| } | ||
|
|
||
| void LogPrintfCheck::check(const clang::ast_matchers::MatchFinder::MatchResult& Result) | ||
| { | ||
| if (const clang::StringLiteral* lit = Result.Nodes.getNodeAs<clang::StringLiteral>("logstring")) { | ||
| const clang::ASTContext& ctx = *Result.Context; | ||
| const auto user_diag = diag(lit->getEndLoc(), "Unterminated format string used with LogPrintf"); | ||
| const auto& loc = lit->getLocationOfByte(lit->getByteLength(), *Result.SourceManager, ctx.getLangOpts(), ctx.getTargetInfo()); | ||
| user_diag << clang::FixItHint::CreateInsertion(loc, "\\n"); | ||
| } | ||
| } | ||
|
|
||
| } // namespace bitcoin | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| // 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 LOGPRINTF_CHECK_H | ||
| #define LOGPRINTF_CHECK_H | ||
|
|
||
| #include <clang-tidy/ClangTidyCheck.h> | ||
|
|
||
| namespace bitcoin { | ||
|
|
||
| class LogPrintfCheck final : public clang::tidy::ClangTidyCheck | ||
| { | ||
| public: | ||
| LogPrintfCheck(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 // LOGPRINTF_CHECK_H |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Could add a reason why this should be avoided?
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 intend to add a bunch of docs as a follow-up. I'll address these comments then as well.
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.
This is still up-for-grabs, if someone wants to take 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.
Actually, looking around, I think it makes more sense to just delete this comment. Autotools checks for a bazillion things it doesn't need. This is just one of those quirks that can't be turned off.
(The issue is that LLVM's CMake file checks for terminfo even though we don't need it. It doesn't seem to be a problem in the real world. If we encounter an actual issue, THEN we can worry about working around it.)