From fa244f3321de7884f530bb38493a8d0a0cec86ab Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 8 Aug 2023 09:17:15 +0200 Subject: [PATCH 1/4] doc: Fix bitcoin-unterminated-logprintf tidy comments * Move module description from test to LogPrintfCheck * Add test doc * Remove unused comment, see https://github.com/bitcoin/bitcoin/pull/26296/files#r1279351539 --- contrib/devtools/bitcoin-tidy/example_logprintf.cpp | 5 ++--- contrib/devtools/bitcoin-tidy/logprintf.h | 1 + 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contrib/devtools/bitcoin-tidy/example_logprintf.cpp b/contrib/devtools/bitcoin-tidy/example_logprintf.cpp index a3d2768964c..d78fc2cd6ce 100644 --- a/contrib/devtools/bitcoin-tidy/example_logprintf.cpp +++ b/contrib/devtools/bitcoin-tidy/example_logprintf.cpp @@ -2,9 +2,10 @@ // 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 +// Test for bitcoin-unterminated-logprintf + enum LogFlags { NONE }; @@ -21,8 +22,6 @@ static inline void LogPrintf_(const std::string& logging_function, const std::st #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__); \ diff --git a/contrib/devtools/bitcoin-tidy/logprintf.h b/contrib/devtools/bitcoin-tidy/logprintf.h index 466849ef011..db95dfe143e 100644 --- a/contrib/devtools/bitcoin-tidy/logprintf.h +++ b/contrib/devtools/bitcoin-tidy/logprintf.h @@ -9,6 +9,7 @@ namespace bitcoin { +// Warn about any use of LogPrintf that does not end with a newline. class LogPrintfCheck final : public clang::tidy::ClangTidyCheck { public: From fa6dc57760e0a04dbb2e365ca7ad9fd8171ebfdb Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 8 Aug 2023 09:30:51 +0200 Subject: [PATCH 2/4] refactor: Enforce C-str fmt strings in WalletLogPrintf() --- contrib/devtools/bitcoin-tidy/example_logprintf.cpp | 4 ++-- src/wallet/scriptpubkeyman.h | 7 ++++--- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 7 ++++--- test/lint/run-lint-format-strings.py | 8 ++++---- 5 files changed, 15 insertions(+), 13 deletions(-) diff --git a/contrib/devtools/bitcoin-tidy/example_logprintf.cpp b/contrib/devtools/bitcoin-tidy/example_logprintf.cpp index d78fc2cd6ce..3106a0c161f 100644 --- a/contrib/devtools/bitcoin-tidy/example_logprintf.cpp +++ b/contrib/devtools/bitcoin-tidy/example_logprintf.cpp @@ -37,9 +37,9 @@ class CWallet public: template - void WalletLogPrintf(std::string fmt, Params... parameters) const + void WalletLogPrintf(const char* fmt, Params... parameters) const { - LogPrintf(("%s " + fmt).c_str(), GetDisplayName(), parameters...); + LogPrintf(("%s " + std::string{fmt}).c_str(), GetDisplayName(), parameters...); }; }; diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index bf35c776ae1..72051493a97 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -249,9 +249,10 @@ public: virtual std::unordered_set GetScriptPubKeys() const { return {}; }; /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */ - template - void WalletLogPrintf(std::string fmt, Params... parameters) const { - LogPrintf(("%s " + fmt).c_str(), m_storage.GetDisplayName(), parameters...); + template + void WalletLogPrintf(const char* fmt, Params... parameters) const + { + LogPrintf(("%s " + std::string{fmt}).c_str(), m_storage.GetDisplayName(), parameters...); }; /** Watch-only address added */ diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6b2755ea530..7df4a2afa2f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2319,7 +2319,7 @@ OutputType CWallet::TransactionChangeType(const std::optional& chang void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm) { LOCK(cs_wallet); - WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); + WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); // NOLINT(bitcoin-unterminated-logprintf) // Add tx to wallet, because if it has change it's also ours, // otherwise just for transaction history. diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index cbd50083669..3d88fab74e9 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -890,9 +890,10 @@ public: }; /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */ - template - void WalletLogPrintf(std::string fmt, Params... parameters) const { - LogPrintf(("%s " + fmt).c_str(), GetDisplayName(), parameters...); + template + void WalletLogPrintf(const char* fmt, Params... parameters) const + { + LogPrintf(("%s " + std::string{fmt}).c_str(), GetDisplayName(), parameters...); }; /** Upgrade the wallet */ diff --git a/test/lint/run-lint-format-strings.py b/test/lint/run-lint-format-strings.py index ed98b1b2f81..244bf5956f0 100755 --- a/test/lint/run-lint-format-strings.py +++ b/test/lint/run-lint-format-strings.py @@ -20,10 +20,10 @@ FALSE_POSITIVES = [ ("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION)"), ("src/test/translation_tests.cpp", "strprintf(format, arg)"), ("src/validationinterface.cpp", "LogPrint(BCLog::VALIDATION, fmt \"\\n\", __VA_ARGS__)"), - ("src/wallet/wallet.h", "WalletLogPrintf(std::string fmt, Params... parameters)"), - ("src/wallet/wallet.h", "LogPrintf((\"%s \" + fmt).c_str(), GetDisplayName(), parameters...)"), - ("src/wallet/scriptpubkeyman.h", "WalletLogPrintf(std::string fmt, Params... parameters)"), - ("src/wallet/scriptpubkeyman.h", "LogPrintf((\"%s \" + fmt).c_str(), m_storage.GetDisplayName(), parameters...)"), + ("src/wallet/wallet.h", "WalletLogPrintf(const char* fmt, Params... parameters)"), + ("src/wallet/wallet.h", "LogPrintf((\"%s \" + std::string{fmt}).c_str(), GetDisplayName(), parameters...)"), + ("src/wallet/scriptpubkeyman.h", "WalletLogPrintf(const char* fmt, Params... parameters)"), + ("src/wallet/scriptpubkeyman.h", "LogPrintf((\"%s \" + std::string{fmt}).c_str(), m_storage.GetDisplayName(), parameters...)"), ] From faa11434fe38aa82892802adb6d879d112ae1675 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 16 Aug 2023 14:50:19 +0200 Subject: [PATCH 3/4] refactor: Enable all clang-tidy plugin bitcoin tests This makes it easier to add new ones without having to modify this file every time. --- src/.clang-tidy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/.clang-tidy b/src/.clang-tidy index b4d50135dd0..4deb5a85a5e 100644 --- a/src/.clang-tidy +++ b/src/.clang-tidy @@ -1,6 +1,6 @@ Checks: ' -*, -bitcoin-unterminated-logprintf, +bitcoin-*, bugprone-argument-comment, bugprone-use-after-move, misc-unused-using-decls, From fa60fa3b0cba4a30726af8e0e9d1e84e14849eda Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 16 Aug 2023 14:55:19 +0200 Subject: [PATCH 4/4] bitcoin-tidy: Apply bitcoin-unterminated-logprintf to spkm as well --- .../bitcoin-tidy/example_logprintf.cpp | 18 ++++++++++++++++++ contrib/devtools/bitcoin-tidy/logprintf.cpp | 2 -- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/contrib/devtools/bitcoin-tidy/example_logprintf.cpp b/contrib/devtools/bitcoin-tidy/example_logprintf.cpp index 3106a0c161f..a12a666c086 100644 --- a/contrib/devtools/bitcoin-tidy/example_logprintf.cpp +++ b/contrib/devtools/bitcoin-tidy/example_logprintf.cpp @@ -43,6 +43,20 @@ public: }; }; +struct ScriptPubKeyMan +{ + std::string GetDisplayName() const + { + return "default wallet"; + } + + template + void WalletLogPrintf(const char* fmt, Params... parameters) const + { + LogPrintf(("%s " + std::string{fmt}).c_str(), GetDisplayName(), parameters...); + }; +}; + void good_func() { LogPrintf("hello world!\n"); @@ -51,6 +65,8 @@ void good_func2() { CWallet wallet; wallet.WalletLogPrintf("hi\n"); + ScriptPubKeyMan spkm; + spkm.WalletLogPrintf("hi\n"); const CWallet& walletref = wallet; walletref.WalletLogPrintf("hi\n"); @@ -80,6 +96,8 @@ void bad_func5() { CWallet wallet; wallet.WalletLogPrintf("hi"); + ScriptPubKeyMan spkm; + spkm.WalletLogPrintf("hi"); const CWallet& walletref = wallet; walletref.WalletLogPrintf("hi"); diff --git a/contrib/devtools/bitcoin-tidy/logprintf.cpp b/contrib/devtools/bitcoin-tidy/logprintf.cpp index 1690c8fde0a..36beac28c86 100644 --- a/contrib/devtools/bitcoin-tidy/logprintf.cpp +++ b/contrib/devtools/bitcoin-tidy/logprintf.cpp @@ -36,14 +36,12 @@ void LogPrintfCheck::registerMatchers(clang::ast_matchers::MatchFinder* finder) 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);