From f36183604eb90bb7a921721e445e6dacff9e25bb Mon Sep 17 00:00:00 2001 From: Andy Getzendanner Date: Tue, 18 Apr 2023 00:29:00 -0700 Subject: Add an API to clear the saved LogBacktraceAt location, and call it when setting an empty or invalid flag value. PiperOrigin-RevId: 525065479 Change-Id: I3c0822db8301e0999b0669394b415df82edd445f --- absl/log/flags.cc | 16 ++++++++++++---- absl/log/flags_test.cc | 26 +++++++++++++++----------- absl/log/globals.cc | 8 ++++++-- absl/log/globals.h | 13 ++++++++++--- 4 files changed, 43 insertions(+), 20 deletions(-) (limited to 'absl/log') diff --git a/absl/log/flags.cc b/absl/log/flags.cc index b5308881..215b7bd5 100644 --- a/absl/log/flags.cc +++ b/absl/log/flags.cc @@ -90,19 +90,27 @@ ABSL_FLAG(std::string, log_backtrace_at, "", .OnUpdate([] { const std::string log_backtrace_at = absl::GetFlag(FLAGS_log_backtrace_at); - if (log_backtrace_at.empty()) return; + if (log_backtrace_at.empty()) { + absl::ClearLogBacktraceLocation(); + return; + } const size_t last_colon = log_backtrace_at.rfind(':'); - if (last_colon == log_backtrace_at.npos) return; + if (last_colon == log_backtrace_at.npos) { + absl::ClearLogBacktraceLocation(); + return; + } const absl::string_view file = absl::string_view(log_backtrace_at).substr(0, last_colon); int line; - if (absl::SimpleAtoi( + if (!absl::SimpleAtoi( absl::string_view(log_backtrace_at).substr(last_colon + 1), &line)) { - absl::SetLogBacktraceLocation(file, line); + absl::ClearLogBacktraceLocation(); + return; } + absl::SetLogBacktraceLocation(file, line); }); ABSL_FLAG(bool, log_prefix, true, diff --git a/absl/log/flags_test.cc b/absl/log/flags_test.cc index a0f6d763..1080ea11 100644 --- a/absl/log/flags_test.cc +++ b/absl/log/flags_test.cc @@ -92,23 +92,23 @@ TEST_F(LogFlagsTest, PrependLogPrefix) { TEST_F(LogFlagsTest, EmptyBacktraceAtFlag) { absl::SetMinLogLevel(absl::LogSeverityAtLeast::kInfo); - absl::SetFlag(&FLAGS_log_backtrace_at, ""); absl::ScopedMockLog test_sink(absl::MockLogDefault::kDisallowUnexpected); EXPECT_CALL(test_sink, Send(TextMessage(Not(HasSubstr("(stacktrace:"))))); test_sink.StartCapturingLogs(); + absl::SetFlag(&FLAGS_log_backtrace_at, ""); LOG(INFO) << "hello world"; } TEST_F(LogFlagsTest, BacktraceAtNonsense) { absl::SetMinLogLevel(absl::LogSeverityAtLeast::kInfo); - absl::SetFlag(&FLAGS_log_backtrace_at, "gibberish"); absl::ScopedMockLog test_sink(absl::MockLogDefault::kDisallowUnexpected); EXPECT_CALL(test_sink, Send(TextMessage(Not(HasSubstr("(stacktrace:"))))); test_sink.StartCapturingLogs(); + absl::SetFlag(&FLAGS_log_backtrace_at, "gibberish"); LOG(INFO) << "hello world"; } @@ -116,13 +116,13 @@ TEST_F(LogFlagsTest, BacktraceAtWrongFile) { absl::SetMinLogLevel(absl::LogSeverityAtLeast::kInfo); const int log_line = __LINE__ + 1; auto do_log = [] { LOG(INFO) << "hello world"; }; - absl::SetFlag(&FLAGS_log_backtrace_at, - absl::StrCat("some_other_file.cc:", log_line)); absl::ScopedMockLog test_sink(absl::MockLogDefault::kDisallowUnexpected); EXPECT_CALL(test_sink, Send(TextMessage(Not(HasSubstr("(stacktrace:"))))); test_sink.StartCapturingLogs(); + absl::SetFlag(&FLAGS_log_backtrace_at, + absl::StrCat("some_other_file.cc:", log_line)); do_log(); } @@ -130,13 +130,13 @@ TEST_F(LogFlagsTest, BacktraceAtWrongLine) { absl::SetMinLogLevel(absl::LogSeverityAtLeast::kInfo); const int log_line = __LINE__ + 1; auto do_log = [] { LOG(INFO) << "hello world"; }; - absl::SetFlag(&FLAGS_log_backtrace_at, - absl::StrCat("flags_test.cc:", log_line + 1)); absl::ScopedMockLog test_sink(absl::MockLogDefault::kDisallowUnexpected); EXPECT_CALL(test_sink, Send(TextMessage(Not(HasSubstr("(stacktrace:"))))); test_sink.StartCapturingLogs(); + absl::SetFlag(&FLAGS_log_backtrace_at, + absl::StrCat("flags_test.cc:", log_line + 1)); do_log(); } @@ -144,12 +144,12 @@ TEST_F(LogFlagsTest, BacktraceAtWholeFilename) { absl::SetMinLogLevel(absl::LogSeverityAtLeast::kInfo); const int log_line = __LINE__ + 1; auto do_log = [] { LOG(INFO) << "hello world"; }; - absl::SetFlag(&FLAGS_log_backtrace_at, absl::StrCat(__FILE__, ":", log_line)); absl::ScopedMockLog test_sink(absl::MockLogDefault::kDisallowUnexpected); EXPECT_CALL(test_sink, Send(TextMessage(Not(HasSubstr("(stacktrace:"))))); test_sink.StartCapturingLogs(); + absl::SetFlag(&FLAGS_log_backtrace_at, absl::StrCat(__FILE__, ":", log_line)); do_log(); } @@ -157,13 +157,13 @@ TEST_F(LogFlagsTest, BacktraceAtNonmatchingSuffix) { absl::SetMinLogLevel(absl::LogSeverityAtLeast::kInfo); const int log_line = __LINE__ + 1; auto do_log = [] { LOG(INFO) << "hello world"; }; - absl::SetFlag(&FLAGS_log_backtrace_at, - absl::StrCat("flags_test.cc:", log_line, "gibberish")); absl::ScopedMockLog test_sink(absl::MockLogDefault::kDisallowUnexpected); EXPECT_CALL(test_sink, Send(TextMessage(Not(HasSubstr("(stacktrace:"))))); test_sink.StartCapturingLogs(); + absl::SetFlag(&FLAGS_log_backtrace_at, + absl::StrCat("flags_test.cc:", log_line, "gibberish")); do_log(); } @@ -171,13 +171,17 @@ TEST_F(LogFlagsTest, LogsBacktrace) { absl::SetMinLogLevel(absl::LogSeverityAtLeast::kInfo); const int log_line = __LINE__ + 1; auto do_log = [] { LOG(INFO) << "hello world"; }; - absl::SetFlag(&FLAGS_log_backtrace_at, - absl::StrCat("flags_test.cc:", log_line)); absl::ScopedMockLog test_sink(absl::MockLogDefault::kDisallowUnexpected); + testing::InSequence seq; EXPECT_CALL(test_sink, Send(TextMessage(HasSubstr("(stacktrace:")))); + EXPECT_CALL(test_sink, Send(TextMessage(Not(HasSubstr("(stacktrace:"))))); test_sink.StartCapturingLogs(); + absl::SetFlag(&FLAGS_log_backtrace_at, + absl::StrCat("flags_test.cc:", log_line)); + do_log(); + absl::SetFlag(&FLAGS_log_backtrace_at, ""); do_log(); } diff --git a/absl/log/globals.cc b/absl/log/globals.cc index 6dfe81f0..f7c2ec42 100644 --- a/absl/log/globals.cc +++ b/absl/log/globals.cc @@ -123,7 +123,7 @@ namespace log_internal { bool ShouldLogBacktraceAt(absl::string_view file, int line) { const size_t flag_hash = - log_backtrace_at_hash.load(std::memory_order_acquire); + log_backtrace_at_hash.load(std::memory_order_relaxed); return flag_hash != 0 && flag_hash == HashSiteForLogBacktraceAt(file, line); } @@ -132,7 +132,11 @@ bool ShouldLogBacktraceAt(absl::string_view file, int line) { void SetLogBacktraceLocation(absl::string_view file, int line) { log_backtrace_at_hash.store(HashSiteForLogBacktraceAt(file, line), - std::memory_order_release); + std::memory_order_relaxed); +} + +void ClearLogBacktraceLocation() { + log_backtrace_at_hash.store(0, std::memory_order_relaxed); } bool ShouldPrependLogPrefix() { diff --git a/absl/log/globals.h b/absl/log/globals.h index 32b87db0..d56622ae 100644 --- a/absl/log/globals.h +++ b/absl/log/globals.h @@ -110,8 +110,8 @@ class ScopedStderrThreshold final { // Log Backtrace At //------------------------------------------------------------------------------ // -// Users can request backtrace to be logged at specific locations, specified -// by file and line number. +// Users can request an existing `LOG` statement, specified by file and line +// number, to also include a backtrace when logged. // ShouldLogBacktraceAt() // @@ -123,9 +123,16 @@ ABSL_MUST_USE_RESULT bool ShouldLogBacktraceAt(absl::string_view file, // SetLogBacktraceLocation() // -// Sets the location the backtrace should be logged at. +// Sets the location the backtrace should be logged at. If the specified +// location isn't a `LOG` statement, the effect will be the same as +// `ClearLogBacktraceLocation` (but less efficient). void SetLogBacktraceLocation(absl::string_view file, int line); +// ClearLogBacktraceLocation() +// +// Clears the set location so that backtraces will no longer be logged at it. +void ClearLogBacktraceLocation(); + //------------------------------------------------------------------------------ // Prepend Log Prefix //------------------------------------------------------------------------------ -- cgit v1.2.3