[rtsan] Add option to allow printing of duplicate stacks

[cjappl]

Following the example of tsan, as well as the naming of this flag:

llvm-project/compiler-rt/lib/tsan/rtl/tsan_flags.inc

Lines 23 to 25 in 1c8ac4c

	TSAN_FLAG(bool, suppress_equal_stacks, true,
	"Suppress a race report if we've already output another race report "
	"with the same stack.")

This would allow users to determine if they want to see ALL output from rtsan.

I also chose to remove UNLIKELY, as it is now up to the flag whether or not it is likely that we go through this conditional. I think it may just be better to leave it to the branch predictor anyway.

[llvmbot]

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

Author: Chris Apple (cjappl)

Changes

Following the example of tsan, as well as the naming of this flag:

llvm-project/compiler-rt/lib/tsan/rtl/tsan_flags.inc

Lines 23 to 25 in 1c8ac4c

	TSAN_FLAG(bool, suppress_equal_stacks, true,
	"Suppress a race report if we've already output another race report "
	"with the same stack.")

This would allow users to determine if they want to see ALL output from rtsan.

I also chose to remove UNLIKELY, as it is now up to the flag whether or not it is likely that we go through this conditional. I think it may just be better to leave it to the branch predictor anyway.

---

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

3 Files Affected:

• (modified) compiler-rt/lib/rtsan/rtsan.cpp (+1-5)
• (modified) compiler-rt/lib/rtsan/rtsan_flags.inc (+3)
• (modified) compiler-rt/test/rtsan/deduplicate_errors.cpp (+4)

diff --git a/compiler-rt/lib/rtsan/rtsan.cpp b/compiler-rt/lib/rtsan/rtsan.cpp
index 70edcc546219fa..81cedb3b5114f0 100644
--- a/compiler-rt/lib/rtsan/rtsan.cpp
+++ b/compiler-rt/lib/rtsan/rtsan.cpp
@@ -55,11 +55,7 @@ static void OnViolation(const BufferedStackTrace &stack,
   StackDepotHandle handle = StackDepotPut_WithHandle(stack);
 
   const bool is_stack_novel = handle.use_count() == 0;
-
-  // 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)) {
+  if (is_stack_novel || !flags().suppress_equal_stacks) {
     IncrementUniqueErrorCount();
 
     {
diff --git a/compiler-rt/lib/rtsan/rtsan_flags.inc b/compiler-rt/lib/rtsan/rtsan_flags.inc
index 5c3eb3f53a5eb4..104fac8f770406 100644
--- a/compiler-rt/lib/rtsan/rtsan_flags.inc
+++ b/compiler-rt/lib/rtsan/rtsan_flags.inc
@@ -19,3 +19,6 @@
 RTSAN_FLAG(bool, halt_on_error, true, "Exit after first reported error.")
 RTSAN_FLAG(bool, print_stats_on_exit, false, "Print stats on exit.")
 RTSAN_FLAG(const char *, suppressions, "", "Suppressions file name.")
+RTSAN_FLAG(bool, suppress_equal_stacks, true,
+           "Suppress a report if we've already output another report "
+           "with the same stack.")
diff --git a/compiler-rt/test/rtsan/deduplicate_errors.cpp b/compiler-rt/test/rtsan/deduplicate_errors.cpp
index 7d60d4d7da7dda..6fcd749cf63ee3 100644
--- a/compiler-rt/test/rtsan/deduplicate_errors.cpp
+++ b/compiler-rt/test/rtsan/deduplicate_errors.cpp
@@ -1,5 +1,6 @@
 // RUN: %clangxx -fsanitize=realtime %s -o %t
 // RUN: env RTSAN_OPTIONS="halt_on_error=false,print_stats_on_exit=true" %run %t 2>&1 | FileCheck %s
+// RUN: env RTSAN_OPTIONS="halt_on_error=false,suppress_equal_stacks=false" %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-DUPLICATES
 
 // UNSUPPORTED: ios
 
@@ -37,3 +38,6 @@ int main() {
 // CHECK: RealtimeSanitizer exit stats:
 // CHECK-NEXT: Total error count: 220
 // CHECK-NEXT: Unique error count: 4
+
+// CHECK-DUPLICATES-COUNT-220: ==ERROR:
+// CHECK-DUPLICATES-NOT: ==ERROR:

[fmayer]

. I think it may just be better to leave it to the branch predictor anyway.

It is up to the branch predictor either way to predict which branch to take, but up to the compiler to lay out the code such that the fallthrough is the common case.

[fmayer]

equal => duplicate, maybe?

[cjappl]

I do agree duplicate is clearer, but this would move away from matching tsan - do we care?

[fmayer]

OK, let's keep it the same as TSan.

[cjappl]

. I think it may just be better to leave it to the branch predictor anyway.

It is up to the branch predictor either way to predict which branch to take, but up to the compiler to lay out the code such that the fallthrough is the common case.

Makes sense - should I change anything about the code to make this easier for the compiler to deduce?

[fmayer]

. I think it may just be better to leave it to the branch predictor anyway.

It is up to the branch predictor either way to predict which branch to take, but up to the compiler to lay out the code such that the fallthrough is the common case.

Makes sense - should I change anything about the code to make this easier for the compiler to deduce?

No it's fine, I was just nitpicking on the commit message.

[cjappl]

. I think it may just be better to leave it to the branch predictor anyway.

It is up to the branch predictor either way to predict which branch to take, but up to the compiler to lay out the code such that the fallthrough is the common case.

Makes sense - should I change anything about the code to make this easier for the compiler to deduce?

No it's fine, I was just nitpicking on the commit message.

Thanks for the clarification! I appreciate the extra info