Skip to content

Commit 6243334

Browse files
fanquakefanquake
committed
Merge bitcoin/bitcoin#26296: ci: Integrate bitcoin-tidy clang-tidy plugin
1c976c6 tidy: Integrate bicoin-tidy clang-tidy plugin (fanquake) 7de23cc refactor: fix unterminated LogPrintf()s (fanquake) 0a1029a lint: remove /* Continued */ markers from codebase (fanquake) 9100079 lint: remove lint-logs.py (fanquake) d86a83d lint: drop DIR_IWYU global (fanquake) Pull request description: Demo of integrating the [bitcoin-tidy](https://github.com/theuni/bitcoin-tidy-plugin), [clang-tidy plugin](https://clang.llvm.org/extra/clang-tidy/) written by theuni into our tidy CI job. The plugin currently has a single check, `bitcoin-unterminated-logprintf`. This would replace our current Python driven, `git-grep`-based, `.cpp` file only, lint-logs linter. ACKs for top commit: TheCharlatan: ACK 1c976c6 theuni: ACK 1c976c6 MarcoFalke: re-ACK 1c976c6 👠 Tree-SHA512: 725b45c70e431d48e6f276671e05c694e10b6047cae1a31906ac3ee9093bc8105fb226b36a5bac6709557526ca6007222112d66aecec05a574434edc4897e4b8
2 parents 97ba721 + 1c976c6 commit 6243334

23 files changed

+294
-66
lines changed

‎ci/test/00_setup_env.sh

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,6 @@ export BASE_BUILD_DIR=${BASE_BUILD_DIR:-$BASE_SCRATCH_DIR/build}
6767
# The folder for previous release binaries.
6868
# This folder exists only on the ci guest, and on the ci host as a volume.
6969
export PREVIOUS_RELEASES_DIR=${PREVIOUS_RELEASES_DIR:-$BASE_ROOT_DIR/releases/$HOST}
70-
export DIR_IWYU="${BASE_SCRATCH_DIR}/iwyu"
7170
export SDK_URL=${SDK_URL:-https://bitcoincore.org/depends-sources/sdks}
7271
export CI_BASE_PACKAGES=${CI_BASE_PACKAGES:-build-essential libtool autotools-dev automake pkg-config bsdmainutils curl ca-certificates ccache python3 rsync git procps bison}
7372
export GOAL=${GOAL:-install}

‎ci/test/01_base_install.sh

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,9 @@ if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
7272
fi
7373

7474
if [[ "${RUN_TIDY}" == "true" ]]; then
75-
git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_16 "${DIR_IWYU}"/include-what-you-use
76-
cmake -B "${DIR_IWYU}"/build/ -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-16 -S "${DIR_IWYU}"/include-what-you-use
77-
make -C "${DIR_IWYU}"/build/ install "$MAKEJOBS"
75+
git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_16 /include-what-you-use
76+
cmake -B /iwyu-build/ -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-16 -S /include-what-you-use
77+
make -C /iwyu-build/ install "$MAKEJOBS"
7878
fi
7979

8080
mkdir -p "${DEPENDS_DIR}/SDKs" "${DEPENDS_DIR}/sdk-sources"

‎ci/test/06_script_b.sh

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -148,23 +148,27 @@ if [ "$RUN_FUNCTIONAL_TESTS" = "true" ]; then
148148
fi
149149

150150
if [ "${RUN_TIDY}" = "true" ]; then
151+
cmake -B /tidy-build -DLLVM_DIR=/usr/lib/llvm-16/cmake -DCMAKE_BUILD_TYPE=Release -S "${BASE_ROOT_DIR}"/contrib/devtools/bitcoin-tidy
152+
cmake --build /tidy-build "$MAKEJOBS"
153+
cmake --build /tidy-build --target bitcoin-tidy-tests "$MAKEJOBS"
154+
151155
set -eo pipefail
152156
cd "${BASE_BUILD_DIR}/bitcoin-$HOST/src/"
153-
( run-clang-tidy-16 -quiet "${MAKEJOBS}" ) | grep -C5 "error"
157+
( run-clang-tidy-16 -quiet -load="/tidy-build/libbitcoin-tidy.so" "${MAKEJOBS}" ) | grep -C5 "error"
154158
# Filter out files by regex here, because regex may not be
155159
# accepted in src/.bear-tidy-config
156160
# Filter out:
157161
# * qt qrc and moc generated files
158162
jq 'map(select(.file | test("src/qt/qrc_.*\\.cpp$|/moc_.*\\.cpp$") | not))' ../compile_commands.json > tmp.json
159163
mv tmp.json ../compile_commands.json
160164
cd "${BASE_BUILD_DIR}/bitcoin-$HOST/"
161-
python3 "${DIR_IWYU}/include-what-you-use/iwyu_tool.py" \
165+
python3 "/include-what-you-use/iwyu_tool.py" \
162166
-p . "${MAKEJOBS}" \
163167
-- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_BUILD_DIR}/bitcoin-$HOST/contrib/devtools/iwyu/bitcoin.core.imp" \
164168
-Xiwyu --max_line_length=160 \
165169
2>&1 | tee /tmp/iwyu_ci.out
166170
cd "${BASE_ROOT_DIR}/src"
167-
python3 "${DIR_IWYU}/include-what-you-use/fix_includes.py" --nosafe_headers < /tmp/iwyu_ci.out
171+
python3 "/include-what-you-use/fix_includes.py" --nosafe_headers < /tmp/iwyu_ci.out
168172
git --no-pager diff
169173
fi
170174

‎contrib/devtools/bitcoin-tidy/CMakeLists.txt

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
cmake_minimum_required(VERSION 3.9)
2+
3+
project(bitcoin-tidy VERSION 1.0.0 DESCRIPTION "clang-tidy checks for Bitcoin Core")
4+
5+
include(GNUInstallDirs)
6+
7+
set(CMAKE_CXX_STANDARD 17)
8+
set(CMAKE_CXX_STANDARD_REQUIRED True)
9+
set(CMAKE_CXX_EXTENSIONS False)
10+
11+
# TODO: Figure out how to avoid the terminfo check
12+
find_package(LLVM REQUIRED CONFIG)
13+
find_program(CLANG_TIDY_EXE NAMES "clang-tidy-${LLVM_VERSION_MAJOR}" "clang-tidy" HINTS ${LLVM_TOOLS_BINARY_DIR})
14+
message(STATUS "Found LLVM ${LLVM_PACKAGE_VERSION}")
15+
message(STATUS "Found clang-tidy: ${CLANG_TIDY_EXE}")
16+
17+
add_library(bitcoin-tidy MODULE bitcoin-tidy.cpp logprintf.cpp)
18+
target_include_directories(bitcoin-tidy SYSTEM PRIVATE ${LLVM_INCLUDE_DIRS})
19+
20+
# Disable RTTI and exceptions as necessary
21+
if (MSVC)
22+
target_compile_options(bitcoin-tidy PRIVATE /GR-)
23+
else()
24+
target_compile_options(bitcoin-tidy PRIVATE -fno-rtti)
25+
target_compile_options(bitcoin-tidy PRIVATE -fno-exceptions)
26+
endif()
27+
28+
# Add warnings
29+
if (MSVC)
30+
target_compile_options(bitcoin-tidy PRIVATE /W4)
31+
else()
32+
target_compile_options(bitcoin-tidy PRIVATE -Wall)
33+
target_compile_options(bitcoin-tidy PRIVATE -Wextra)
34+
endif()
35+
36+
set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "--load=${CMAKE_BINARY_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}bitcoin-tidy${CMAKE_SHARED_LIBRARY_SUFFIX}" "-checks=-*,bitcoin-*")
37+
38+
# Create a dummy library that runs clang-tidy tests as a side-effect of building
39+
add_library(bitcoin-tidy-tests OBJECT EXCLUDE_FROM_ALL example_logprintf.cpp)
40+
add_dependencies(bitcoin-tidy-tests bitcoin-tidy)
41+
42+
set_target_properties(bitcoin-tidy-tests PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_COMMAND}")
43+
44+
45+
install(TARGETS bitcoin-tidy LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR})

‎contrib/devtools/bitcoin-tidy/README

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
# Bitcoin Tidy
2+
3+
Example Usage:
4+
5+
```bash
6+
cmake -S . -B build -DLLVM_DIR=/path/to/lib/cmake/llvm -DCMAKE_BUILD_TYPE=Release
7+
make -C build -j$(nproc)
8+
```

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
// Copyright (c) 2023 Bitcoin Developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#include "logprintf.h"
6+
7+
#include <clang-tidy/ClangTidyModule.h>
8+
#include <clang-tidy/ClangTidyModuleRegistry.h>
9+
10+
class BitcoinModule final : public clang::tidy::ClangTidyModule
11+
{
12+
public:
13+
void addCheckFactories(clang::tidy::ClangTidyCheckFactories& CheckFactories) override
14+
{
15+
CheckFactories.registerCheck<bitcoin::LogPrintfCheck>("bitcoin-unterminated-logprintf");
16+
}
17+
};
18+
19+
static clang::tidy::ClangTidyModuleRegistry::Add<BitcoinModule>
20+
X("bitcoin-module", "Adds bitcoin checks.");
21+
22+
volatile int BitcoinModuleAnchorSource = 0;

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

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
// Copyright (c) 2023 Bitcoin Developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
// Warn about any use of LogPrintf that does not end with a newline.
6+
#include <string>
7+
8+
enum LogFlags {
9+
NONE
10+
};
11+
12+
enum Level {
13+
None
14+
};
15+
16+
template <typename... Args>
17+
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)
18+
{
19+
}
20+
21+
#define LogPrintLevel_(category, level, ...) LogPrintf_(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__)
22+
#define LogPrintf(...) LogPrintLevel_(LogFlags::NONE, Level::None, __VA_ARGS__)
23+
24+
// Use a macro instead of a function for conditional logging to prevent
25+
// evaluating arguments when logging for the category is not enabled.
26+
#define LogPrint(category, ...) \
27+
do { \
28+
LogPrintf(__VA_ARGS__); \
29+
} while (0)
30+
31+
32+
class CWallet
33+
{
34+
std::string GetDisplayName() const
35+
{
36+
return "default wallet";
37+
}
38+
39+
public:
40+
template <typename... Params>
41+
void WalletLogPrintf(std::string fmt, Params... parameters) const
42+
{
43+
LogPrintf(("%s " + fmt).c_str(), GetDisplayName(), parameters...);
44+
};
45+
};
46+
47+
void good_func()
48+
{
49+
LogPrintf("hello world!\n");
50+
}
51+
void good_func2()
52+
{
53+
CWallet wallet;
54+
wallet.WalletLogPrintf("hi\n");
55+
56+
const CWallet& walletref = wallet;
57+
walletref.WalletLogPrintf("hi\n");
58+
59+
auto* walletptr = new CWallet();
60+
walletptr->WalletLogPrintf("hi\n");
61+
delete walletptr;
62+
}
63+
void bad_func()
64+
{
65+
LogPrintf("hello world!");
66+
}
67+
void bad_func2()
68+
{
69+
LogPrintf("");
70+
}
71+
void bad_func3()
72+
{
73+
// Ending in "..." has no special meaning.
74+
LogPrintf("hello world!...");
75+
}
76+
void bad_func4_ignored()
77+
{
78+
LogPrintf("hello world!"); // NOLINT(bitcoin-unterminated-logprintf)
79+
}
80+
void bad_func5()
81+
{
82+
CWallet wallet;
83+
wallet.WalletLogPrintf("hi");
84+
85+
const CWallet& walletref = wallet;
86+
walletref.WalletLogPrintf("hi");
87+
88+
auto* walletptr = new CWallet();
89+
walletptr->WalletLogPrintf("hi");
90+
delete walletptr;
91+
}

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

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
// Copyright (c) 2023 Bitcoin Developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#include "logprintf.h"
6+
7+
#include <clang/AST/ASTContext.h>
8+
#include <clang/ASTMatchers/ASTMatchFinder.h>
9+
10+
11+
namespace {
12+
AST_MATCHER(clang::StringLiteral, unterminated)
13+
{
14+
size_t len = Node.getLength();
15+
if (len > 0 && Node.getCodeUnit(len - 1) == '\n') {
16+
return false;
17+
}
18+
return true;
19+
}
20+
} // namespace
21+
22+
namespace bitcoin {
23+
24+
void LogPrintfCheck::registerMatchers(clang::ast_matchers::MatchFinder* finder)
25+
{
26+
using namespace clang::ast_matchers;
27+
28+
/*
29+
Logprintf(..., ..., ..., ..., ..., "foo", ...)
30+
*/
31+
32+
finder->addMatcher(
33+
callExpr(
34+
callee(functionDecl(hasName("LogPrintf_"))),
35+
hasArgument(5, stringLiteral(unterminated()).bind("logstring"))),
36+
this);
37+
38+
/*
39+
CWallet wallet;
40+
auto walletptr = &wallet;
41+
wallet.WalletLogPrintf("foo");
42+
wallet->WalletLogPrintf("foo");
43+
*/
44+
finder->addMatcher(
45+
cxxMemberCallExpr(
46+
thisPointerType(qualType(hasDeclaration(cxxRecordDecl(hasName("CWallet"))))),
47+
callee(cxxMethodDecl(hasName("WalletLogPrintf"))),
48+
hasArgument(0, stringLiteral(unterminated()).bind("logstring"))),
49+
this);
50+
}
51+
52+
void LogPrintfCheck::check(const clang::ast_matchers::MatchFinder::MatchResult& Result)
53+
{
54+
if (const clang::StringLiteral* lit = Result.Nodes.getNodeAs<clang::StringLiteral>("logstring")) {
55+
const clang::ASTContext& ctx = *Result.Context;
56+
const auto user_diag = diag(lit->getEndLoc(), "Unterminated format string used with LogPrintf");
57+
const auto& loc = lit->getLocationOfByte(lit->getByteLength(), *Result.SourceManager, ctx.getLangOpts(), ctx.getTargetInfo());
58+
user_diag << clang::FixItHint::CreateInsertion(loc, "\\n");
59+
}
60+
}
61+
62+
} // namespace bitcoin

‎contrib/devtools/bitcoin-tidy/logprintf.h

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// Copyright (c) 2023 Bitcoin Developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#ifndef LOGPRINTF_CHECK_H
6+
#define LOGPRINTF_CHECK_H
7+
8+
#include <clang-tidy/ClangTidyCheck.h>
9+
10+
namespace bitcoin {
11+
12+
class LogPrintfCheck final : public clang::tidy::ClangTidyCheck
13+
{
14+
public:
15+
LogPrintfCheck(clang::StringRef Name, clang::tidy::ClangTidyContext* Context)
16+
: clang::tidy::ClangTidyCheck(Name, Context) {}
17+
18+
bool isLanguageVersionSupported(const clang::LangOptions& LangOpts) const override
19+
{
20+
return LangOpts.CPlusPlus;
21+
}
22+
void registerMatchers(clang::ast_matchers::MatchFinder* Finder) override;
23+
void check(const clang::ast_matchers::MatchFinder::MatchResult& Result) override;
24+
};
25+
26+
} // namespace bitcoin
27+
28+
#endif // LOGPRINTF_CHECK_H

‎src/.clang-tidy

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
Checks: '
22
-*,
3+
bitcoin-unterminated-logprintf,
34
bugprone-argument-comment,
45
bugprone-use-after-move,
56
misc-unused-using-decls,

0 commit comments

Comments
 (0)