[rtsan][NFC] Refactor where we unwind the stack

[cjappl]

This one is possibly easier to view without whitespace changes. The rtsan.cpp is mostly just extracting from that lambda.

Why?

Short answer: this gets us ready for suppression list support.

Longer answer:
This change alters where we unwind the stack. We now do it in ExpectNotRealtime, and pass in the DiagnosticInfo and Stack to OnViolationAction.

Currently, this change is a no-op NFC.

This is important, however, for suppression lists. When we add suppressions (after this review) we need to inspect the stack before determining if it is a violation.

void ExpectNotRealtime
    stack = unwind_stack()
    if is_suppressed(stack)
        return
    else
        OnViolationAction(info, stack);

We need to unwind the stack to see if it is suppressed before optionally calling OnViolationAction.

[llvmbot]

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

Author: Chris Apple (cjappl)

Changes

This one is possibly easier to view without whitespace changes. The rtsan.cpp is mostly just extracting from that lambda.

Why?

Short answer: this gets us ready for suppression list support.

Longer answer:
This change alters where we unwind the stack. We now do it in ExpectNotRealtime, and pass in the DiagnosticInfo and Stack to OnViolationAction.

Currently, this change is a no-op NFC.

This is important, however, for suppression lists. When we add suppressions (after this review) we need to inspect the stack before determining if it is a violation.

void ExpectNotRealtime
    stack = unwind_stack()
    if is_suppressed(stack)
        return
    else
        OnViolationAction(info, stack);

We need to unwind the stack to see if it is suppressed before optionally calling OnViolationAction.

---

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

3 Files Affected:

• (modified) compiler-rt/lib/rtsan/rtsan.cpp (+20-28)
• (modified) compiler-rt/lib/rtsan/rtsan_assertions.h (+14-2)
• (modified) compiler-rt/lib/rtsan/tests/rtsan_test_assertions.cpp (+8-2)

diff --git a/compiler-rt/lib/rtsan/rtsan.cpp b/compiler-rt/lib/rtsan/rtsan.cpp
index 9f4a9a5701636f..7434412bed42ce 100644
--- a/compiler-rt/lib/rtsan/rtsan.cpp
+++ b/compiler-rt/lib/rtsan/rtsan.cpp
@@ -46,41 +46,33 @@ static InitializationState GetInitializationState() {
       atomic_load(&rtsan_initialized, memory_order_acquire));
 }
 
-static auto OnViolationAction(DiagnosticsInfo info) {
-  return [info]() {
-    IncrementTotalErrorCount();
+static auto OnViolationAction(const BufferedStackTrace &stack,
+                              const DiagnosticsInfo &info) {
+  IncrementTotalErrorCount();
 
-    BufferedStackTrace stack;
+  // If in the future we interop with other sanitizers, we will
+  // need to make our own stackdepot
+  StackDepotHandle handle = StackDepotPut_WithHandle(stack);
 
-    // We use the unwind_on_fatal flag here because of precedent with other
-    // sanitizers, this action is not necessarily fatal if halt_on_error=false
-    stack.Unwind(info.pc, info.bp, nullptr,
-                 common_flags()->fast_unwind_on_fatal);
+  const bool is_stack_novel = handle.use_count() == 0;
 
-    // If in the future we interop with other sanitizers, we will
-    // need to make our own stackdepot
-    StackDepotHandle handle = StackDepotPut_WithHandle(stack);
+  // Marked UNLIKELY as if user is runing with halt_on_error=false
+  // we expect a high number of duplicate stacks. We are willing
+  // To pay for the first insertion.
+  if (UNLIKELY(is_stack_novel)) {
+    IncrementUniqueErrorCount();
 
-    const bool is_stack_novel = handle.use_count() == 0;
+    PrintDiagnostics(info);
+    stack.Print();
 
-    // Marked UNLIKELY as if user is runing with halt_on_error=false
-    // we expect a high number of duplicate stacks. We are willing
-    // To pay for the first insertion.
-    if (UNLIKELY(is_stack_novel)) {
-      IncrementUniqueErrorCount();
-
-      PrintDiagnostics(info);
-      stack.Print();
-
-      handle.inc_use_count_unsafe();
-    }
+    handle.inc_use_count_unsafe();
+  }
 
     if (flags().halt_on_error) {
       if (flags().print_stats_on_exit)
         PrintStatisticsSummary();
       Die();
     }
-  };
 }
 
 extern "C" {
@@ -141,8 +133,8 @@ __rtsan_notify_intercepted_call(const char *func_name) {
   __rtsan_ensure_initialized();
   GET_CALLER_PC_BP;
   ExpectNotRealtime(GetContextForThisThread(),
-                    OnViolationAction({DiagnosticsInfoType::InterceptedCall,
-                                       func_name, pc, bp}));
+                    {DiagnosticsInfoType::InterceptedCall, func_name, pc, bp},
+                    OnViolationAction);
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE void
@@ -150,8 +142,8 @@ __rtsan_notify_blocking_call(const char *func_name) {
   __rtsan_ensure_initialized();
   GET_CALLER_PC_BP;
   ExpectNotRealtime(GetContextForThisThread(),
-                    OnViolationAction({DiagnosticsInfoType::BlockingCall,
-                                       func_name, pc, bp}));
+                    {DiagnosticsInfoType::BlockingCall, func_name, pc, bp},
+                    OnViolationAction);
 }
 
 } // extern "C"
diff --git a/compiler-rt/lib/rtsan/rtsan_assertions.h b/compiler-rt/lib/rtsan/rtsan_assertions.h
index 4646e750b6796e..745cbea0eb3a29 100644
--- a/compiler-rt/lib/rtsan/rtsan_assertions.h
+++ b/compiler-rt/lib/rtsan/rtsan_assertions.h
@@ -14,15 +14,27 @@
 
 #include "rtsan/rtsan.h"
 #include "rtsan/rtsan_context.h"
+#include "rtsan/rtsan_diagnostics.h"
+
+#include "sanitizer_common/sanitizer_stacktrace.h"
 
 namespace __rtsan {
 
 template <typename OnViolationAction>
-void ExpectNotRealtime(Context &context, OnViolationAction &&OnViolation) {
+void ExpectNotRealtime(Context &context, const DiagnosticsInfo &info,
+                       OnViolationAction &&OnViolation) {
   CHECK(__rtsan_is_initialized());
   if (context.InRealtimeContext() && !context.IsBypassed()) {
     ScopedBypass sb{context};
-    OnViolation();
+
+    __sanitizer::BufferedStackTrace stack;
+
+    // We use the unwind_on_fatal flag here because of precedent with other
+    // sanitizers, this action is not necessarily fatal if halt_on_error=false
+    stack.Unwind(info.pc, info.bp, nullptr,
+                 __sanitizer::common_flags()->fast_unwind_on_fatal);
+
+    OnViolation(stack, info);
   }
 }
 
diff --git a/compiler-rt/lib/rtsan/tests/rtsan_test_assertions.cpp b/compiler-rt/lib/rtsan/tests/rtsan_test_assertions.cpp
index 3b279989a49cb3..5e54225025fbd0 100644
--- a/compiler-rt/lib/rtsan/tests/rtsan_test_assertions.cpp
+++ b/compiler-rt/lib/rtsan/tests/rtsan_test_assertions.cpp
@@ -14,8 +14,11 @@
 
 #include "rtsan/rtsan_assertions.h"
 
+#include "sanitizer_common/sanitizer_stacktrace.h"
+
 #include <gmock/gmock.h>
 
+using namespace __sanitizer;
 using namespace __rtsan;
 
 class TestRtsanAssertions : public ::testing::Test {
@@ -25,9 +28,12 @@ class TestRtsanAssertions : public ::testing::Test {
 
 static void ExpectViolationAction(Context &context,
                                   bool expect_violation_callback) {
-  ::testing::MockFunction<void()> mock_on_violation;
+  ::testing::MockFunction<void(const BufferedStackTrace &stack,
+                               const DiagnosticsInfo &info)>
+      mock_on_violation;
   EXPECT_CALL(mock_on_violation, Call).Times(expect_violation_callback ? 1 : 0);
-  ExpectNotRealtime(context, mock_on_violation.AsStdFunction());
+  DiagnosticsInfo info{};
+  ExpectNotRealtime(context, info, mock_on_violation.AsStdFunction());
 }
 
 TEST_F(TestRtsanAssertions,

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[cjappl]

Let me know if you have any post-commit reviews @davidtrevelyan - felt comfortable submitting this because you reviewed a preliminary version of this.

Thanks as always for the thorough and quick reviews @fmayer