Moves the generic bug utility to QUICHE. Protected by refactoring, no functional change; not protected. PiperOrigin-RevId: 712628602
diff --git a/build/source_list.bzl b/build/source_list.bzl index 32e4def..ac160d7 100644 --- a/build/source_list.bzl +++ b/build/source_list.bzl
@@ -10,6 +10,8 @@ ] quiche_core_hdrs = [ "common/btree_scheduler.h", + "common/bug_utils.h", + "common/bug_utils_test_helper.h", "common/capsule.h", "common/http/http_header_block.h", "common/http/http_header_storage.h", @@ -409,6 +411,7 @@ "web_transport/web_transport_priority_scheduler.h", ] quiche_core_srcs = [ + "common/bug_utils.cc", "common/capsule.cc", "common/http/http_header_block.cc", "common/http/http_header_storage.cc", @@ -1077,6 +1080,7 @@ "balsa/simple_buffer_test.cc", "binary_http/binary_http_message_test.cc", "common/btree_scheduler_test.cc", + "common/bug_utils_test.cc", "common/capsule_test.cc", "common/http/http_header_block_test.cc", "common/http/http_header_storage_test.cc",
diff --git a/build/source_list.gni b/build/source_list.gni index 3b75a6b..a8bf15a 100644 --- a/build/source_list.gni +++ b/build/source_list.gni
@@ -10,6 +10,8 @@ ] quiche_core_hdrs = [ "src/quiche/common/btree_scheduler.h", + "src/quiche/common/bug_utils.h", + "src/quiche/common/bug_utils_test_helper.h", "src/quiche/common/capsule.h", "src/quiche/common/http/http_header_block.h", "src/quiche/common/http/http_header_storage.h", @@ -409,6 +411,7 @@ "src/quiche/web_transport/web_transport_priority_scheduler.h", ] quiche_core_srcs = [ + "src/quiche/common/bug_utils.cc", "src/quiche/common/capsule.cc", "src/quiche/common/http/http_header_block.cc", "src/quiche/common/http/http_header_storage.cc", @@ -1078,6 +1081,7 @@ "src/quiche/balsa/simple_buffer_test.cc", "src/quiche/binary_http/binary_http_message_test.cc", "src/quiche/common/btree_scheduler_test.cc", + "src/quiche/common/bug_utils_test.cc", "src/quiche/common/capsule_test.cc", "src/quiche/common/http/http_header_block_test.cc", "src/quiche/common/http/http_header_storage_test.cc",
diff --git a/build/source_list.json b/build/source_list.json index 5eb9364..ab414ec 100644 --- a/build/source_list.json +++ b/build/source_list.json
@@ -9,6 +9,8 @@ ], "quiche_core_hdrs": [ "quiche/common/btree_scheduler.h", + "quiche/common/bug_utils.h", + "quiche/common/bug_utils_test_helper.h", "quiche/common/capsule.h", "quiche/common/http/http_header_block.h", "quiche/common/http/http_header_storage.h", @@ -408,6 +410,7 @@ "quiche/web_transport/web_transport_priority_scheduler.h" ], "quiche_core_srcs": [ + "quiche/common/bug_utils.cc", "quiche/common/capsule.cc", "quiche/common/http/http_header_block.cc", "quiche/common/http/http_header_storage.cc", @@ -1077,6 +1080,7 @@ "quiche/balsa/simple_buffer_test.cc", "quiche/binary_http/binary_http_message_test.cc", "quiche/common/btree_scheduler_test.cc", + "quiche/common/bug_utils_test.cc", "quiche/common/capsule_test.cc", "quiche/common/http/http_header_block_test.cc", "quiche/common/http/http_header_storage_test.cc",
diff --git a/quiche/common/bug_utils.cc b/quiche/common/bug_utils.cc new file mode 100644 index 0000000..68bcb6b --- /dev/null +++ b/quiche/common/bug_utils.cc
@@ -0,0 +1,64 @@ +#include "quiche/common/bug_utils.h" + +#include <atomic> +#include <cstdint> +#include <limits> +#include <string> + +#include "absl/base/log_severity.h" +#include "absl/strings/str_cat.h" +#include "absl/strings/string_view.h" + +namespace quiche { +namespace internal { + +GenericBugStreamHandler::GenericBugStreamHandler( + const char* prefix, const char* bug_id, const GenericBugOptions& options) + : bug_id_(bug_id), options_(options) { + if (options_.condition_str.empty()) { + absl::StrAppend(&str_, prefix, "(", bug_id, "): "); + } else { + absl::StrAppend(&str_, prefix, "_IF(", bug_id, ", ", options_.condition_str, + "): "); + } +} + +GenericBugStreamHandler::~GenericBugStreamHandler() { + GenericBugStreamHandler::OverrideFunction override_function = + GetOverrideFunction(); + if (options_.bug_listener != nullptr) { + options_.bug_listener->OnBug(bug_id_, options_.file_name, options_.line, + str_); + } + if (override_function != nullptr) { + override_function(options_.severity, options_.file_name, options_.line, + str_); + } +} + +// static +void GenericBugStreamHandler::SetOverrideFunction( + GenericBugStreamHandler::OverrideFunction override_function) { + atomic_override_function_.store(override_function); +} + +// static +GenericBugStreamHandler::OverrideFunction +GenericBugStreamHandler::GetOverrideFunction() { + return atomic_override_function_.load(std::memory_order_relaxed); +} + +// static +std::atomic<GenericBugStreamHandler::OverrideFunction> + GenericBugStreamHandler::atomic_override_function_ = nullptr; + +void GenericBugWithoutLog(const char* bug_id, + const GenericBugOptions& options) { + if (options.bug_listener != nullptr) { + options.bug_listener->OnBug(bug_id, options.file_name, options.line, + /*No bug message*/ ""); + } +} + +} // namespace internal +} // namespace quiche
diff --git a/quiche/common/bug_utils.h b/quiche/common/bug_utils.h new file mode 100644 index 0000000..ea6fbf8 --- /dev/null +++ b/quiche/common/bug_utils.h
@@ -0,0 +1,151 @@ +#ifndef QUICHE_COMMON_BUG_UTILS_H_ +#define QUICHE_COMMON_BUG_UTILS_H_ + +// This file contains macros for emitting bug log events when invariants are +// violated. +// +// Each instance of a QUICHE_BUG and friends has an associated id, which can be +// helpful for quickly finding the associated source code. +// +// The IDs are free form, but are expected to be unique. Best practice is to +// provide a *short* description of the condition being detected, without +// quotes, e.g., +// +// QUICHE_BUG(http2_decoder_invalid_parse_state) << "..."; +// +// QUICHE_BUG is defined in platform/api/quiche_bug_tracker.h. +// + +#include <atomic> +#include <cstdint> +#include <sstream> +#include <string> +#include <tuple> +#include <type_traits> + +#include "absl/base/log_severity.h" +#include "absl/strings/str_cat.h" +#include "absl/strings/string_view.h" +#include "quiche/common/platform/api/quiche_export.h" + +namespace quiche { +namespace internal { + +QUICHE_EXPORT class GenericBugListener { + public: + virtual ~GenericBugListener() = default; + virtual void OnBug(const char* bug_id, const char* file, int line, + absl::string_view bug_message) = 0; +}; + +QUICHE_NO_EXPORT struct GenericBugOptions { + explicit GenericBugOptions(absl::LogSeverity log_severity, + const char* file_name, int line) + : severity(log_severity), file_name(file_name), line(line) {} + + GenericBugOptions& SetCondition(absl::string_view condition) { + this->condition_str = condition; + return *this; + } + + GenericBugOptions& SetBugListener(GenericBugListener* listener) { + this->bug_listener = listener; + return *this; + } + + absl::LogSeverity severity; + const char* const file_name; + const int line; + // !empty() for conditional GENERIC_BUGs. + absl::string_view condition_str; + // If not nullptr, |bug_listener| will be notified of this GENERIC_BUG hit. + // Since GenericBugListener may be a temporary object, this is only safe to + // access from GenericBugStreamHandler, whose scope is strictly narrower. + GenericBugListener* bug_listener = nullptr; +}; + +QUICHE_EXPORT inline GenericBugOptions DefaultBugOptions(const char* file_name, + int line) { + return GenericBugOptions(absl::kLogDebugFatal, file_name, line); +} + +// Called if a GENERIC_BUG is hit, but logging is omitted. +QUICHE_EXPORT void GenericBugWithoutLog(const char* bug_id, + const GenericBugOptions& options); + +// GenericBugStreamHandler exposes an interface similar to Abseil log objects, +// and is used by GENERIC_BUG to trigger a function which can be overridden in +// tests. By default, this class performs no action. SetOverrideFunction must be +// called to accomplish anything interesting. +class QUICHE_EXPORT GenericBugStreamHandler { + public: + // |prefix| and |bug_id| must be literal strings. They are used in the + // destructor. + explicit GenericBugStreamHandler(const char* prefix, const char* bug_id, + const GenericBugOptions& options); + + ~GenericBugStreamHandler(); + + template <typename T, + std::enable_if_t<absl::HasAbslStringify<T>::value, bool> = true> + QUICHE_EXPORT GenericBugStreamHandler& operator<<(const T& v) { + absl::StrAppend(&str_, v); + return *this; + } + + // For types that support only operator<<. There's a better solution in + // Abseil, but unfortunately OStringStream is in a namespace marked internal. + template <typename T, + std::enable_if_t<!absl::HasAbslStringify<T>::value, bool> = true> + QUICHE_EXPORT GenericBugStreamHandler& operator<<(const T& v) { + absl::StrAppend(&str_, (std::ostringstream() << v).view()); + return *this; + } + + // Interface similar to Abseil log objects. + GenericBugStreamHandler& stream() { return *this; } + + using OverrideFunction = void (*)(absl::LogSeverity severity, + const char* file, int line, + absl::string_view log_message); + + // Allows overriding the internal implementation. Call with nullptr to make + // this class a no-op. This getter and setter are thread-safe. + static void SetOverrideFunction(OverrideFunction override_function); + static OverrideFunction GetOverrideFunction(); + + private: + static std::atomic<OverrideFunction> atomic_override_function_; + + const char* bug_id_; + std::string str_; + const GenericBugOptions& options_; +}; + +} // namespace internal +} // namespace quiche + +// The GNU compiler emits a warning for code like: +// +// if (foo) +// if (bar) { } else baz; +// +// because it thinks you might want the else to bind to the first if. This +// leads to problems with code like: +// +// if (do_expr) GENERIC_BUG(bug_id) << "Some message"; +// +// The "switch (0) case 0:" idiom is used to suppress this. +#define GENERIC_BUG_UNBRACED_ELSE_BLOCKER \ + switch (0) \ + case 0: \ + default: + +#define GENERIC_BUG_IMPL(prefix, bug_id, skip_log_condition, options) \ + if (skip_log_condition) { \ + ::quiche::internal::GenericBugWithoutLog(#bug_id, (options)); \ + } else /* NOLINT */ \ + ::quiche::internal::GenericBugStreamHandler(prefix, #bug_id, (options)) \ + .stream() + +#endif // QUICHE_COMMON_BUG_UTILS_H_
diff --git a/quiche/common/bug_utils_test.cc b/quiche/common/bug_utils_test.cc new file mode 100644 index 0000000..0bddb0d --- /dev/null +++ b/quiche/common/bug_utils_test.cc
@@ -0,0 +1,201 @@ +#include "quiche/common/bug_utils.h" + +#include <string.h> + +#include <algorithm> +#include <memory> +#include <string> + +#include "absl/base/log_severity.h" +#include "absl/strings/string_view.h" +#include "quiche/common/bug_utils_test_helper.h" +#include "quiche/common/platform/api/quiche_test.h" + +namespace quiche { +namespace internal { +namespace { + +using ::testing::_; +using ::testing::AnyNumber; +using ::testing::EndsWith; +using ::testing::InSequence; + +class BugHandler { + public: + virtual ~BugHandler() = default; + virtual void OnBug(absl::string_view file, int line, + absl::string_view message) = 0; +}; + +// This class provides a convenient way to write expectations for the bug +// override function. +class MockBugHandler : public BugHandler { + public: + MockBugHandler() = default; + + MOCK_METHOD(void, OnBug, + (absl::string_view file, int line, absl::string_view message), + (override)); +}; + +MockBugHandler* mock_bug_handler = nullptr; + +MockBugHandler* GetInstance() { + if (mock_bug_handler == nullptr) { + mock_bug_handler = new MockBugHandler; + } + return mock_bug_handler; +} + +void ResetInstance() { + delete mock_bug_handler; + mock_bug_handler = nullptr; +} + +class BugUtilsTest : public ::quiche::test::QuicheTest { + public: + void SetUp() override { + fn_ = GenericBugStreamHandler::GetOverrideFunction(); + GenericBugStreamHandler::SetOverrideFunction( + [](absl::LogSeverity /*severity*/, const char* file, int line, + absl::string_view log_message) { + GetInstance()->OnBug(absl::string_view(file, strlen(file)), line, + log_message); + }); + } + + void TearDown() override { + GenericBugStreamHandler::SetOverrideFunction(fn_); + ResetInstance(); + } + + inline static GenericBugStreamHandler::OverrideFunction fn_ = nullptr; +}; + +// Tests several permutations. +TEST_F(BugUtilsTest, TestsEverythingUsing23And26) { + InSequence seq; + EXPECT_CALL(*GetInstance(), OnBug(EndsWith("bug_utils_test_helper.h"), 23, + EndsWith("Here on line 23"))); + LogBugLine23(); + + EXPECT_CALL(*GetInstance(), OnBug(EndsWith("bug_utils_test_helper.h"), 23, + EndsWith("Here on line 23"))); + LogBugLine23(); + + EXPECT_CALL(*GetInstance(), OnBug(EndsWith("bug_utils_test_helper.h"), 26, + EndsWith("Here on line 26"))); + EXPECT_CALL(*GetInstance(), OnBug(EndsWith("bug_utils_test_helper.h"), 27, + EndsWith("And 27!"))); + LogBugLine26(); +} + +TEST_F(BugUtilsTest, TestBugIf) { + InSequence seq; + + // Verify that we don't invoke the function for a false condition. + LogIfBugLine31(false); + + // The first true should trigger an invocation. + EXPECT_CALL(*GetInstance(), OnBug(EndsWith("bug_utils_test_helper.h"), 31, + EndsWith("Here on line 31"))); + LogIfBugLine31(true); + + // It's always a no-op if the condition is false. + LogIfBugLine31(false); // no-op + LogIfBugLine31(false); // no-op +} + +TEST_F(BugUtilsTest, TestBugIfMessage) { + int i; + + // Check success + LogIfBugNullCheckLine35(&i); + + // Check failure + EXPECT_CALL( + *GetInstance(), + OnBug( + EndsWith("bug_utils_test_helper.h"), 35, + EndsWith( + "QUICHE_TEST_BUG_IF(Bug 35, ptr == nullptr): Here on line 35"))); + LogIfBugNullCheckLine35(nullptr); +} + +// Don't actually need to crash, just cause a side effect the test can assert +// on. +int num_times_called = 0; +bool BadCondition() { + ++num_times_called; + return true; +} + +TEST_F(BugUtilsTest, BadCondition) { + InSequence seq; + + EXPECT_EQ(num_times_called, 0); + + EXPECT_CALL(*GetInstance(), OnBug(_, _, EndsWith("Called BadCondition"))); + QUICHE_TEST_BUG_IF(id, BadCondition()) << "Called BadCondition"; + EXPECT_EQ(num_times_called, 1); +} + +TEST_F(BugUtilsTest, NoDanglingElse) { + auto unexpected_bug_message = [] { + ADD_FAILURE() << "This should not be called"; + return "bad"; + }; + + if (false) QUICHE_TEST_BUG(dangling_else) << unexpected_bug_message(); + + bool expected_else_reached = false; + if (false) + QUICHE_TEST_BUG(dangling_else_2) << unexpected_bug_message(); + else + expected_else_reached = true; + + EXPECT_TRUE(expected_else_reached); +} + +TEST_F(BugUtilsTest, BugListener) { + class TestListener : public GenericBugListener { + public: + explicit TestListener(bool expect_log_message) + : expect_log_message_(expect_log_message) {} + + ~TestListener() override { EXPECT_EQ(hit_count_, 1); } + + void OnBug(const char* bug_id, const char* file, int line, + absl::string_view bug_message) override { + ++hit_count_; + EXPECT_EQ(bug_id, "bug_listener_test"); + EXPECT_EQ(file, __FILE__); + EXPECT_GT(line, 0); + if (expect_log_message_) { + EXPECT_EQ(bug_message, "TEST_BUG(bug_listener_test): Bug listener msg"); + } else { + EXPECT_EQ(bug_message, ""); + } + } + + TestListener* self() { return this; } + + private: + int hit_count_ = 0; + const bool expect_log_message_; + }; + + GENERIC_BUG_IMPL("TEST_BUG", bug_listener_test, /*skip_log_condition=*/false, + QUICHE_TEST_BUG_OPTIONS().SetBugListener( + TestListener(/*expect_log_message=*/true).self())) + << "Bug listener msg"; + + GENERIC_BUG_IMPL("TEST_BUG", bug_listener_test, /*skip_log_condition=*/true, + QUICHE_TEST_BUG_OPTIONS().SetBugListener( + TestListener(/*expect_log_message=*/false).self())) + << "Bug listener msg"; +} + +} // namespace +} // namespace internal +} // namespace quiche
diff --git a/quiche/common/bug_utils_test_helper.h b/quiche/common/bug_utils_test_helper.h new file mode 100644 index 0000000..6497bca --- /dev/null +++ b/quiche/common/bug_utils_test_helper.h
@@ -0,0 +1,41 @@ +#ifndef QUICHE_COMMON_BUG_UTILS_TEST_HELPER_H_ +#define QUICHE_COMMON_BUG_UTILS_TEST_HELPER_H_ + +#include "quiche/common/bug_utils.h" + +// Sticking various logging functions used by the test in a separate file, +// so their line numbers are unlikely to change as we modify the test file +// itself, as the expectations we set take the file + line numbers into account. + +#define QUICHE_TEST_BUG(bug_id) \ + GENERIC_BUG_UNBRACED_ELSE_BLOCKER \ + GENERIC_BUG_IMPL("QUICHE_TEST_BUG", bug_id, false, \ + ::quiche::internal::DefaultBugOptions(__FILE__, __LINE__)) + +#define QUICHE_TEST_BUG_IF(bug_id, condition) \ + GENERIC_BUG_UNBRACED_ELSE_BLOCKER \ + if (!(condition)) { /* Do nothing */ \ + } else /* NOLINT */ \ + GENERIC_BUG_IMPL("QUICHE_TEST_BUG", bug_id, false, \ + ::quiche::internal::DefaultBugOptions(__FILE__, __LINE__) \ + .SetCondition(#condition)) + +inline void LogBugLine23() { QUICHE_TEST_BUG(Bug 23) << "Here on line 23"; } + +inline void LogBugLine26() { + QUICHE_TEST_BUG(Bug 26) << "Here on line 26"; + QUICHE_TEST_BUG(Bug 27) << "And 27!"; +} + +inline void LogIfBugLine31(bool condition) { + QUICHE_TEST_BUG_IF(Bug 31, condition) << "Here on line 31"; +} + +inline void LogIfBugNullCheckLine35(int *ptr) { + QUICHE_TEST_BUG_IF(Bug 35, ptr == nullptr) << "Here on line 35"; +} + +#define QUICHE_TEST_BUG_OPTIONS() \ + ::quiche::internal::DefaultBugOptions(__FILE__, __LINE__) + +#endif // QUICHE_COMMON_BUG_UTILS_TEST_HELPER_H_