Skip to content

Commit

Permalink
Patch gtest so that it terminates if a ScopedTrace is destroyed on th…
Browse files Browse the repository at this point in the history
…read different than where it was destroyed

gtest ScopedTrace/SCOPED_TRACE uses thread local stacks to keep track of traces. When combined with pika
tasks that may yield and be stolen onto other worker threads, destroying a ScopedTrace on a different
thread can segfault, cause hangs, or simply corrupt data. Instead of allowing that to happen every now
and then we instead patch gtest to check if this happens and terminate as soon as it's detected. Use of
ScopedTrace should be avoided in situations where tasks can yield. This patch does not fix the issue, but
helps recognizing when e.g. a segfault is the cause of "only" improper use of ScopedTrace or if it is a
result of other algorithmic or implementation issues.
  • Loading branch information
msimberg committed Jan 29, 2025
1 parent eea5bd2 commit 80b1b40
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 1 deletion.
4 changes: 3 additions & 1 deletion external/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ FetchContent_Declare(
GIT_REPOSITORY https://github.com/google/googletest.git
GIT_TAG v1.13.0
# Patch for int to float conversion warning.
PATCH_COMMAND git cherry-pick -n 5cd81a7848203d3df6959c148eb91f1a4ff32c1d -m 1
PATCH_COMMAND
git reset --hard HEAD && git cherry-pick -n 5cd81a7848203d3df6959c148eb91f1a4ff32c1d -m 1 && git
apply "${CMAKE_CURRENT_SOURCE_DIR}/scoped_trace_terminate_on_thread_migrate.patch"
)
set(BUILD_GMOCK OFF CACHE INTERNAL "")
set(INSTALL_GTEST OFF CACHE INTERNAL "")
Expand Down
76 changes: 76 additions & 0 deletions external/scoped_trace_terminate_on_thread_migrate.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
diff --git a/googletest/include/gtest/gtest.h b/googletest/include/gtest/gtest.h
index 3e452a50..c51dd607 100644
--- a/googletest/include/gtest/gtest.h
+++ b/googletest/include/gtest/gtest.h
@@ -58,6 +58,7 @@
#include <set>
#include <sstream>
#include <string>
+#include <thread>
#include <type_traits>
#include <vector>

@@ -2056,15 +2057,18 @@ class GTEST_API_ ScopedTrace {
// Slow, but flexible.
template <typename T>
ScopedTrace(const char* file, int line, const T& message) {
+ id = std::this_thread::get_id();
PushTrace(file, line, (Message() << message).GetString());
}

// Optimize for some known types.
ScopedTrace(const char* file, int line, const char* message) {
+ id = std::this_thread::get_id();
PushTrace(file, line, message ? message : "(null)");
}

ScopedTrace(const char* file, int line, const std::string& message) {
+ id = std::this_thread::get_id();
PushTrace(file, line, message);
}

@@ -2079,6 +2083,8 @@ class GTEST_API_ ScopedTrace {

ScopedTrace(const ScopedTrace&) = delete;
ScopedTrace& operator=(const ScopedTrace&) = delete;
+
+ std::thread::id id;
};

// Causes a trace (including the source file path, the current line
diff --git a/googletest/src/gtest.cc b/googletest/src/gtest.cc
index a64e887c..2e1fccb9 100644
--- a/googletest/src/gtest.cc
+++ b/googletest/src/gtest.cc
@@ -44,14 +44,17 @@
#include <chrono> // NOLINT
#include <cmath>
#include <cstdint>
+#include <exception>
#include <initializer_list>
#include <iomanip>
+#include <iostream>
#include <iterator>
#include <limits>
#include <list>
#include <map>
#include <ostream> // NOLINT
#include <sstream>
+#include <thread>
#include <unordered_set>
#include <vector>

@@ -6838,6 +6841,13 @@ void ScopedTrace::PushTrace(const char* file, int line, std::string message) {

// Pops the info pushed by the c'tor.
ScopedTrace::~ScopedTrace() GTEST_LOCK_EXCLUDED_(&UnitTest::mutex_) {
+ if (id != std::this_thread::get_id()) {
+ std::cerr << "ScopedTrace was created and destroyed on different "
+ "std::threads, terminating to avoid segfaults, hangs, or "
+ "silent corruption. Are you using any pika functionality that "
+ "may yield a task after creating the ScopedTrace?\n";
+ std::terminate();
+ }
UnitTest::GetInstance()->PopGTestTrace();
}

0 comments on commit 80b1b40

Please sign in to comment.