diff --git a/ci/test/06_script_b.sh b/ci/test/06_script_b.sh index d769b49c08d..02358db7891 100755 --- a/ci/test/06_script_b.sh +++ b/ci/test/06_script_b.sh @@ -148,9 +148,13 @@ if [ "$RUN_FUNCTIONAL_TESTS" = "true" ]; then 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/" - ( run-clang-tidy-16 -quiet "${MAKEJOBS}" ) | grep -C5 "error" + ( run-clang-tidy-16 -quiet -load="/tidy-build/libbitcoin-tidy.so" "${MAKEJOBS}" ) | grep -C5 "error" # Filter out files by regex here, because regex may not be # accepted in src/.bear-tidy-config # Filter out: diff --git a/contrib/devtools/bitcoin-tidy/CMakeLists.txt b/contrib/devtools/bitcoin-tidy/CMakeLists.txt new file mode 100644 index 00000000000..9ed18696d41 --- /dev/null +++ b/contrib/devtools/bitcoin-tidy/CMakeLists.txt @@ -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}) diff --git a/contrib/devtools/bitcoin-tidy/README b/contrib/devtools/bitcoin-tidy/README new file mode 100644 index 00000000000..1669fe98f57 --- /dev/null +++ b/contrib/devtools/bitcoin-tidy/README @@ -0,0 +1,8 @@ +# Bitcoin Tidy + +Example Usage: + +```bash +cmake -S . -B build -DLLVM_DIR=/path/to/lib/cmake/llvm -DCMAKE_BUILD_TYPE=Release +make -C build -j$(nproc) +``` diff --git a/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp b/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp new file mode 100644 index 00000000000..0f34d377938 --- /dev/null +++ b/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp @@ -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 +#include + +class BitcoinModule final : public clang::tidy::ClangTidyModule +{ +public: + void addCheckFactories(clang::tidy::ClangTidyCheckFactories& CheckFactories) override + { + CheckFactories.registerCheck("bitcoin-unterminated-logprintf"); + } +}; + +static clang::tidy::ClangTidyModuleRegistry::Add + X("bitcoin-module", "Adds bitcoin checks."); + +volatile int BitcoinModuleAnchorSource = 0; diff --git a/contrib/devtools/bitcoin-tidy/example_logprintf.cpp b/contrib/devtools/bitcoin-tidy/example_logprintf.cpp new file mode 100644 index 00000000000..a3d2768964c --- /dev/null +++ b/contrib/devtools/bitcoin-tidy/example_logprintf.cpp @@ -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. +#include + +enum LogFlags { + NONE +}; + +enum Level { + None +}; + +template +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. +#define LogPrint(category, ...) \ + do { \ + LogPrintf(__VA_ARGS__); \ + } while (0) + + +class CWallet +{ + std::string GetDisplayName() const + { + return "default wallet"; + } + +public: + template + 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; +} diff --git a/contrib/devtools/bitcoin-tidy/logprintf.cpp b/contrib/devtools/bitcoin-tidy/logprintf.cpp new file mode 100644 index 00000000000..1690c8fde0a --- /dev/null +++ b/contrib/devtools/bitcoin-tidy/logprintf.cpp @@ -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 +#include + + +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"))))), + callee(cxxMethodDecl(hasName("WalletLogPrintf"))), + hasArgument(0, stringLiteral(unterminated()).bind("logstring"))), + this); +} + +void LogPrintfCheck::check(const clang::ast_matchers::MatchFinder::MatchResult& Result) +{ + if (const clang::StringLiteral* lit = Result.Nodes.getNodeAs("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 diff --git a/contrib/devtools/bitcoin-tidy/logprintf.h b/contrib/devtools/bitcoin-tidy/logprintf.h new file mode 100644 index 00000000000..466849ef011 --- /dev/null +++ b/contrib/devtools/bitcoin-tidy/logprintf.h @@ -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 + +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 diff --git a/src/.clang-tidy b/src/.clang-tidy index 84c9d5fb3a0..b4d50135dd0 100644 --- a/src/.clang-tidy +++ b/src/.clang-tidy @@ -1,5 +1,6 @@ Checks: ' -*, +bitcoin-unterminated-logprintf, bugprone-argument-comment, bugprone-use-after-move, misc-unused-using-decls, diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp index 91dc731a4ca..7cf6bef90fb 100644 --- a/src/dbwrapper.cpp +++ b/src/dbwrapper.cpp @@ -76,7 +76,7 @@ public: assert(p <= limit); base[std::min(bufsize - 1, (int)(p - base))] = '\0'; - LogPrintLevel(BCLog::LEVELDB, BCLog::Level::Debug, "%s", base); + LogPrintLevel(BCLog::LEVELDB, BCLog::Level::Debug, "%s", base); // NOLINT(bitcoin-unterminated-logprintf) if (base != buffer) { delete[] base; } diff --git a/test/lint/lint-format-strings.py b/test/lint/lint-format-strings.py index 43addab2f3d..5ac5840ecff 100755 --- a/test/lint/lint-format-strings.py +++ b/test/lint/lint-format-strings.py @@ -77,7 +77,7 @@ def main(): matching_files_filtered = [] for matching_file in matching_files: - if not re.search('^src/(leveldb|secp256k1|minisketch|tinyformat|test/fuzz/strprintf.cpp)', matching_file): + if not re.search('^src/(leveldb|secp256k1|minisketch|tinyformat|test/fuzz/strprintf.cpp)|contrib/devtools/bitcoin-tidy/example_logprintf.cpp', matching_file): matching_files_filtered.append(matching_file) matching_files_filtered.sort() diff --git a/test/lint/lint-include-guards.py b/test/lint/lint-include-guards.py index 5867aae028a..48b918e9dab 100755 --- a/test/lint/lint-include-guards.py +++ b/test/lint/lint-include-guards.py @@ -17,7 +17,8 @@ from typing import List HEADER_ID_PREFIX = 'BITCOIN_' HEADER_ID_SUFFIX = '_H' -EXCLUDE_FILES_WITH_PREFIX = ['src/crypto/ctaes', +EXCLUDE_FILES_WITH_PREFIX = ['contrib/devtools/bitcoin-tidy', + 'src/crypto/ctaes', 'src/leveldb', 'src/crc32c', 'src/secp256k1', diff --git a/test/lint/lint-includes.py b/test/lint/lint-includes.py index b14caa4855f..8e79ba51218 100755 --- a/test/lint/lint-includes.py +++ b/test/lint/lint-includes.py @@ -15,7 +15,8 @@ import sys from subprocess import check_output, CalledProcessError -EXCLUDED_DIRS = ["src/leveldb/", +EXCLUDED_DIRS = ["contrib/devtools/bitcoin-tidy/", + "src/leveldb/", "src/crc32c/", "src/secp256k1/", "src/minisketch/",