[rtsan] Decouple assertions from error actions

[davidtrevelyan]

Decouples sanitizer assertion ExpectNotRealtime from the action that should happen if a real-time context is detected. Why?

• makes unit tests faster (we don't need to fork the process to expect a death, we just pass through a mock error action)
• opens up future error actions like Continue instead of always dying

[davidtrevelyan]

@cjappl for review

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: None (davidtrevelyan)

Changes

Decouples sanitizer assertion ExpectNotRealtime from the action that should happen if a real-time context is detected. Why?

• makes unit tests faster (we don't need to fork the process to expect a death, we just pass through a mock error action)
• opens up future error actions like Continue instead of always dying

---

Full diff: https://github.com/llvm/llvm-project/pull/109535.diff

5 Files Affected:

• (modified) compiler-rt/lib/rtsan/CMakeLists.txt (-1)
• (modified) compiler-rt/lib/rtsan/rtsan.cpp (+16-9)
• (removed) compiler-rt/lib/rtsan/rtsan_assertions.cpp (-28)
• (modified) compiler-rt/lib/rtsan/rtsan_assertions.h (+11-2)
• (modified) compiler-rt/lib/rtsan/tests/rtsan_test_assertions.cpp (+16-14)

diff --git a/compiler-rt/lib/rtsan/CMakeLists.txt b/compiler-rt/lib/rtsan/CMakeLists.txt
index 68f890050c6ea8..0fc3a3f8f48960 100644
--- a/compiler-rt/lib/rtsan/CMakeLists.txt
+++ b/compiler-rt/lib/rtsan/CMakeLists.txt
@@ -2,7 +2,6 @@ include_directories(..)
 
 set(RTSAN_CXX_SOURCES
   rtsan.cpp
-  rtsan_assertions.cpp
   rtsan_context.cpp
   rtsan_diagnostics.cpp
   rtsan_flags.cpp
diff --git a/compiler-rt/lib/rtsan/rtsan.cpp b/compiler-rt/lib/rtsan/rtsan.cpp
index 936f0b5b8cee39..2afdf3c76696e7 100644
--- a/compiler-rt/lib/rtsan/rtsan.cpp
+++ b/compiler-rt/lib/rtsan/rtsan.cpp
@@ -10,6 +10,7 @@
 
 #include <rtsan/rtsan.h>
 #include <rtsan/rtsan_assertions.h>
+#include <rtsan/rtsan_diagnostics.h>
 #include <rtsan/rtsan_flags.h>
 #include <rtsan/rtsan_interceptors.h>
 
@@ -28,6 +29,13 @@ static void SetInitialized() {
   atomic_store(&rtsan_initialized, 1, memory_order_release);
 }
 
+static auto PrintDiagnosticsAndDieAction(DiagnosticsInfo info) {
+  return [info]() {
+    __rtsan::PrintDiagnostics(info);
+    Die();
+  };
+}
+
 extern "C" {
 
 SANITIZER_INTERFACE_ATTRIBUTE void __rtsan_init() {
@@ -74,22 +82,21 @@ SANITIZER_INTERFACE_ATTRIBUTE void __rtsan_enable() {
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE void
-__rtsan_notify_intercepted_call(const char *intercepted_function_name) {
+__rtsan_notify_intercepted_call(const char *func_name) {
   __rtsan_ensure_initialized();
-
   GET_CALLER_PC_BP;
-  DiagnosticsInfo info = {InterceptedCallInfo{intercepted_function_name}, pc,
-                          bp};
-  ExpectNotRealtime(GetContextForThisThread(), info);
+  ExpectNotRealtime(
+      GetContextForThisThread(),
+      PrintDiagnosticsAndDieAction({InterceptedCallInfo{func_name}, pc, bp}));
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE void
-__rtsan_notify_blocking_call(const char *blocking_function_name) {
+__rtsan_notify_blocking_call(const char *func_name) {
   __rtsan_ensure_initialized();
-
   GET_CALLER_PC_BP;
-  DiagnosticsInfo info = {BlockingCallInfo{blocking_function_name}, pc, bp};
-  ExpectNotRealtime(GetContextForThisThread(), info);
+  ExpectNotRealtime(
+      GetContextForThisThread(),
+      PrintDiagnosticsAndDieAction({BlockingCallInfo{func_name}, pc, bp}));
 }
 
 } // extern "C"
diff --git a/compiler-rt/lib/rtsan/rtsan_assertions.cpp b/compiler-rt/lib/rtsan/rtsan_assertions.cpp
deleted file mode 100644
index 4aae85de5c52f1..00000000000000
--- a/compiler-rt/lib/rtsan/rtsan_assertions.cpp
+++ /dev/null
@@ -1,28 +0,0 @@
-//===--- rtsan_assertions.cpp - Realtime Sanitizer --------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-//
-// Part of the RealtimeSanitizer runtime library
-//
-//===----------------------------------------------------------------------===//
-#include "rtsan/rtsan_assertions.h"
-
-#include "rtsan/rtsan.h"
-#include "rtsan/rtsan_diagnostics.h"
-
-using namespace __sanitizer;
-
-void __rtsan::ExpectNotRealtime(Context &context, const DiagnosticsInfo &info) {
-  CHECK(__rtsan_is_initialized());
-  if (context.InRealtimeContext() && !context.IsBypassed()) {
-    context.BypassPush();
-
-    PrintDiagnostics(info);
-    Die();
-    context.BypassPop();
-  }
-}
diff --git a/compiler-rt/lib/rtsan/rtsan_assertions.h b/compiler-rt/lib/rtsan/rtsan_assertions.h
index bc1235363669df..ac9e1d203d20be 100644
--- a/compiler-rt/lib/rtsan/rtsan_assertions.h
+++ b/compiler-rt/lib/rtsan/rtsan_assertions.h
@@ -12,10 +12,19 @@
 
 #pragma once
 
+#include "rtsan/rtsan.h"
 #include "rtsan/rtsan_context.h"
-#include "rtsan/rtsan_diagnostics.h"
 
 namespace __rtsan {
 
-void ExpectNotRealtime(Context &context, const DiagnosticsInfo &info);
+template <typename OnViolationAction>
+void ExpectNotRealtime(Context &context, OnViolationAction &&on_violation) {
+  CHECK(__rtsan_is_initialized());
+  if (context.InRealtimeContext() && !context.IsBypassed()) {
+    context.BypassPush();
+    on_violation();
+    context.BypassPop();
+  }
+}
+
 } // namespace __rtsan
diff --git a/compiler-rt/lib/rtsan/tests/rtsan_test_assertions.cpp b/compiler-rt/lib/rtsan/tests/rtsan_test_assertions.cpp
index b6999eeb7746a2..5df3e026e8e5fe 100644
--- a/compiler-rt/lib/rtsan/tests/rtsan_test_assertions.cpp
+++ b/compiler-rt/lib/rtsan/tests/rtsan_test_assertions.cpp
@@ -13,9 +13,8 @@
 #include "rtsan_test_utilities.h"
 
 #include "rtsan/rtsan_assertions.h"
-#include "rtsan/rtsan_diagnostics.h"
 
-#include <gtest/gtest.h>
+#include <gmock/gmock.h>
 
 using namespace __rtsan;
 
@@ -24,30 +23,33 @@ class TestRtsanAssertions : public ::testing::Test {
   void SetUp() override { __rtsan_ensure_initialized(); }
 };
 
-DiagnosticsInfo FakeDiagnosticsInfo() {
-  DiagnosticsInfo info;
-  info.pc = 0;
-  info.bp = 0;
-  info.call_info = InterceptedCallInfo{"fake_function_name"};
-  return info;
+static void testExpectNotRealtime(__rtsan::Context &context,
+                                  bool expect_violation_callback) {
+  ::testing::MockFunction<void()> mock_on_violation;
+  EXPECT_CALL(mock_on_violation, Call).Times(expect_violation_callback ? 1 : 0);
+  ExpectNotRealtime(context, mock_on_violation.AsStdFunction());
 }
 
-TEST_F(TestRtsanAssertions, ExpectNotRealtimeDoesNotDieIfNotInRealtimeContext) {
+TEST_F(TestRtsanAssertions,
+       ExpectNotRealtimeDoesNotCallViolationActionIfNotInRealtimeContext) {
   __rtsan::Context context{};
   ASSERT_FALSE(context.InRealtimeContext());
-  ExpectNotRealtime(context, FakeDiagnosticsInfo());
+  testExpectNotRealtime(context, false);
 }
 
-TEST_F(TestRtsanAssertions, ExpectNotRealtimeDiesIfInRealtimeContext) {
+TEST_F(TestRtsanAssertions,
+       ExpectNotRealtimeCallsViolationActionIfInRealtimeContext) {
   __rtsan::Context context{};
   context.RealtimePush();
   ASSERT_TRUE(context.InRealtimeContext());
-  EXPECT_DEATH(ExpectNotRealtime(context, FakeDiagnosticsInfo()), "");
+  testExpectNotRealtime(context, true);
 }
 
-TEST_F(TestRtsanAssertions, ExpectNotRealtimeDoesNotDieIfRealtimeButBypassed) {
+TEST_F(TestRtsanAssertions,
+       ExpectNotRealtimeDoesNotCallViolationActionIfRealtimeButBypassed) {
   __rtsan::Context context{};
   context.RealtimePush();
   context.BypassPush();
-  ExpectNotRealtime(context, FakeDiagnosticsInfo());
+  ASSERT_TRUE(context.IsBypassed());
+  testExpectNotRealtime(context, false);
 }

[cjappl]

Don't 100% know what is right here, but I have been using FunctionCase for lambdas and other callables. Leaving it to you to determine if you want to change it or not

(and we should get clarity on what is appropriate, our tests use a lot of auto Func = [](){})

[cjappl]

This name could be improved, hard to reason about what it is doing.

perhaps:

ExpectRealtime(context, true)

has less double negatives. also test and expect is unnecessary duplication, would rather have "assertion style" leading with expect or assert

[cjappl]

PascalCase