<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_RrnhYjhyJ3bQx5ymWAD7Owsumex2KeAkmibHYnUiFTc7q_fU0XqajlpTMOTm8i6Hladoia1-eHuZZFcvyux017sd9KLF-F32rwJ7YQUpXaZLStdSK-tEWmzFdqIrJBmu0gesUHVCsv2tfaKdpT2XeW0RBqhi0JtZSH28gCRkKyNr23eZIok59JLUcsM202-SJ5tLRqnhN2IxmijvZaF_glZpSptfViKf5Amtskr50UmsbhWJC0Xm9qWojzgd6jLbK5SerpIbhaXD49svqrFbz-qwtaqXlw-4cnO-8ot1vfQjM_W5qkt_NLWcOv5J_7L3P8y9zPL60VySx-sXD0tVu3Xq1X8hF-TtTZZ0eRQun7MbG0br41arH-be45nhfaH_il_db5uMi--2VLd76X2Mi2w-vohPBUiGCq-KteUCvFUwjVVVWi4v0MEsdiaLDyQ3f5UIXsINwKTH5CwibBa-aZGtLeIahTCiXI72xQ5dmMFqcuFsXuxQYpYr3zjPLlDWSpf60z4Whq3UfVyIv9lM7GH8lewMZ1J0QgCjCorf2gN2WsPK7zILdtnDoTK7UTDRN-T2iDsEX8WqutgWkQvgOWX4l_WRzchEYgFOjNrvNRmIo2AFONxMNmutgaARDDJsl7-jhzpzHI-J6is7zsQvAZ_kHckHOnbaucBwuTmk6UMupj9xfVT_3NE1C87HDa8W52LJ50b_61xlaIjdzNSMQQhAxCyBvAb4XKRXOHT57WTIkRqbQFksgqVT8THx7zvNQKArCBFLQ42snAkUJDTo72fxTaIdZ17yCoKxWe7dlF_q6VL_73jk7YnKBadT4hpoZwTVY1KZwTHfK8dHbXC7pfiv0qUjSPYHglkuC2S-1ThJCGG9yhiRUHo7lHxKJymYwytBEycwBzVrrYHJDk9tEmeEU3RFLIFqm1cgdVOOAsY4Dvk7eQ7neegSfWQFX26UJ2eAKBGHeWtj4r3dEyFt3xQUQc2VCQqxFqnXNe4TnDd0NQknqMD5CcVgy4NAwNCpmQDod6-hprDtnQIWw4Ozu4YtyREb7jiJxediKPMdl78-WFVWooXE7LFXQaxe6P6x6WgtUPkOEvonH5G-keBGYcAxwqL8HvODzYy80ENdH6gAMsaQOcgOrA_UhdFZwRQg4bY9LyNJRzNFVWiGMd9mvr27A1XxGO5G_356GxO-xMxCfUOA7p47aXxjqzoTBoYEpX0meuVHaniStaWkJDmm1NnGA1oEMlhuEpqYoZagd2EkFPwlzNw6ovZMZw-KKijMA3dOKrlsej-fvhTureBgvh3VBqwMPXqD5Ua6w87xFZRJCiArzb9rjJ_XFRpG-sdmjSoqNHHV8M5i_RrUEr_pjhZgPs5gR51JM1Y9SNTKCSfi-WENyZU7fw17EN7m-1gtIELSUBIYCa2LrH44xZ2nNxoVWyGeNj5RywXAoebh2mdSf3pTh6T-Gztg6yPoZzZV1YzbcA3U34wy0v5KxU7nStJdS7ynlj9KNcoC2VVqFIZH3pM25SkgJyu7F6txKYxGdMKYhtUzJTemiO-EVTkKoNKFyYKZAPtcMKg0f4iPezK4Qzv1KCYuUYtKZgrQnzICs0BpkG1a2mrhVW5ziRzO-2Wn-ripj7QBBdCA2YdYUpxiHlo8QM9sZ6iI9Bqquk9ESYiWdtiSaXjd3mAeASug42xtnrtp4OQw6vVkr4NzH1uam7yYb4yXMFsKGBd4d9roiJVW9EoX1hmFBXyq9WoOiJznIDgzcYW4Cnn2Lq39Rv7O-x1taqw7QgLFDoIGpW7ASK4j-0VD3AMm75mYhUmuDS0ImAleyOnOKTYMG1koCXozYRJxzOlh_uBLnWmjEHZdr5AfzaE6JZ6LKdngIbXF-GUIpfLQz8otucCOMGHfA3zLDoDVDAfCkGceoaNuSokkbPGYKqLFjDQJLVie2QuaYj8op9fu4mWcx6pGqWLZx5QwhIDeN3PwHyqjgZbsdXvAC0nuQ4zephVoPicQfiULZIHfJKVOP_jQpxvjD3vyqkj-z4cc4c1CivFR_8w8orFl9Voy6C2ClE1bsdVNv0xfoDiyt9x7ikc62T8mAOLf7ZStfRw1CgcxJsGNGtLo1BhiXXezsskZTRyyhM6_9oTFas8Rp_LB1pFdwWPxOWc8pMaffmQQZcSy3jOLp9-TSZC8yBufklypM5HogsUc3YnhGgg9-LmpB3pAZCemHFxHe1YnYiX07PxapMwbkpc5OYm3NdX8-Xm6uLkHEYdPo68vxDCNNiefIm2H1nHx4msl_PZzqh_D56fQmAM70nv59AyxritOtiNHqCJHR-pWUidOmYz5-tzSH565trzAccnEatmEpHrcSImIPpuFH9f_vPh4eL1xDGeYdC8rt20vv81SMyb8LcTMDh5JzLwcQJOndwuA6cw1ZU1NBbliSTw4aC-MS2grW9TUZ1vJ9lgz4I2tUS_kHle06UGNV5D7YLiBWIi7rmfnW69oYkRIeuvISHEwQG-laKeP6fPt4Uv-RIGV-I5LRODz3FQtrXe0uzQiu5Y4QYsDYTIhStDvqINPIBZXHRi7jhQH9-pztTQhmeMIj6E_irJDTjxU9V0NUEXgdjg9E_VUqLZMZHUgArmmsnRMPoPjW9Nx_ApnNwwp-SAzUfTzEYw3kJFUWQe4VSaQ6Sr77JoVEeJW34z5vPe2rdPryiiHW186VKK2UklnaNMcNDIhcmFKDHC8Z_6vYSnQFTbYwJrrBlmJj3wb16HGziMuELCIsPZIHZGeGR80ZXut0CQwozAwJnqbe934iVYgNHYUXKPyKE0kzcQIZZDeIxSen5-Pvy1fX3iI42dvkNZgrgh3jnso2CrH5LGr9kEDa6mISYjrxE6GJnFaYuEQH7j1Y8ggKLnZhM4n6Z-nmHmyRfbLd08VhixOBLQX43rlm7ykdK18zwGtXSe3xxo0P1W1ICB0wQURwhK0ISFXiz5vm7Iyccs3PQzj-uHE6pD6q9GGWLYcYg4zczD_S3heV7cKOVJsOiEpMHbAjeISDs925kocvJCIibpG-ldk97R-4gYzRlX2zFoFg0jqV-W4dbu1GLB19Y3LxQirSg9z5Svz4J7NIQGHyUAOsngyJpLjm3di_wg7wj4eYzgTlZ44oa28N-P9420feMGhQ17SRV58FayvaV_xNzo4zS1k65_iQnZhgsVj4Bt8IK1833vo7JB7z95rIslHfTG70g7JlaY7sP0zvUwb1BMoO7la5w3Lea3ssUCF-aIu-kr0baK80GW8SUqFyoUvjK0Or4SoTaah3IRwiwDdlPl91hJIf329Y_7f0MXZYJk4vsj3P1WoY_Ev89c-fyHx-TJ9j5ulAUuQFQSY1OcDSXn5bMzfXQJNNELOAP0wdXOBFl4u1XtBUz7fpTrAhRUOw2Y5G1HrYqmTOP20CZyXaNaFoee_KS1xGkNFSOsIS_bnfzq-vPqRKngXHcv9EZeTSOPFJ4PgBruZCic4dyEm2W-Qn389M5iOe5ushQbNAe2m97Z_gBhReNR_VkJlxwvgDA3KL2Zxr2_5oPbqc5zkL_wCl--UbA7PKNRhut5QigOhJBZbUF7JJfoongvl4SLZV-muTaR-rP8bp3frm_lmde-UHcgg2mzBQWMR7lrysoNOUC866UXBPadO1oARdR71BPPmrq4m_y_BDh5TQrDSvxCNsZv51Vtw-38s3auUQ4_XF6tVxdnuzt1fbW-zlZX2bVcXV4m11J-ubrerC7S1UpeJ4k8KySKryMvFkliFBoVicDP8OhM3yWrJFmRrNVFslovN6sszS5ub28vVzfr202OOUWVUhdLDpmtt2f1HZuEkDg8LJBr1z9EmuguhYNG8mXjd7a--y4zmzp5xqrv2PT_AwA60UM">