[prev in list] [next in list] [prev in thread] [next in thread]
List: llvm-bugs
Subject: [llvm-bugs] [Bug 56301] [bug] clang miscompiles coroutine awaiter, moving write across a critical se
From: LLVM Bugs via llvm-bugs <llvm-bugs () lists ! llvm ! org>
Date: 2022-06-30 10:12:03
Message-ID: 20220630101203.f0cbc199950839fd () email ! llvm ! org
[Download RAW message or body]
[Attachment #2 (text/html)]
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>Issue</th>
<td>
<a href=https://github.com/llvm/llvm-project/issues/56301>56301</a>
</td>
</tr>
<tr>
<th>Summary</th>
<td>
[bug] clang miscompiles coroutine awaiter, moving write across a critical \
section </td>
</tr>
<tr>
<th>Labels</th>
<td>
new issue
</td>
</tr>
<tr>
<th>Assignees</th>
<td>
</td>
</tr>
<tr>
<th>Reporter</th>
<td>
jacobsa
</td>
</tr>
</table>
<pre>
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](https://godbolt.org/z/csYcszcdr)):
```
#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
ret
```
The 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:
1. The write of `suspended` in `await_suspend` is sequenced before the call to \
`Register` below it in `await_suspend`.
2. The call to `Register` synchronizes with the function on the other thread that \
resumes the coroutine.
3. That synchronization is sequenced before resuming the coroutine handle.
4. Resuming the coroutine handle is (I believe?) sequenced before the call to \
`await_resume` that reads `suspended`.
5. Therefore the write of `suspended` inter-thread happens before the read of \
`suspended`.
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 `SROAPass` the write was a simple store to the coroutine frame, before the \
call to `Register`.
* `SROAPass` eliminated the write altogether, turning it into phi nodes that plumbed \
the value directly into the branch. The value was plumbed from before the call to \
`Register` to after it.
* `CoroSplitPass` re-introduced a `store` instruction, after the call to `Register`.
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`?** </pre>
<img width="1px" height="1px" alt="" \
src="http://email.email.llvm.org/o/eJydWttu4zgS_RrnhYjhyJ3bQx5ymWAD7Owsumex2KeAkmibHYn \
UiFTc7q_fU0XqajlpTMOTm8i6Hladoia1-eHuZZFcvyux017sd9KLF-F32rwJ7YQUpXaZLStdSK-tEWmzFdqIr \
JBmu0gesUHVCsv2tfaKdpT2XeW0RBqhi0JtZSH28gCRkKyNr23eZIok59JLUcsM202-SJ5tLRqnhN2IxmijvZa \
F_glZpSptfViKf5Amtskr50UmsbhWJC0Xm9qWojzgd6jLbK5SerpIbhaXD49svqrFbz-qwtaqXlw-4cnO-8ot1 \
vfQjM_W5qkt_NLWcOv5J_7L3P8y9zPL60VySx-sXD0tVu3Xq1X8hF-TtTZZ0eRQun7MbG0br41arH-be45nhfa \
H_il_db5uMi--2VLd76X2Mi2w-vohPBUiGCq-KteUCvFUwjVVVWi4v0MEsdiaLDyQ3f5UIXsINwKTH5CwibBa- \
aZGtLeIahTCiXI72xQ5dmMFqcuFsXuxQYpYr3zjPLlDWSpf60z4Whq3UfVyIv9lM7GH8lewMZ1J0QgCjCorf2g \
N2WsPK7zILdtnDoTK7UTDRN-T2iDsEX8WqutgWkQvgOWX4l_WRzchEYgFOjNrvNRmIo2AFONxMNmutgaARDDJs \
l7-jhzpzHI-J6is7zsQvAZ_kHckHOnbaucBwuTmk6UMupj9xfVT_3NE1C87HDa8W52LJ50b_61xlaIjdzNSMQQ \
hAxCyBvAb4XKRXOHT57WTIkRqbQFksgqVT8THx7zvNQKArCBFLQ42snAkUJDTo72fxTaIdZ17yCoKxWe7dlF_q \
6VL_73jk7YnKBadT4hpoZwTVY1KZwTHfK8dHbXC7pfiv0qUjSPYHglkuC2S-1ThJCGG9yhiRUHo7lHxKJymYwy \
tBEycwBzVrrYHJDk9tEmeEU3RFLIFqm1cgdVOOAsY4Dvk7eQ7neegSfWQFX26UJ2eAKBGHeWtj4r3dEyFt3xQU \
Qc2VCQqxFqnXNe4TnDd0NQknqMD5CcVgy4NAwNCpmQDod6-hprDtnQIWw4Ozu4YtyREb7jiJxediKPMdl78-WF \
VWooXE7LFXQaxe6P6x6WgtUPkOEvonH5G-keBGYcAxwqL8HvODzYy80ENdH6gAMsaQOcgOrA_UhdFZwRQg4bY9 \
LyNJRzNFVWiGMd9mvr27A1XxGO5G_356GxO-xMxCfUOA7p47aXxjqzoTBoYEpX0meuVHaniStaWkJDmm1NnGA1 \
oEMlhuEpqYoZagd2EkFPwlzNw6ovZMZw-KKijMA3dOKrlsej-fvhTureBgvh3VBqwMPXqD5Ua6w87xFZRJCiAr \
zb9rjJ_XFRpG-sdmjSoqNHHV8M5i_RrUEr_pjhZgPs5gR51JM1Y9SNTKCSfi-WENyZU7fw17EN7m-1gtIELSUB \
IYCa2LrH44xZ2nNxoVWyGeNj5RywXAoebh2mdSf3pTh6T-Gztg6yPoZzZV1YzbcA3U34wy0v5KxU7nStJdS7yn \
lj9KNcoC2VVqFIZH3pM25SkgJyu7F6txKYxGdMKYhtUzJTemiO-EVTkKoNKFyYKZAPtcMKg0f4iPezK4Qzv1KC \
YuUYtKZgrQnzICs0BpkG1a2mrhVW5ziRzO-2Wn-ripj7QBBdCA2YdYUpxiHlo8QM9sZ6iI9Bqquk9ESYiWdtiS \
aXjd3mAeASug42xtnrtp4OQw6vVkr4NzH1uam7yYb4yXMFsKGBd4d9roiJVW9EoX1hmFBXyq9WoOiJznIDgzcY \
W4Cnn2Lq39Rv7O-x1taqw7QgLFDoIGpW7ASK4j-0VD3AMm75mYhUmuDS0ImAleyOnOKTYMG1koCXozYRJxzOlh \
_uBLnWmjEHZdr5AfzaE6JZ6LKdngIbXF-GUIpfLQz8otucCOMGHfA3zLDoDVDAfCkGceoaNuSokkbPGYKqLFjD \
QJLVie2QuaYj8op9fu4mWcx6pGqWLZx5QwhIDeN3PwHyqjgZbsdXvAC0nuQ4zephVoPicQfiULZIHfJKVOP_jQ \
pxvjD3vyqkj-z4cc4c1CivFR_8w8orFl9Voy6C2ClE1bsdVNv0xfoDiyt9x7ikc62T8mAOLf7ZStfRw1CgcxJs \
GNGtLo1BhiXXezsskZTRyyhM6_9oTFas8Rp_LB1pFdwWPxOWc8pMaffmQQZcSy3jOLp9-TSZC8yBufklypM5Ho \
gsUc3YnhGgg9-LmpB3pAZCemHFxHe1YnYiX07PxapMwbkpc5OYm3NdX8-Xm6uLkHEYdPo68vxDCNNiefIm2H1n \
Hx4msl_PZzqh_D56fQmAM70nv59AyxritOtiNHqCJHR-pWUidOmYz5-tzSH565trzAccnEatmEpHrcSImIPpuF \
H9f_vPh4eL1xDGeYdC8rt20vv81SMyb8LcTMDh5JzLwcQJOndwuA6cw1ZU1NBbliSTw4aC-MS2grW9TUZ1vJ9l \
gz4I2tUS_kHle06UGNV5D7YLiBWIi7rmfnW69oYkRIeuvISHEwQG-laKeP6fPt4Uv-RIGV-I5LRODz3FQtrXe0 \
uzQiu5Y4QYsDYTIhStDvqINPIBZXHRi7jhQH9-pztTQhmeMIj6E_irJDTjxU9V0NUEXgdjg9E_VUqLZMZHUgAr \
mmsnRMPoPjW9Nx_ApnNwwp-SAzUfTzEYw3kJFUWQe4VSaQ6Sr77JoVEeJW34z5vPe2rdPryiiHW186VKK2Ukln \
aNMcNDIhcmFKDHC8Z_6vYSnQFTbYwJrrBlmJj3wb16HGziMuELCIsPZIHZGeGR80ZXut0CQwozAwJnqbe934iV \
YgNHYUXKPyKE0kzcQIZZDeIxSen5-Pvy1fX3iI42dvkNZgrgh3jnso2CrH5LGr9kEDa6mISYjrxE6GJnFaYuEQ \
H7j1Y8ggKLnZhM4n6Z-nmHmyRfbLd08VhixOBLQX43rlm7ykdK18zwGtXSe3xxo0P1W1ICB0wQURwhK0ISFXiz \
5vm7Iyccs3PQzj-uHE6pD6q9GGWLYcYg4zczD_S3heV7cKOVJsOiEpMHbAjeISDs925kocvJCIibpG-ldk97R- \
4gYzRlX2zFoFg0jqV-W4dbu1GLB19Y3LxQirSg9z5Svz4J7NIQGHyUAOsngyJpLjm3di_wg7wj4eYzgTlZ44oa \
28N-P9420feMGhQ17SRV58FayvaV_xNzo4zS1k65_iQnZhgsVj4Bt8IK1833vo7JB7z95rIslHfTG70g7JlaY7 \
sP0zvUwb1BMoO7la5w3Lea3ssUCF-aIu-kr0baK80GW8SUqFyoUvjK0Or4SoTaah3IRwiwDdlPl91hJIf329Y_ \
7f0MXZYJk4vsj3P1WoY_Ev89c-fyHx-TJ9j5ulAUuQFQSY1OcDSXn5bMzfXQJNNELOAP0wdXOBFl4u1XtBUz7f \
pTrAhRUOw2Y5G1HrYqmTOP20CZyXaNaFoee_KS1xGkNFSOsIS_bnfzq-vPqRKngXHcv9EZeTSOPFJ4PgBruZCi \
c4dyEm2W-Qn389M5iOe5ushQbNAe2m97Z_gBhReNR_VkJlxwvgDA3KL2Zxr2_5oPbqc5zkL_wCl--UbA7PKNRh \
ut5QigOhJBZbUF7JJfoongvl4SLZV-muTaR-rP8bp3frm_lmde-UHcgg2mzBQWMR7lrysoNOUC866UXBPadO1o \
ARdR71BPPmrq4m_y_BDh5TQrDSvxCNsZv51Vtw-38s3auUQ4_XF6tVxdnuzt1fbW-zlZX2bVcXV4m11J-ubrer \
C7S1UpeJ4k8KySKryMvFkliFBoVicDP8OhM3yWrJFmRrNVFslovN6sszS5ub28vVzfr202OOUWVUhdLDpmtt2f1HZuEkDg8LJBr1z9EmuguhYNG8mXjd7a--y4zmzp5xqrv2PT_AwA60UM">
[Attachment #3 (text/plain)]
_______________________________________________
llvm-bugs mailing list
llvm-bugs@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-bugs
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic