All fmt functions only accept a raw C-string as argument.
There should never be a need to pass a format string that is not a compile-time string literal, so disallow it in WalletLogPrintf() to avoid accidentally introducing it.
Apart from consistency, this also fixes the clang-tidy plugin bug #26296 (review).
* Move module description from test to LogPrintfCheck
* Add test doc
* Remove unused comment, see https://github.com/bitcoin/bitcoin/pull/26296/files#r1279351539
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | theuni |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
2318@@ -2319,7 +2319,7 @@ OutputType CWallet::TransactionChangeType(const std::optional<OutputType>& chang
2319 void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm)
2320 {
2321 LOCK(cs_wallet);
2322- WalletLogPrintf("CommitTransaction:\n%s", tx->ToString());
2323+ WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); // NOLINT(bitcoin-unterminated-logprintf)
Hmm.
I haven’t tried this using the c-i scripts, but running clang-tidy manually against current master, this works fine for me as-is:
0$ /opt/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang-tidy-16 --load=/home/cory/dev/bitcoin2/contrib/devtools/bitcoin-tidy/build2/libbitcoin-tidy.so -checks="-*,bitcoin-*" wallet/wallet.cpp -- -DHAVE_BUILD_INFO -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I./leveldb/include -isystem /home/cory/dev/bitcoin2/depends/x86_64-pc-linux-gnu/include -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE -DHAVE_CONFIG_H -std=c++17
11 warning generated.
2/home/cory/dev/bitcoin2/src/wallet/wallet.cpp:2322:21: error: Unterminated format string used with LogPrintf [bitcoin-unterminated-logprintf,-warnings-as-errors]
3 WalletLogPrintf("CommitTransaction:\n%s", tx->ToString());
4 ^
5 \n
61 warning treated as error
Any idea what the difference is?
@theuni You’ll have to share exact steps to reproduce, ideally starting from a fresh install of you operating system. Otherwise it will be close to impossible to reproduce. Locally and on CI this isn’t caught (otherwise CI would be red). c.f
0$ (cd src/ && clang-tidy --load=/home/fake/bitcoin/contrib/devtools/bitcoin-tidy/build/libbitcoin-tidy.so -checks="-*,bitcoin-*" wallet/wallet.cpp )
112 warnings generated.
2Suppressed 12 warnings (12 in non-user code).
3Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
Huh. For whatever reason, my clang-tidy-16 acts as if IgnoreUnlessSpelledInSource is set. That’s weird.
It came from here: https://github.com/llvm/llvm-project/releases/download/llvmorg-16.0.0/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04.tar.xz
I can’t reproduce with clang-tidy-15. clang-query-16 only matches if traversal is set to IgnoreUnlessSpelledInSource.
$ /opt/clang+llvm-15.0.2-x86_64-unknown-linux-gnu/bin/clang-query wallet/wallet.cpp – -DHAVE_BUILD_INFO -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I./leveldb/include -isystem /home/cory/dev/bitcoin2/depends/x86_64-pc-linux-gnu/include -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE -DHAVE_CONFIG_H -std=c++17 -O2 clang-query> m cxxMemberCallExpr(thisPointerType(qualType(hasDeclaration(cxxRecordDecl(hasName(“CWallet”))))),callee(cxxMethodDecl(hasName(“WalletLogPrintf”))),hasArgument(0, stringLiteral().bind(“logstring”))) 0 matches. clang-query> set traversal IgnoreUnlessSpelledInSource clang-query> m cxxMemberCallExpr(thisPointerType(qualType(hasDeclaration(cxxRecordDecl(hasName(“CWallet”))))),callee(cxxMethodDecl(hasName(“WalletLogPrintf”))),hasArgument(0, stringLiteral().bind(“logstring”)))
…
Match #33:
/home/cory/dev/bitcoin2/src/wallet/wallet.cpp:4149:36: note: “logstring” binds here wallet.WalletLogPrintf(“Making a new watchonly wallet containing the unwatched solvable scripts\n”); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/cory/dev/bitcoin2/src/wallet/wallet.cpp:4149:13: note: “root” binds here wallet.WalletLogPrintf(“Making a new watchonly wallet containing the unwatched solvable scripts\n”); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 33 matches
I don’t think this is worth spending any more time on, but I still don’t understand why it’s different.
889@@ -890,9 +890,10 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
890 };
891
892 /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */
893- template<typename... Params>
894- void WalletLogPrintf(std::string fmt, Params... parameters) const {
895- LogPrintf(("%s " + fmt).c_str(), GetDisplayName(), parameters...);
896+ template <typename... Params>
897+ void WalletLogPrintf(const char* fmt, Params... parameters) const
nonnull attribute here, but I believe our tidy check actually enforces that :)
LogPrint(nullptr, 1, 2, 3). But adding nonnull can be done in a follow-up, if needed, for all log-functions.
Makes sense to me. I almost did the same thing while poking at WalletLogPrintf for the sake of easier static analysis.
Am I missing some obvious reason though, other than “might as well”?
Am I missing some obvious reason though, other than “might as well”?
Edited OP to list both reasons.
This makes it easier to add new ones without having to modify this file
every time.
Labels
Refactoring