Skip to content

Conversation

@cjappl
Copy link
Contributor

@cjappl cjappl commented Oct 3, 2024

Coroutines 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

@llvmbot
Copy link
Member

llvmbot commented Oct 3, 2024

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

Author: Chris Apple (cjappl)

Changes

Coroutines 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:

  • (added) compiler-rt/test/rtsan/coroutine.cpp (+31)
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
Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@cjappl cjappl requested a review from fmayer October 3, 2024 19:41
@cjappl
Copy link
Contributor Author

cjappl commented Oct 3, 2024

CC @davidtrevelyan for review

@dougsonos because we were chatting about this

@davidtrevelyan
Copy link
Contributor

davidtrevelyan commented Oct 3, 2024

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 malloc and friends - where that allocation ultimately comes from (at the widest part of the funnel) is imho immaterial.

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?

@cjappl
Copy link
Contributor Author

cjappl commented Oct 3, 2024

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 malloc and friends - where that allocation ultimately comes from (at the widest part of the funnel) is imho immaterial.

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:

Each coroutine is associated with
...
the coroutine state, which is internal, dynamically-allocated storage (unless the allocation is optimized out), object that contains

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.

@davidtrevelyan
Copy link
Contributor

So maybe we can just close this and assume we are good in this case?

Sounds good to me! Happy to hear contrasting opinions of course 👍

@cjappl
Copy link
Contributor Author

cjappl commented Oct 3, 2024

Closing, I agree this may be a bit finicky for us to test directly.

thanks for everyone's input on this :)

@cjappl cjappl closed this Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants