[clang] 20e6515 - [Coroutines] Mark 'coroutine_handle<>::address' as always-inline

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2023-08-29T14:35:27+08:00
New Revision: 20e6515d5c5ff155a54e10f64caef1c76d8d5976

URL: https://github.com/llvm/llvm-project/commit/20e6515d5c5ff155a54e10f64caef1c76d8d5976
DIFF: https://github.com/llvm/llvm-project/commit/20e6515d5c5ff155a54e10f64caef1c76d8d5976.diff

LOG: [Coroutines] Mark 'coroutine_handle<>::address' as always-inline

Close https://github.com/llvm/llvm-project/issues/65054

The direct issue is still the call to coroutine_handle<>::address()
after await_suspend(). Without optimizations, the current logic will put
the temporary result of await_suspend() to the coroutine frame since the
middle end feel the temporary is escaped from
coroutine_handle<>::address. To fix this fundamentally, we should wrap
the whole logic about await-suspend into a standalone function. See
https://github.com/llvm/llvm-project/issues/64945

And as a short-term workaround, we probably can mark
coroutine_handle<>::address() as always-inline so that the temporary
result may not be thought to be escaped then it won't be put on the
coroutine frame. Although it looks dirty, it is probably do-able since
the compiler are allowed to do special tricks to standard library
components.

Added: 
    clang/test/CodeGenCoroutines/pr65054.cpp

Modified: 
    clang/lib/Sema/SemaCoroutine.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
index 0b2987376b8b3b..42e428344888a8 100644
--- a/clang/lib/Sema/SemaCoroutine.cpp
+++ b/clang/lib/Sema/SemaCoroutine.cpp
@@ -344,6 +344,28 @@ static Expr *maybeTailCall(Sema &S, QualType RetType, Expr *E,
 
   Expr *JustAddress = AddressExpr.get();
 
+  // FIXME: Without optimizations, the temporary result from `await_suspend()`
+  // may be put on the coroutine frame since the coroutine frame constructor
+  // will think the temporary variable will escape from the
+  // `coroutine_handle<>::address()` call. This is problematic since the
+  // coroutine should be considered to be suspended after it enters
+  // `await_suspend` so it shouldn't access/update the coroutine frame after
+  // that.
+  //
+  // See https://github.com/llvm/llvm-project/issues/65054 for the report.
+  //
+  // The long term solution may wrap the whole logic about `await-suspend`
+  // into a standalone function. This is similar to the proposed solution
+  // in tryMarkAwaitSuspendNoInline. See the comments there for details.
+  //
+  // The short term solution here is to mark `coroutine_handle<>::address()`
+  // function as always-inline so that the coroutine frame constructor won't
+  // think the temporary result is escaped incorrectly.
+  if (auto *FD = cast<CallExpr>(JustAddress)->getDirectCallee())
+    if (!FD->hasAttr<AlwaysInlineAttr>() && !FD->hasAttr<NoInlineAttr>())
+      FD->addAttr(AlwaysInlineAttr::CreateImplicit(S.getASTContext(),
+                                                   FD->getLocation()));
+
   // Check that the type of AddressExpr is void*
   if (!JustAddress->getType().getTypePtr()->isVoidPointerType())
     S.Diag(cast<CallExpr>(JustAddress)->getCalleeDecl()->getLocation(),

diff  --git a/clang/test/CodeGenCoroutines/pr65054.cpp b/clang/test/CodeGenCoroutines/pr65054.cpp
new file mode 100644
index 00000000000000..834b71050f59ff
--- /dev/null
+++ b/clang/test/CodeGenCoroutines/pr65054.cpp
@@ -0,0 +1,60 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 \
+// RUN:      -O0 -disable-llvm-passes -emit-llvm %s -o - \
+// RUN:      | FileCheck %s --check-prefix=FRONTEND
+
+// The output of O0 is highly redundant and hard to test. Also it is not good
+// limit the output of O0. So we test the optimized output from O0. The idea
+// is the optimizations shouldn't change the semantics of the program.
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++20 \
+// RUN:      -O0 -emit-llvm %s -o - -disable-O0-optnone \
+// RUN:      | opt -passes='sroa,mem2reg,simplifycfg' -S | FileCheck %s --check-prefix=CHECK-O0
+
+#include "Inputs/coroutine.h"
+
+// A simple awaiter type with an await_suspend method that can't be
+// inlined.
+struct Awaiter {
+  const int& x;
+
+  bool await_ready() { return false; }
+  std::coroutine_handle<> await_suspend(const std::coroutine_handle<> h);
+  void await_resume() {}
+};
+
+struct MyTask {
+  // A lazy promise with an await_transform method that supports awaiting
+  // integer references using the Awaiter struct above.
+  struct promise_type {
+    MyTask get_return_object() {
+      return MyTask{
+          std::coroutine_handle<promise_type>::from_promise(*this),
+      };
+    }
+
+    std::suspend_always initial_suspend() { return {}; }
+    std::suspend_always final_suspend() noexcept { return {}; }
+    void unhandled_exception();
+
+    auto await_transform(const int& x) { return Awaiter{x}; }
+  };
+
+  std::coroutine_handle<> h;
+};
+
+// A global array of integers.
+int g_array[32];
+
+// A coroutine that awaits each integer in the global array.
+MyTask FooBar() {
+  for (const int& x : g_array) {
+    co_await x;
+  }
+}
+
+// FRONTEND: define{{.*}}@_ZNKSt16coroutine_handleIvE7addressEv{{.*}}#[[address_attr:[0-9]+]]
+// FRONTEND: attributes #[[address_attr]] = {{.*}}alwaysinline
+
+// CHECK-O0: define{{.*}}@_Z6FooBarv.resume
+// CHECK-O0: call{{.*}}@_ZN7Awaiter13await_suspendESt16coroutine_handleIvE
+// CHECK-O0-NOT: store
+// CHECK-O0: ret void