From bbbb2e43ee95c9a8866aa1f65e3f001f752dfed2 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 19 Sep 2024 17:09:10 +0200 Subject: [PATCH 1/2] log: Enforce trailing newline, Remove redundant m_started_new_line All log lines already have a trailing newline, but enforcing it allows to delete unused code. --- src/logging.cpp | 21 ++++----------------- src/logging.h | 8 -------- src/test/logging_tests.cpp | 22 +++++++++++----------- 3 files changed, 15 insertions(+), 36 deletions(-) diff --git a/src/logging.cpp b/src/logging.cpp index d04db767e6e..5f055566ef5 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -368,6 +369,8 @@ static size_t MemUsage(const BCLog::Logger::BufferedLog& buflog) void BCLog::Logger::FormatLogStrInPlace(std::string& str, BCLog::LogFlags category, BCLog::Level level, std::string_view source_file, int source_line, std::string_view logging_function, std::string_view threadname, SystemClock::time_point now, std::chrono::seconds mocktime) const { + if (!str.ends_with('\n')) str.push_back('\n'); + str.insert(0, GetLogPrefix(category, level)); if (m_log_sourcelocations) { @@ -391,21 +394,7 @@ void BCLog::Logger::LogPrintStr_(std::string_view str, std::string_view logging_ { std::string str_prefixed = LogEscapeMessage(str); - const bool starts_new_line = m_started_new_line; - m_started_new_line = !str.empty() && str[str.size()-1] == '\n'; - if (m_buffering) { - if (!starts_new_line) { - if (!m_msgs_before_open.empty()) { - m_msgs_before_open.back().str += str_prefixed; - m_cur_buffer_memusage += str_prefixed.size(); - return; - } else { - // unlikely edge case; add a marker that something was trimmed - str_prefixed.insert(0, "[...] "); - } - } - { BufferedLog buf{ .now=SystemClock::now(), @@ -435,9 +424,7 @@ void BCLog::Logger::LogPrintStr_(std::string_view str, std::string_view logging_ return; } - if (starts_new_line) { - FormatLogStrInPlace(str_prefixed, category, level, source_file, source_line, logging_function, util::ThreadGetInternalName(), SystemClock::now(), GetMockTime()); - } + FormatLogStrInPlace(str_prefixed, category, level, source_file, source_line, logging_function, util::ThreadGetInternalName(), SystemClock::now(), GetMockTime()); if (m_print_to_console) { // print to console diff --git a/src/logging.h b/src/logging.h index 8605c8cd64f..fdc12c79b32 100644 --- a/src/logging.h +++ b/src/logging.h @@ -105,13 +105,6 @@ namespace BCLog { size_t m_cur_buffer_memusage GUARDED_BY(m_cs){0}; size_t m_buffer_lines_discarded GUARDED_BY(m_cs){0}; - /** - * m_started_new_line is a state variable that will suppress printing of - * the timestamp when multiple calls are made that don't end in a - * newline. - */ - std::atomic_bool m_started_new_line{true}; - //! Category-specific log level. Overrides `m_log_level`. std::unordered_map m_category_log_levels GUARDED_BY(m_cs); @@ -253,7 +246,6 @@ inline void LogPrintFormatInternal(std::string_view logging_function, std::strin try { log_msg = tfm::format(fmt, args...); } catch (tinyformat::format_error& fmterr) { - /* Original format string will have newline so don't add one here */ log_msg = "Error \"" + std::string{fmterr.what()} + "\" while formatting log message: " + fmt.fmt; } LogInstance().LogPrintStr(log_msg, logging_function, source_file, source_line, flag, level); diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp index 8217a2385cf..77ec81e5977 100644 --- a/src/test/logging_tests.cpp +++ b/src/test/logging_tests.cpp @@ -86,12 +86,12 @@ BOOST_AUTO_TEST_CASE(logging_timer) BOOST_FIXTURE_TEST_CASE(logging_LogPrintStr, LogSetup) { LogInstance().m_log_sourcelocations = true; - LogInstance().LogPrintStr("foo1: bar1\n", "fn1", "src1", 1, BCLog::LogFlags::NET, BCLog::Level::Debug); - LogInstance().LogPrintStr("foo2: bar2\n", "fn2", "src2", 2, BCLog::LogFlags::NET, BCLog::Level::Info); - LogInstance().LogPrintStr("foo3: bar3\n", "fn3", "src3", 3, BCLog::LogFlags::ALL, BCLog::Level::Debug); - LogInstance().LogPrintStr("foo4: bar4\n", "fn4", "src4", 4, BCLog::LogFlags::ALL, BCLog::Level::Info); - LogInstance().LogPrintStr("foo5: bar5\n", "fn5", "src5", 5, BCLog::LogFlags::NONE, BCLog::Level::Debug); - LogInstance().LogPrintStr("foo6: bar6\n", "fn6", "src6", 6, BCLog::LogFlags::NONE, BCLog::Level::Info); + LogInstance().LogPrintStr("foo1: bar1", "fn1", "src1", 1, BCLog::LogFlags::NET, BCLog::Level::Debug); + LogInstance().LogPrintStr("foo2: bar2", "fn2", "src2", 2, BCLog::LogFlags::NET, BCLog::Level::Info); + LogInstance().LogPrintStr("foo3: bar3", "fn3", "src3", 3, BCLog::LogFlags::ALL, BCLog::Level::Debug); + LogInstance().LogPrintStr("foo4: bar4", "fn4", "src4", 4, BCLog::LogFlags::ALL, BCLog::Level::Info); + LogInstance().LogPrintStr("foo5: bar5", "fn5", "src5", 5, BCLog::LogFlags::NONE, BCLog::Level::Debug); + LogInstance().LogPrintStr("foo6: bar6", "fn6", "src6", 6, BCLog::LogFlags::NONE, BCLog::Level::Info); std::ifstream file{tmp_log_path}; std::vector log_lines; for (std::string log; std::getline(file, log);) { @@ -133,11 +133,11 @@ BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacrosDeprecated, LogSetup) BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacros, LogSetup) { - LogTrace(BCLog::NET, "foo6: %s\n", "bar6"); // not logged - LogDebug(BCLog::NET, "foo7: %s\n", "bar7"); - LogInfo("foo8: %s\n", "bar8"); - LogWarning("foo9: %s\n", "bar9"); - LogError("foo10: %s\n", "bar10"); + LogTrace(BCLog::NET, "foo6: %s", "bar6"); // not logged + LogDebug(BCLog::NET, "foo7: %s", "bar7"); + LogInfo("foo8: %s", "bar8"); + LogWarning("foo9: %s", "bar9"); + LogError("foo10: %s", "bar10"); std::ifstream file{tmp_log_path}; std::vector log_lines; for (std::string log; std::getline(file, log);) { From fa2b7d8d6b3f8d53199921e1e542072441b26fab Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 19 Sep 2024 16:46:09 +0200 Subject: [PATCH 2/2] Remove redundant unterminated-logprintf tidy check --- contrib/devtools/bitcoin-tidy/CMakeLists.txt | 4 +- .../devtools/bitcoin-tidy/bitcoin-tidy.cpp | 2 - .../bitcoin-tidy/example_logprintf.cpp | 108 ------------------ contrib/devtools/bitcoin-tidy/logprintf.cpp | 60 ---------- contrib/devtools/bitcoin-tidy/logprintf.h | 29 ----- test/lint/lint-format-strings.py | 2 +- 6 files changed, 3 insertions(+), 202 deletions(-) delete mode 100644 contrib/devtools/bitcoin-tidy/example_logprintf.cpp delete mode 100644 contrib/devtools/bitcoin-tidy/logprintf.cpp delete mode 100644 contrib/devtools/bitcoin-tidy/logprintf.h diff --git a/contrib/devtools/bitcoin-tidy/CMakeLists.txt b/contrib/devtools/bitcoin-tidy/CMakeLists.txt index 95345b47829..c6f683f7aba 100644 --- a/contrib/devtools/bitcoin-tidy/CMakeLists.txt +++ b/contrib/devtools/bitcoin-tidy/CMakeLists.txt @@ -25,7 +25,7 @@ find_program(CLANG_TIDY_EXE NAMES "clang-tidy-${LLVM_VERSION_MAJOR}" "clang-tidy 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 nontrivial-threadlocal.cpp) +add_library(bitcoin-tidy MODULE bitcoin-tidy.cpp nontrivial-threadlocal.cpp) target_include_directories(bitcoin-tidy SYSTEM PRIVATE ${LLVM_INCLUDE_DIRS}) # Disable RTTI and exceptions as necessary @@ -58,7 +58,7 @@ else() endif() # 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 example_nontrivial-threadlocal.cpp) +add_library(bitcoin-tidy-tests OBJECT EXCLUDE_FROM_ALL example_nontrivial-threadlocal.cpp) add_dependencies(bitcoin-tidy-tests bitcoin-tidy) set_target_properties(bitcoin-tidy-tests PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_COMMAND}") diff --git a/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp b/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp index 1ef44949737..f2658b5a586 100644 --- a/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp +++ b/contrib/devtools/bitcoin-tidy/bitcoin-tidy.cpp @@ -2,7 +2,6 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#include "logprintf.h" #include "nontrivial-threadlocal.h" #include @@ -13,7 +12,6 @@ class BitcoinModule final : public clang::tidy::ClangTidyModule public: void addCheckFactories(clang::tidy::ClangTidyCheckFactories& CheckFactories) override { - CheckFactories.registerCheck("bitcoin-unterminated-logprintf"); CheckFactories.registerCheck("bitcoin-nontrivial-threadlocal"); } }; diff --git a/contrib/devtools/bitcoin-tidy/example_logprintf.cpp b/contrib/devtools/bitcoin-tidy/example_logprintf.cpp deleted file mode 100644 index dc77f668e3e..00000000000 --- a/contrib/devtools/bitcoin-tidy/example_logprintf.cpp +++ /dev/null @@ -1,108 +0,0 @@ -// 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 - -// Test for bitcoin-unterminated-logprintf - -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__) - -#define LogDebug(category, ...) \ - do { \ - LogPrintf(__VA_ARGS__); \ - } while (0) - - -class CWallet -{ - std::string GetDisplayName() const - { - return "default wallet"; - } - -public: - template - void WalletLogPrintf(const char* fmt, Params... parameters) const - { - LogPrintf(("%s " + std::string{fmt}).c_str(), GetDisplayName(), parameters...); - }; -}; - -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"); -} -void good_func2() -{ - CWallet wallet; - wallet.WalletLogPrintf("hi\n"); - ScriptPubKeyMan spkm; - spkm.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"); - ScriptPubKeyMan spkm; - spkm.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 deleted file mode 100644 index 36beac28c86..00000000000 --- a/contrib/devtools/bitcoin-tidy/logprintf.cpp +++ /dev/null @@ -1,60 +0,0 @@ -// 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); - - /* - auto walletptr = &wallet; - wallet.WalletLogPrintf("foo"); - wallet->WalletLogPrintf("foo"); - */ - finder->addMatcher( - cxxMemberCallExpr( - 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 deleted file mode 100644 index db95dfe143e..00000000000 --- a/contrib/devtools/bitcoin-tidy/logprintf.h +++ /dev/null @@ -1,29 +0,0 @@ -// 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 { - -// Warn about any use of LogPrintf that does not end with a newline. -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/test/lint/lint-format-strings.py b/test/lint/lint-format-strings.py index a809851ec63..86a17fb0f8c 100755 --- a/test/lint/lint-format-strings.py +++ b/test/lint/lint-format-strings.py @@ -62,7 +62,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)|contrib/devtools/bitcoin-tidy/example_logprintf.cpp', matching_file): + if not re.search('^src/(leveldb|secp256k1|minisketch|tinyformat|test/fuzz/strprintf.cpp)', matching_file): matching_files_filtered.append(matching_file) matching_files_filtered.sort()