[TySan] Added a 'print_stacktrace' flag for more detailed errors #121756
Conversation
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: None (gbMattN) ChangesRaised in issue #121697 Full diff: https://github.com/llvm/llvm-project/pull/121756.diff 2 Files Affected:
diff --git a/compiler-rt/lib/tysan/tysan.cpp b/compiler-rt/lib/tysan/tysan.cpp
index 39d78e7c95e0cd..2786d826afb5f4 100644
--- a/compiler-rt/lib/tysan/tysan.cpp
+++ b/compiler-rt/lib/tysan/tysan.cpp
@@ -198,7 +198,7 @@ static void reportError(void *Addr, int Size, tysan_type_descriptor *TD,
if (pc) {
- bool request_fast = StackTrace::WillUseFastUnwind(true);
+ bool request_fast = StackTrace::WillUseFastUnwind(true) && !flags().print_stacktrace;
BufferedStackTrace ST;
ST.Unwind(kStackTraceMax, pc, bp, 0, 0, 0, request_fast);
ST.Print();
diff --git a/compiler-rt/lib/tysan/tysan_flags.inc b/compiler-rt/lib/tysan/tysan_flags.inc
index 98b6591f844ef0..be65c8e828794a 100644
--- a/compiler-rt/lib/tysan/tysan_flags.inc
+++ b/compiler-rt/lib/tysan/tysan_flags.inc
@@ -15,3 +15,6 @@
// TYSAN_FLAG(Type, Name, DefaultValue, Description)
// See COMMON_FLAG in sanitizer_flags.inc for more details.
+
+TYSAN_FLAG(bool, print_stacktrace, false,
+ "Include full stacktrace into an error report")
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
gbMattN
force-pushed
the
users/nagym/tysan-add-full-stack-trace-option
branch
from
f55dd6c to
6580b3f
Compare
compiler-rt/lib/tysan/tysan.cpp
Outdated
| @@ -198,7 +198,8 @@ static void reportError(void *Addr, int Size, tysan_type_descriptor *TD, | |||
|
|
|||
| if (pc) { | |||
|
|
|||
| bool request_fast = StackTrace::WillUseFastUnwind(true); | |||
| bool request_fast = | |||
There was a problem hiding this comment.
I thought "fastunwind" was unrelated to whether we print a stack trace or not ? I thought it controls the method of obtaining the stack trace.
There was a problem hiding this comment.
For some reason, setting it to false also gave a full stack printout
Thanks for pointing this out though, I've found the proper way of doing it!
I think it should match the existing behavior in other places i.e. full stacktrace off by default. |
|
That sounds goo to me! We now have the existing behaviour if you don't add |
For correctness that should be |
There was a problem hiding this comment.
Does Tysan support passing options through environment variables, such as TYSAN_OPTIONS? If so, it might be helpful to add a test to check if print_stacktrace works as intended. This could be done using a lit substitution variable like %env_tysan_opts (if not already present?).
|
My previous comment was a mistake, |
| // RUN: %clang_tysan -O0 %s -o %t && %run %t >%t.out 2>&1 | ||
| // RUN: FileCheck --check-prefixes=CHECK,CHECK-SHORT %s < %t.out | ||
|
|
||
| // RUN: env TYSAN_OPTIONS=print_stacktrace=1 %run %t >%t.out 2>&1 |
There was a problem hiding this comment.
I think eventually we will need to introduce a %env_tysan_opts lit substitutions to protect from environment variables affecting test results, but can be done in a separate patch.
There was a problem hiding this comment.
Ah! We actually have that already it seems. I'll change that
compiler-rt/lib/tysan/tysan.cpp
Outdated
| @@ -198,9 +198,14 @@ static void reportError(void *Addr, int Size, tysan_type_descriptor *TD, | |||
|
|
|||
| if (pc) { | |||
|
|
|||
There was a problem hiding this comment.
nit: While you are here, maybe drop the newline at the top here?
gbMattN
force-pushed
the
users/nagym/tysan-add-full-stack-trace-option
branch
from
b3d03da to
af53f83
Compare
gbMattN
force-pushed
the
users/nagym/tysan-add-full-stack-trace-option
branch
from
cacfa49 to
d8c7734
Compare
Raised in issue #121697