From 8aaea6c092bb27dac8772e776c78849ef7e02177 Mon Sep 17 00:00:00 2001 From: Matthew Glazar Date: Wed, 9 Aug 2023 20:21:00 -0700 Subject: [PATCH 1/2] Split OsStackTraceGetter into AbslStackTraceGetter + NoopStackTraceGetter The OsStackTraceGetter class does different things depending on whether GTEST_HAS_ABSL is set. I want to add another implementation of OsStackTraceGetter, and I think it's ugly if my new implementation litters OsStackTraceGetter with more #if-s. Split OsStackTraceGetter into two different classes: * AbslStackTraceGetter, which is only defined if GTEST_HAS_ABSL is set. This class behaves the same as OsStackTraceGetter currently behaves. * NoopStackTraceGetter, which is always defined. This class behaves the same as OsStackTraceGetter currently behaves if GTEST_HAS_ABSL is not set. After this refactor, it is easier to add new OsStackTraceGetter backend classes. --- googletest/src/gtest-internal-inl.h | 30 ++++++++++++++++++---------- googletest/src/gtest.cc | 31 ++++++++++++++++++----------- 2 files changed, 39 insertions(+), 22 deletions(-) diff --git a/googletest/src/gtest-internal-inl.h b/googletest/src/gtest-internal-inl.h index 5b7fcbd0..5b15e1a3 100644 --- a/googletest/src/gtest-internal-inl.h +++ b/googletest/src/gtest-internal-inl.h @@ -433,16 +433,16 @@ class OsStackTraceGetterInterface { delete; }; +#ifdef GTEST_HAS_ABSL // A working implementation of the OsStackTraceGetterInterface interface. -class OsStackTraceGetter : public OsStackTraceGetterInterface { +class AbslStackTraceGetter : public OsStackTraceGetterInterface { public: - OsStackTraceGetter() = default; + AbslStackTraceGetter() = default; std::string CurrentStackTrace(int max_depth, int skip_count) override; void UponLeavingGTest() override; private: -#ifdef GTEST_HAS_ABSL Mutex mutex_; // Protects all internal state. // We save the stack frame below the frame that calls user code. @@ -450,10 +450,20 @@ class OsStackTraceGetter : public OsStackTraceGetterInterface { // the user code changes between the call to UponLeavingGTest() // and any calls to the stack trace code from within the user code. void* caller_frame_ = nullptr; + + AbslStackTraceGetter(const AbslStackTraceGetter&) = delete; + AbslStackTraceGetter& operator=(const AbslStackTraceGetter&) = delete; +}; #endif // GTEST_HAS_ABSL - OsStackTraceGetter(const OsStackTraceGetter&) = delete; - OsStackTraceGetter& operator=(const OsStackTraceGetter&) = delete; +// An implementation of OsStackTraceGetterInterface which returns no +// stack trace. +class NoopStackTraceGetter : public OsStackTraceGetterInterface { + public: + NoopStackTraceGetter() = default; + + std::string CurrentStackTrace(int max_depth, int skip_count) override; + void UponLeavingGTest() override; }; // Information about a Google Test trace point. @@ -622,8 +632,8 @@ class GTEST_API_ UnitTestImpl { void set_os_stack_trace_getter(OsStackTraceGetterInterface* getter); // Returns the current OS stack trace getter if it is not NULL; - // otherwise, creates an OsStackTraceGetter, makes it the current - // getter, and returns it. + // otherwise, creates an OsStackTraceGetterInterface, makes it the + // current getter, and returns it. OsStackTraceGetterInterface* os_stack_trace_getter(); // Returns the current OS stack trace as an std::string. @@ -922,9 +932,9 @@ class GTEST_API_ UnitTestImpl { TestEventListeners listeners_; // The OS stack trace getter. Will be deleted when the UnitTest - // object is destructed. By default, an OsStackTraceGetter is used, - // but the user can set this field to use a custom getter if that is - // desired. + // object is destructed. By default, an OsStackTraceGetterInterface + // is created when os_stack_trace_getter is first called, but the user + // can set this field to use a custom getter if that is desired. OsStackTraceGetterInterface* os_stack_trace_getter_; // True if and only if PostFlagParsingInit() has been called. diff --git a/googletest/src/gtest.cc b/googletest/src/gtest.cc index 30a5cc3f..1bfd95dc 100644 --- a/googletest/src/gtest.cc +++ b/googletest/src/gtest.cc @@ -4968,9 +4968,9 @@ void StreamingListener::SocketWriter::MakeConnection() { const char* const OsStackTraceGetterInterface::kElidedFramesMarker = "... " GTEST_NAME_ " internal frames ..."; -std::string OsStackTraceGetter::CurrentStackTrace(int max_depth, int skip_count) - GTEST_LOCK_EXCLUDED_(mutex_) { #ifdef GTEST_HAS_ABSL +std::string AbslStackTraceGetter::CurrentStackTrace(int max_depth, int skip_count) + GTEST_LOCK_EXCLUDED_(mutex_) { std::string result; if (max_depth <= 0) { @@ -5010,16 +5010,11 @@ std::string OsStackTraceGetter::CurrentStackTrace(int max_depth, int skip_count) } return result; - -#else // !GTEST_HAS_ABSL - static_cast(max_depth); - static_cast(skip_count); - return ""; -#endif // GTEST_HAS_ABSL } +#endif // GTEST_HAS_ABSL -void OsStackTraceGetter::UponLeavingGTest() GTEST_LOCK_EXCLUDED_(mutex_) { #ifdef GTEST_HAS_ABSL +void AbslStackTraceGetter::UponLeavingGTest() GTEST_LOCK_EXCLUDED_(mutex_) { void* caller_frame = nullptr; if (absl::GetStackTrace(&caller_frame, 1, 3) <= 0) { caller_frame = nullptr; @@ -5027,7 +5022,17 @@ void OsStackTraceGetter::UponLeavingGTest() GTEST_LOCK_EXCLUDED_(mutex_) { MutexLock lock(&mutex_); caller_frame_ = caller_frame; +} #endif // GTEST_HAS_ABSL + +std::string NoopStackTraceGetter::CurrentStackTrace(int max_depth, int skip_count) { + static_cast(max_depth); + static_cast(skip_count); + return ""; +} + +void NoopStackTraceGetter::UponLeavingGTest() { + // Do nothing. } #ifdef GTEST_HAS_DEATH_TEST @@ -6233,14 +6238,16 @@ void UnitTestImpl::set_os_stack_trace_getter( } // Returns the current OS stack trace getter if it is not NULL; -// otherwise, creates an OsStackTraceGetter, makes it the current -// getter, and returns it. +// otherwise, creates an OsStackTraceGetterInterface, makes it the +// current getter, and returns it. OsStackTraceGetterInterface* UnitTestImpl::os_stack_trace_getter() { if (os_stack_trace_getter_ == nullptr) { #ifdef GTEST_OS_STACK_TRACE_GETTER_ os_stack_trace_getter_ = new GTEST_OS_STACK_TRACE_GETTER_; +#elif defined(GTEST_HAS_ABSL) + os_stack_trace_getter_ = new AbslStackTraceGetter; #else - os_stack_trace_getter_ = new OsStackTraceGetter; + os_stack_trace_getter_ = new NoopStackTraceGetter; #endif // GTEST_OS_STACK_TRACE_GETTER_ } From dd45329f01818ae7eb5fd637a9d5d325edc96c98 Mon Sep 17 00:00:00 2001 From: Matthew Glazar Date: Wed, 9 Aug 2023 20:39:34 -0700 Subject: [PATCH 2/2] Print stacktraces using std::stacktrace if available If std::stacktrace is available (currently only GCC (opt-in feature) and MSVC), use it for printing stack traces. If both Abseil and std::stacktrace are available, prefer std::stacktrace. Advantages of std::stacktrace over Abseil: * Includes file and line information * Includes DLL information * Better stack trace quality on Windows Disadvantages of std::stacktrace compared to Abseil: * Runs more slowly on Windows * Includes information which might not be relevant such as byte offsets * Printed lines are longer thus harder to skim Before (Windows using Abseil): [ RUN ] FooTest.Xyz [snip]\googletest-filter-unittest_.cc(49): error: Failed Expected failure. Stack trace: 00007FF69EF90E8D: testing::internal::HandleSehExceptionsInMethodIfSupported 00007FF69EF90AC3: testing::internal::HandleExceptionsInMethodIfSupported 00007FF69EF5C01B: testing::Test::Run 00007FF69EF5CCC5: testing::TestInfo::Run 00007FF69EF5D6EC: testing::TestSuite::Run ... Google Test internal frames ... [ FAILED ] FooTest.Xyz (21 ms) After (Windows using MSVC STL's std::stacktrace): [ RUN ] FooTest.Xyz [snip]\googletest-filter-unittest_.cc(49): error: Failed Expected failure. Stack trace: 00007FF6770283A9: googletest_filter_unittest_!`anonymous namespace'::FooTest_Xyz_Test::TestBody+0x89 [snip]\googletest-filter-unittest_.cc:49 00007FF677080E8D: googletest_filter_unittest_!testing::internal::HandleSehExceptionsInMethodIfSupported+0x1D [snip]\gtest.cc:2609 00007FF677080AC3: googletest_filter_unittest_!testing::internal::HandleExceptionsInMethodIfSupported+0x73 [snip]\gtest.cc:2652 00007FF67704C01B: googletest_filter_unittest_!testing::Test::Run+0xAB [snip]\gtest.cc:2698 ... Google Test internal frames ... [ FAILED ] FooTest.Xyz (127 ms) --- .../include/gtest/internal/gtest-port.h | 9 +++ googletest/src/gtest-internal-inl.h | 27 ++++++++ googletest/src/gtest.cc | 64 +++++++++++++++++++ 3 files changed, 100 insertions(+) diff --git a/googletest/include/gtest/internal/gtest-port.h b/googletest/include/gtest/internal/gtest-port.h index b887e24e..bf6e3653 100644 --- a/googletest/include/gtest/internal/gtest-port.h +++ b/googletest/include/gtest/internal/gtest-port.h @@ -216,6 +216,7 @@ // specializations. Always defined to 0 or 1. // GTEST_USE_OWN_FLAGFILE_FLAG_ - Always defined to 0 or 1. // GTEST_HAS_CXXABI_H_ - Always defined to 0 or 1. +// GTEST_HAS_STD_STACKTRACE_ - Always defined to 0 or 1. // GTEST_CAN_STREAM_RESULTS_ - Always defined to 0 or 1. // GTEST_HAS_ALT_PATH_SEP_ - Always defined to 0 or 1. // GTEST_WIDE_STRING_USES_UTF16_ - Always defined to 0 or 1. @@ -873,6 +874,14 @@ typedef struct _RTL_CRITICAL_SECTION GTEST_CRITICAL_SECTION; #endif #endif +#if !defined(GTEST_HAS_STD_STACKTRACE_) +#if defined(__cpp_lib_stacktrace) && __cpp_lib_stacktrace >= 202011L +#define GTEST_HAS_STD_STACKTRACE_ 1 +#else +#define GTEST_HAS_STD_STACKTRACE_ 0 +#endif +#endif + // A function level attribute to disable checking for use of uninitialized // memory when built with MemorySanitizer. #if GTEST_HAVE_ATTRIBUTE_(no_sanitize_memory) diff --git a/googletest/src/gtest-internal-inl.h b/googletest/src/gtest-internal-inl.h index 5b15e1a3..8cd647f2 100644 --- a/googletest/src/gtest-internal-inl.h +++ b/googletest/src/gtest-internal-inl.h @@ -59,6 +59,10 @@ #include // NOLINT #endif // GTEST_OS_WINDOWS +#if GTEST_HAS_STD_STACKTRACE_ +#include +#endif + #include "gtest/gtest-spi.h" #include "gtest/gtest.h" @@ -456,6 +460,29 @@ class AbslStackTraceGetter : public OsStackTraceGetterInterface { }; #endif // GTEST_HAS_ABSL +#if GTEST_HAS_STD_STACKTRACE_ +// A working implementation of the OsStackTraceGetterInterface interface. +class StdStackTraceGetter : public OsStackTraceGetterInterface { + public: + StdStackTraceGetter() = default; + + std::string CurrentStackTrace(int max_depth, int skip_count) override; + void UponLeavingGTest() override; + + private: + Mutex mutex_; // Protects all internal state. + + // We save the stack frame below the frame that calls user code. + // We do this because the address of the frame immediately below + // the user code changes between the call to UponLeavingGTest() + // and any calls to the stack trace code from within the user code. + std::stacktrace_entry::native_handle_type caller_frame_ = {}; + + StdStackTraceGetter(const StdStackTraceGetter&) = delete; + StdStackTraceGetter& operator=(const StdStackTraceGetter&) = delete; +}; +#endif // GTEST_HAS_STD_STACKTRACE_ + // An implementation of OsStackTraceGetterInterface which returns no // stack trace. class NoopStackTraceGetter : public OsStackTraceGetterInterface { diff --git a/googletest/src/gtest.cc b/googletest/src/gtest.cc index 1bfd95dc..8865b022 100644 --- a/googletest/src/gtest.cc +++ b/googletest/src/gtest.cc @@ -153,6 +153,10 @@ #include "absl/strings/strip.h" #endif // GTEST_HAS_ABSL +#if GTEST_HAS_STD_STACKTRACE_ +#include +#endif + // Checks builtin compiler feature |x| while avoiding an extra layer of #ifdefs // at the callsite. #if defined(__has_builtin) @@ -5025,6 +5029,64 @@ void AbslStackTraceGetter::UponLeavingGTest() GTEST_LOCK_EXCLUDED_(mutex_) { } #endif // GTEST_HAS_ABSL +#if GTEST_HAS_STD_STACKTRACE_ +std::string StdStackTraceGetter::CurrentStackTrace(int max_depth, int skip_count) + GTEST_LOCK_EXCLUDED_(mutex_) { + if (max_depth <= 0) { + return ""; + } + + max_depth = std::min(max_depth, kMaxStackTraceDepth); + // Skips the frames requested by the caller, plus this function. + skip_count += 1; + + std::stacktrace stack = std::stacktrace::current(skip_count, max_depth); + + std::stacktrace_entry::native_handle_type caller_frame = {}; + { + MutexLock lock(&mutex_); + caller_frame = caller_frame_; + } + + std::ostringstream result; + for (const std::stacktrace_entry &entry : stack) { + if (entry.native_handle() == caller_frame && + !GTEST_FLAG_GET(show_internal_stack_frames)) { + // Add a marker to the trace and stop adding frames. + result << kElidedFramesMarker << '\n'; + break; + } + + result << " " << entry.native_handle() << ": "; + std::string description = entry.description(); + if (description.empty()) { + result << "(unknown)"; + } else { + result << description; + } + std::string source_file = entry.source_file(); + if (!source_file.empty()) { + result << ' ' << source_file << ':' << entry.source_line(); + } + result << '\n'; + } + return std::move(result).str(); +} +#endif // GTEST_HAS_ABSL + +#if GTEST_HAS_STD_STACKTRACE_ +void StdStackTraceGetter::UponLeavingGTest() GTEST_LOCK_EXCLUDED_(mutex_) { + std::stacktrace stack = std::stacktrace::current(/*skip=*/2, /*max_depth=*/1); + std::stacktrace_entry::native_handle_type caller_frame = {}; + if (!stack.empty()) { + caller_frame = stack[0].native_handle(); + } + + MutexLock lock(&mutex_); + caller_frame_ = caller_frame; +} +#endif // GTEST_HAS_ABSL + std::string NoopStackTraceGetter::CurrentStackTrace(int max_depth, int skip_count) { static_cast(max_depth); static_cast(skip_count); @@ -6244,6 +6306,8 @@ OsStackTraceGetterInterface* UnitTestImpl::os_stack_trace_getter() { if (os_stack_trace_getter_ == nullptr) { #ifdef GTEST_OS_STACK_TRACE_GETTER_ os_stack_trace_getter_ = new GTEST_OS_STACK_TRACE_GETTER_; +#elif GTEST_HAS_STD_STACKTRACE_ + os_stack_trace_getter_ = new StdStackTraceGetter; #elif defined(GTEST_HAS_ABSL) os_stack_trace_getter_ = new AbslStackTraceGetter; #else