-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[rtsan] Add test to ensure coroutines get caught #111049
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Chris Apple (cjappl) ChangesCoroutines allocate when they are called: https://en.cppreference.com/w/cpp/language/coroutines > When a coroutine begins execution, it performs the following: > allocates the coroutine state object using operator new. Just adding this test as a sanity check that we catch this allocation, and it passes on my machines Full diff: https://github.com/llvm/llvm-project/pull/111049.diff 1 Files Affected:
diff --git a/compiler-rt/test/rtsan/coroutine.cpp b/compiler-rt/test/rtsan/coroutine.cpp
new file mode 100644
index 00000000000000..bd7c0ad630c0f4
--- /dev/null
+++ b/compiler-rt/test/rtsan/coroutine.cpp
@@ -0,0 +1,31 @@
+// RUN: %clangxx -std=c++20 -fsanitize=realtime %s -o %t
+// RUN: not %run %t 2>&1 | FileCheck %s
+// UNSUPPORTED: ios
+
+// Intent: Coroutines allocate memory and are not allowed in a [[clang::nonblocking]] function.
+
+#include <coroutine>
+
+struct SimpleCoroutine {
+ struct promise_type {
+ SimpleCoroutine get_return_object() { return SimpleCoroutine{}; }
+ std::suspend_never initial_suspend() { return {}; }
+ std::suspend_never final_suspend() noexcept { return {}; }
+ void unhandled_exception() {}
+ void return_void() {}
+ };
+};
+
+SimpleCoroutine example_coroutine() { co_return; }
+
+void calls_a_coroutine() [[clang::nonblocking]] { example_coroutine(); }
+
+int main() {
+ calls_a_coroutine();
+ return 0;
+}
+
+// CHECK: ==ERROR: RealtimeSanitizer
+
+// Somewhere in the stack this should be mentioned
+// CHECK: calls_a_coroutine
|
| @@ -0,0 +1,31 @@ | |||
| // RUN: %clangxx -std=c++20 -fsanitize=realtime %s -o %t | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only valid in C++20, what should I do to ensure it doesn't affect systems without c++20? This test should be a no-op in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a system without C++20? This is a test of clang trunk, which supports C++20?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running through this test on one of my older ubuntu machines, I get this error:
/test_radsan/llvm-project/compiler-rt/test/rtsan/coroutine.cpp:7:10: fatal error: 'coroutine' file not found
7 | #include <coroutine> | ^~~~~~~~~~~
That led me to believe that I needed to do something special here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, hmm, yes that makes sense, you are right. That means the lib(std)c++ it uses is too old.
|
CC @davidtrevelyan for review @dougsonos because we were chatting about this |
|
My personal opinion on this one is that whether or not a coroutine allocates memory is irrelevant for realtime sanitizer, so I'm a bit skeptical of the value provided by this test. Memory allocation should be caught by our tests for In the hypothetical scenario that a valid coroutines implementation is able to begin a coroutine without allocating from the system, would this test start to fail erroneously? |
Yeah, this is a good point. I think the specification also says:
So maybe we can just close this and assume we are good in this case? I'm amenable to that. I initially went down this path just to confirm we were well behaved in this case. |
Sounds good to me! Happy to hear contrasting opinions of course 👍 |
|
Closing, I agree this may be a bit finicky for us to test directly. thanks for everyone's input on this :) |
Coroutines allocate when they are called:
https://en.cppreference.com/w/cpp/language/coroutines
Just adding this test as a sanity check that we catch this allocation, and it passes on my machines