From 993ed7d0f769d68cbc6e48c4da5119f6b66ee72d Mon Sep 17 00:00:00 2001 From: Raven Black Date: Wed, 22 Jan 2025 16:26:21 +0000 Subject: [PATCH 1/7] Add stacktrace to ASSERTs Signed-off-by: Raven Black --- source/common/common/assert.h | 3 +++ test/common/common/assert_test.cc | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/source/common/common/assert.h b/source/common/common/assert.h index 6fa4d8d388d05..148a69091f596 100644 --- a/source/common/common/assert.h +++ b/source/common/common/assert.h @@ -135,6 +135,9 @@ void resetEnvoyBugCountersForTest(); ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::assert), critical, \ "assert failure: {}.{}{}", CONDITION_STR, \ details.empty() ? "" : " Details: ", details); \ + Envoy::Assert::EnvoyBugStackTrace st; \ + st.capture(); \ + st.logStackTrace(); \ ACTION; \ } \ } while (false) diff --git a/test/common/common/assert_test.cc b/test/common/common/assert_test.cc index 68587688f6968..7b896118700c3 100644 --- a/test/common/common/assert_test.cc +++ b/test/common/common/assert_test.cc @@ -7,12 +7,16 @@ namespace Envoy { +static void releaseAssertInAFunction() { RELEASE_ASSERT(0, ""); } + TEST(ReleaseAssertDeathTest, VariousLogs) { EXPECT_DEATH({ RELEASE_ASSERT(0, ""); }, ".*assert failure: 0.*"); EXPECT_DEATH({ RELEASE_ASSERT(0, "With some logs"); }, ".*assert failure: 0. Details: With some logs.*"); EXPECT_DEATH({ RELEASE_ASSERT(0 == EAGAIN, fmt::format("using {}", "fmt")); }, ".*assert failure: 0 == EAGAIN. Details: using fmt.*"); + // ensure a stack trace is included. + EXPECT_DEATH({ releaseAssertInAFunction(); }, "releaseAssertInAFunction"); } TEST(AssertDeathTest, VariousLogs) { From 826863b5e03649f616242db50b41fd32570bd7b6 Mon Sep 17 00:00:00 2001 From: Raven Black Date: Wed, 22 Jan 2025 18:12:54 +0000 Subject: [PATCH 2/7] Stack traces don't work when functions get inlined! Signed-off-by: Raven Black --- test/common/common/assert_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/common/assert_test.cc b/test/common/common/assert_test.cc index 7b896118700c3..eec209d6d159e 100644 --- a/test/common/common/assert_test.cc +++ b/test/common/common/assert_test.cc @@ -7,7 +7,7 @@ namespace Envoy { -static void releaseAssertInAFunction() { RELEASE_ASSERT(0, ""); } +static void __attribute__((noinline)) releaseAssertInAFunction() { RELEASE_ASSERT(0, ""); } TEST(ReleaseAssertDeathTest, VariousLogs) { EXPECT_DEATH({ RELEASE_ASSERT(0, ""); }, ".*assert failure: 0.*"); From d7495bd17051b0863e96176af9c5e6f500721d7c Mon Sep 17 00:00:00 2001 From: Raven Black Date: Wed, 22 Jan 2025 20:02:48 +0000 Subject: [PATCH 3/7] Wrangle optimization inlining Signed-off-by: Raven Black --- test/common/common/assert_test.cc | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/test/common/common/assert_test.cc b/test/common/common/assert_test.cc index eec209d6d159e..59ddd9c6f6233 100644 --- a/test/common/common/assert_test.cc +++ b/test/common/common/assert_test.cc @@ -6,8 +6,18 @@ #include "gtest/gtest.h" namespace Envoy { - -static void __attribute__((noinline)) releaseAssertInAFunction() { RELEASE_ASSERT(0, ""); } +namespace { +class ReleaseAsserter { +public: + virtual void releaseAssertInAFunction() PURE; + virtual ~ReleaseAsserter() = default; +}; + +class ReleaseAsserterImpl : public ReleaseAsserter { +public: + void releaseAssertInAFunction() override { RELEASE_ASSERT(0, "") } +}; +} // namespace TEST(ReleaseAssertDeathTest, VariousLogs) { EXPECT_DEATH({ RELEASE_ASSERT(0, ""); }, ".*assert failure: 0.*"); @@ -16,7 +26,14 @@ TEST(ReleaseAssertDeathTest, VariousLogs) { EXPECT_DEATH({ RELEASE_ASSERT(0 == EAGAIN, fmt::format("using {}", "fmt")); }, ".*assert failure: 0 == EAGAIN. Details: using fmt.*"); // ensure a stack trace is included. - EXPECT_DEATH({ releaseAssertInAFunction(); }, "releaseAssertInAFunction"); + // To avoid the call being inlined such that it doesn't make it to the + // stack trace in opt builds, we make it through a messy virtual function. + EXPECT_DEATH( + { + std::unique_ptr asserter = std::make_unique(); + asserter->releaseAssertInAFunction(); + }, + "releaseAssertInAFunction"); } TEST(AssertDeathTest, VariousLogs) { From fbb39dfcbbbf45e6428a4c3a62803e206249cf05 Mon Sep 17 00:00:00 2001 From: Raven Black Date: Wed, 22 Jan 2025 20:52:03 +0000 Subject: [PATCH 4/7] Missing semicolon Signed-off-by: Raven Black --- test/common/common/assert_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/common/assert_test.cc b/test/common/common/assert_test.cc index 59ddd9c6f6233..42ed843bf79e3 100644 --- a/test/common/common/assert_test.cc +++ b/test/common/common/assert_test.cc @@ -15,7 +15,7 @@ class ReleaseAsserter { class ReleaseAsserterImpl : public ReleaseAsserter { public: - void releaseAssertInAFunction() override { RELEASE_ASSERT(0, "") } + void releaseAssertInAFunction() override { RELEASE_ASSERT(0, ""); } }; } // namespace From 2b5553367a70cbffd2668ad8267fbb3196388657 Mon Sep 17 00:00:00 2001 From: Raven Black Date: Fri, 24 Jan 2025 16:02:27 +0000 Subject: [PATCH 5/7] Skip additional test for nondebug builds Signed-off-by: Raven Black --- test/common/common/assert_test.cc | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/test/common/common/assert_test.cc b/test/common/common/assert_test.cc index 42ed843bf79e3..a148da6cf4dce 100644 --- a/test/common/common/assert_test.cc +++ b/test/common/common/assert_test.cc @@ -6,18 +6,8 @@ #include "gtest/gtest.h" namespace Envoy { -namespace { -class ReleaseAsserter { -public: - virtual void releaseAssertInAFunction() PURE; - virtual ~ReleaseAsserter() = default; -}; - -class ReleaseAsserterImpl : public ReleaseAsserter { -public: - void releaseAssertInAFunction() override { RELEASE_ASSERT(0, ""); } -}; -} // namespace + +static void releaseAssertInAFunction() { RELEASE_ASSERT(0, ""); } TEST(ReleaseAssertDeathTest, VariousLogs) { EXPECT_DEATH({ RELEASE_ASSERT(0, ""); }, ".*assert failure: 0.*"); @@ -25,15 +15,13 @@ TEST(ReleaseAssertDeathTest, VariousLogs) { ".*assert failure: 0. Details: With some logs.*"); EXPECT_DEATH({ RELEASE_ASSERT(0 == EAGAIN, fmt::format("using {}", "fmt")); }, ".*assert failure: 0 == EAGAIN. Details: using fmt.*"); - // ensure a stack trace is included. - // To avoid the call being inlined such that it doesn't make it to the - // stack trace in opt builds, we make it through a messy virtual function. - EXPECT_DEATH( - { - std::unique_ptr asserter = std::make_unique(); - asserter->releaseAssertInAFunction(); - }, - "releaseAssertInAFunction"); +} + +TEST(ReleaseAssertDeathTest, AssertIncludesStackTrace) { +#ifdef NDEBUG + GTEST_SKIP() << "optimized build inlines functions so the stack trace won't be reliable"; +#endif + EXPECT_DEATH({ releaseAssertInAFunction(); }, "releaseAssertInAFunction"); } TEST(AssertDeathTest, VariousLogs) { From 10f2ac099176bb8cec99f67f33c0d12b961e5648 Mon Sep 17 00:00:00 2001 From: Raven Black Date: Mon, 27 Jan 2025 20:20:20 +0000 Subject: [PATCH 6/7] Also skip memory sanitizer Signed-off-by: Raven Black --- test/common/common/assert_test.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/common/common/assert_test.cc b/test/common/common/assert_test.cc index a148da6cf4dce..9541416c6ad40 100644 --- a/test/common/common/assert_test.cc +++ b/test/common/common/assert_test.cc @@ -20,6 +20,9 @@ TEST(ReleaseAssertDeathTest, VariousLogs) { TEST(ReleaseAssertDeathTest, AssertIncludesStackTrace) { #ifdef NDEBUG GTEST_SKIP() << "optimized build inlines functions so the stack trace won't be reliable"; +#endif +#if __has_feature(memory_sanitizer) + GTEST_SKIP() << "memory sanitizer build inlines functions so the stack trace won't be reliable"; #endif EXPECT_DEATH({ releaseAssertInAFunction(); }, "releaseAssertInAFunction"); } From 2f0497c03e0baa779fa6c6d4d7c1ee531c71228f Mon Sep 17 00:00:00 2001 From: Raven Black Date: Mon, 27 Jan 2025 20:22:45 +0000 Subject: [PATCH 7/7] Urp Signed-off-by: Raven Black --- test/common/common/assert_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/common/common/assert_test.cc b/test/common/common/assert_test.cc index 9541416c6ad40..82a43825be914 100644 --- a/test/common/common/assert_test.cc +++ b/test/common/common/assert_test.cc @@ -21,8 +21,10 @@ TEST(ReleaseAssertDeathTest, AssertIncludesStackTrace) { #ifdef NDEBUG GTEST_SKIP() << "optimized build inlines functions so the stack trace won't be reliable"; #endif +#if defined(__has_feature) #if __has_feature(memory_sanitizer) GTEST_SKIP() << "memory sanitizer build inlines functions so the stack trace won't be reliable"; +#endif #endif EXPECT_DEATH({ releaseAssertInAFunction(); }, "releaseAssertInAFunction"); }