Automated g4 rollback of changelist 712628602. *** Reason for rollback *** Breaks TAP project vr.c9.unit.android.x86 *** Original change description *** Moves the generic bug utility to QUICHE. *** PiperOrigin-RevId: 713678123
diff --git a/build/source_list.bzl b/build/source_list.bzl index ac160d7..32e4def 100644 --- a/build/source_list.bzl +++ b/build/source_list.bzl
@@ -10,8 +10,6 @@ ] 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", @@ -411,7 +409,6 @@ "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", @@ -1080,7 +1077,6 @@ "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 a8bf15a..3b75a6b 100644 --- a/build/source_list.gni +++ b/build/source_list.gni
@@ -10,8 +10,6 @@ ] 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", @@ -411,7 +409,6 @@ "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", @@ -1081,7 +1078,6 @@ "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 ab414ec..5eb9364 100644 --- a/build/source_list.json +++ b/build/source_list.json
@@ -9,8 +9,6 @@ ], "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", @@ -410,7 +408,6 @@ "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", @@ -1080,7 +1077,6 @@ "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 deleted file mode 100644 index 68bcb6b..0000000 --- a/quiche/common/bug_utils.cc +++ /dev/null
@@ -1,64 +0,0 @@ -#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 deleted file mode 100644 index ea6fbf8..0000000 --- a/quiche/common/bug_utils.h +++ /dev/null
@@ -1,151 +0,0 @@ -#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 deleted file mode 100644 index 0bddb0d..0000000 --- a/quiche/common/bug_utils_test.cc +++ /dev/null
@@ -1,201 +0,0 @@ -#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 deleted file mode 100644 index 6497bca..0000000 --- a/quiche/common/bug_utils_test_helper.h +++ /dev/null
@@ -1,41 +0,0 @@ -#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_