[PATCH] D157070: [C++20] [Coroutines] Introduce `@llvm.coro.opt.blocker` to block the may-be-incorrect optimization for awaiter

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu created this revision.
ChuanqiXu added reviewers: ilya-biryukov, rjmccall, cor3ntin, weiwang, bruno, clang-language-wg.
Herald added a subscriber: hiraditya.
Herald added a project: All.
ChuanqiXu requested review of this revision.
Herald added subscribers: llvm-commits, jdoerfert.
Herald added a project: LLVM.

Fixed https://github.com/llvm/llvm-project/issues/56301.

Given its context is pretty long, I'll give it a simple summarize here.

The issue pattern here is:

  C++
  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();
      }
    }
  };

In the example, the program would only access the coroutine frame conditionally. However, the optimizer failed to think the variable `suspended ` may be aliased with the coroutine handle `h` in `await_suspend`. In this case, the value of `suspended` is the same with the nullness of `to_resume`. So the variable `suspended` is optimized out and the nullness of the `to_resume` is stored to the coroutine frame **unconditionally**. So that it is a UB if the coroutine handle get destroyed in `awaitable.Register()` in another thread while we access the coroutine frame.

The root cause of the problem is that the optimizer can't recognize the may_alias relationship between the data members of the awaiter with the coroutine handle. So I thought I must fight with AA initially. But I realized that it is sufficient to mark the awaiter as escaped during the await_suspend. This is not a workaround but a proper fix to the underlying issue since the C++ language doesn't allow the programmers to access the coroutine handle except via `await_suspend`.

And it is pretty easy to mark certain variables as escaped. We can make it by passing its address to a foreign function simply. So it is the framework of the patch. We introduced a LLVM intrinsic `@llvm.coro.opt.blocker` to block the optimization of awaiter.

Another important point is that we shouldn't make it for awaiter with no non-static data members. The instance of an empty class  may still occupy 1 byte due to the requirement of C++ language. But the optimizer can optimize such variables out if it can inline `await_ready`, `await_suspend` and `await_resume`. So it doesn't matter. However, it won't be done after we introduced `@llvm.coro.opt.blocker`. This is not acceptable. So I spent some time in this page to make it not happen with the empty awaiter.

Ideally, it will be better to emit  `@llvm.coro.opt.blocker` only if we observed the coroutine handle leaked from await_suspend and there is conditional access after that. But it is more complex and it is also questionable if the benefits worth the cost. Currently, coroutines already compile slowly. So let's leave it to the future.


https://reviews.llvm.org/D157070

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGCoroutine.cpp
  clang/test/CodeGenCoroutines/coro-awaiter-addr.cpp
  clang/test/CodeGenCoroutines/coro-symmetric-transfer-02.cpp
  clang/test/CodeGenCoroutines/pr56301.cpp
  llvm/docs/Coroutines.rst
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Transforms/Coroutines/CoroInstr.h
  llvm/lib/Transforms/Coroutines/Coroutines.cpp
  llvm/test/Transforms/Coroutines/coro-opt-blocker.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D157070.547104.patch
Type: text/x-patch
Size: 26102 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230804/b8222b70/attachment.bin>