[bug] clang miscompiles coroutine awaiter, moving write across a critical section #56301
Description
I've hit what I think is a miscompilation bug in clang, where a write is moved in an illegal way that introduces a data race and/or use of uninitialized memory. Here is a test case reduced from my real codebase (Compiler Explorer):
#include <coroutine>
#include <utility>
struct SomeAwaitable {
// Resume the supplied handle once the awaitable becomes ready,
// returning a handle that should be resumed now for the sake of symmetric transfer.
// If the awaitable is already ready, return an empty handle without doing anything.
//
// Defined in another translation unit. Note that this may contain
// code that synchronizees with another thread.
std::coroutine_handle<> Register(std::coroutine_handle<>);
};
// Defined in another translation unit.
void DidntSuspend();
struct Awaiter {
SomeAwaitable&& awaitable;
bool suspended;
bool await_ready() { return false; }
std::coroutine_handle<> await_suspend(const std::coroutine_handle<> h) {
// Assume we will suspend unless proven otherwise below. We must do
// this *before* calling Register, since we may be destroyed by another
// thread asynchronously as soon as we have registered.
suspended = true;
// Attempt to hand off responsibility for resuming/destroying the coroutine.
const auto to_resume = awaitable.Register(h);
if (!to_resume) {
// The awaitable is already ready. In this case we know that Register didn't
// hand off responsibility for the coroutine. So record the fact that we didn't
// actually suspend, and tell the compiler to resume us inline.
suspended = false;
return h;
}
// Resume whatever Register wants us to resume.
return to_resume;
}
void await_resume() {
// If we didn't suspend, make note of that fact.
if (!suspended) {
DidntSuspend();
}
}
};
struct MyTask{
struct promise_type {
MyTask get_return_object() { return {}; }
std::suspend_never initial_suspend() { return {}; }
std::suspend_always final_suspend() noexcept { return {}; }
void unhandled_exception();
auto await_transform(SomeAwaitable&& awaitable) {
return Awaiter{std::move(awaitable)};
}
};
};
MyTask FooBar() {
co_await SomeAwaitable();
}The idea is that the awaiter is implemented by calling a Register function in a foreign translation unit that decides what to do:
-
If the coroutine should be resumed immediately, it returns a null handle to indicate this.
-
If the coroutine will be resumed later, it reduces some other handle to resume now, for symmetric control. (Maybe
std::noop_coroutine().)
Further, when we don't actually wind up suspending we need await_resume to do some follow-up work, in this case represented by calling the DidntSuspend function. So we use a suspended member to track whether we actually suspended. This is written before calling Register, and read after resuming.
The bug I see in my codebase is that the write of true to suspended is delayed until after the call to Register. In the reduced test case, we have something similar. Here is what Compiler Explorer gives me for clang with -std=c++20 -O1 -fno-exceptions:
FooBar(): # @FooBar()
push rbx
mov edi, 32
call operator new(unsigned long)
mov rbx, rax
mov qword ptr [rax], offset FooBar() [clone .resume]
mov qword ptr [rax + 8], offset FooBar() [clone .destroy]
lea rdi, [rax + 18]
mov byte ptr [rax + 17], 0
mov rsi, rax
call SomeAwaitable::Register(std::__n4861::coroutine_handle<void>)
mov qword ptr [rbx + 24], rax
test rax, rax
cmove rax, rbx
mov rdi, rax
call qword ptr [rax]
pop rbx
ret
FooBar() [clone .resume]: # @FooBar() [clone .resume]
push rbx
mov rbx, rdi
cmp qword ptr [rdi + 24], 0
jne .LBB1_2
call DidntSuspend()
.LBB1_2:
mov qword ptr [rbx], 0
pop rbx
ret
FooBar() [clone .destroy]: # @FooBar() [clone .destroy]
push rax
call operator delete(void*)
pop rax
retThe coroutine frame address is in rbx. After calling Register, the returned handle is stored into the coroutine frame at offset 24 and then resumed (or the original handle resumed if it's empty), and later in [clone .resume] the handle in the frame at offset 24 is compared to zero to synthesize the if (!suspended) condition.
But it's not safe to store the returned handle in the coroutine frame unless it's zero: any other value indicates that Register took responsibility for the coroutine handle, and may have passed it off to another thread. So another thread may have called destroy on the handle by the time we get around to writing into it. Similarly, the other thread may already have resumed the coroutine and see an uninitialized value at offset 24.
I think this is a miscompilation. Consider for example that Register may contain a critical section under a mutex that hands the coroutine handle off to another thread to resume, with a similar critical section in the other thread synchronizing with the first. (This is the situation in my codebase.) So we have:
-
The write of
suspendedinawait_suspendis sequenced before the call toRegisterbelow it inawait_suspend. -
The call to
Registersynchronizes with the function on the other thread that resumes the coroutine. -
That synchronization is sequenced before resuming the coroutine handle.
-
Resuming the coroutine handle is (I believe?) sequenced before the call to
await_resumethat readssuspended. -
Therefore the write of
suspendedinter-thread happens before the read ofsuspended.
So there was no data race before, but clang has introduced one by delaying the write to the coroutine frame.
For what it's worth, I spent some time dumping IR after optimization passes with my real codebase, and in that case this seemed to be related to an interaction betweem SROAPass and CoroSplitPass:
-
Until
SROAPassthe write was a simple store to the coroutine frame, before the call toRegister. -
SROAPasseliminated the write altogether, turning it into phi nodes that plumbed the value directly into the branch. The value was plumbed from before the call toRegisterto after it. -
CoroSplitPassre-introduced astoreinstruction, after the call toRegister.
I am far from an expert here, but I wonder if SROAPass should be forbidden from making optimizatons of this sort across an llvm.coro.suspend?
Metadata
Metadata
Assignees
Type
Projects
Status
Status
