[Clang] Fix FE crash during CGCoroutine GRO Alloca Emission

[yuxuanchen1997]

Fixes: #148953

Currently when coroutine return object type is const qualified, we don't do direct emission. The regular emission logic assumed that the auto var emission will always result in an AllocaInst. However, based on my findings, NRVO var emissions don't result in AllocaInsts. Therefore, this assertion will fail.

Since the NRVOed returned object don't live on the coroutine frame, we won't have the problem of it outliving the coroutine frame, therefore, we can safely omit this metadata.

[Copilot]

Pull Request Overview

This PR fixes a frontend crash in the clang compiler that occurs when emitting coroutine code for const-qualified return objects that are eligible for Named Return Value Optimization (NRVO). The crash was caused by an assertion failure when the code assumed all auto variable emissions would result in AllocaInst objects, but NRVO variables don't create allocas.

• Adds a guard to check if the coroutine return object is an NRVO variable before attempting to access its alloca
• Skips metadata assignment for NRVO variables since they don't live on the coroutine frame and don't have the outliving issue

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
clang/lib/CodeGen/CGCoroutine.cpp	Adds NRVO variable check to prevent assertion failure when setting coroutine frame metadata
clang/test/CodeGenCoroutines/coro-gro.cpp	Adds regression test for const-qualified coroutine return types with NRVO

[llvmbot]

@llvm/pr-subscribers-coroutines

@llvm/pr-subscribers-clang

Author: Yuxuan Chen (yuxuanchen1997)

Changes

Fixes: #148953

Currently when coroutine return object type is const qualified, we don't do direct emission. The regular emission logic assumed that the auto var emission will always result in an AllocaInst. However, based on my findings, NRVO var emissions don't result in AllocaInsts. Therefore, this assertion will fail.

Since the NRVOed returned object don't live on the coroutine frame, we won't have the problem of it outliving the coroutine frame, therefore, we can safely omit this metadata.

---

Full diff: https://github.com/llvm/llvm-project/pull/148962.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CGCoroutine.cpp (+9-5)
• (modified) clang/test/CodeGenCoroutines/coro-gro.cpp (+28-1)

diff --git a/clang/lib/CodeGen/CGCoroutine.cpp b/clang/lib/CodeGen/CGCoroutine.cpp
index 0fc488e98aaf0..117ef3d16e21b 100644
--- a/clang/lib/CodeGen/CGCoroutine.cpp
+++ b/clang/lib/CodeGen/CGCoroutine.cpp
@@ -707,11 +707,15 @@ struct GetReturnObjectManager {
     Builder.CreateStore(Builder.getFalse(), GroActiveFlag);
 
     GroEmission = CGF.EmitAutoVarAlloca(*GroVarDecl);
-    auto *GroAlloca = dyn_cast_or_null<llvm::AllocaInst>(
-        GroEmission.getOriginalAllocatedAddress().getPointer());
-    assert(GroAlloca && "expected alloca to be emitted");
-    GroAlloca->setMetadata(llvm::LLVMContext::MD_coro_outside_frame,
-                           llvm::MDNode::get(CGF.CGM.getLLVMContext(), {}));
+
+    if (!GroVarDecl->isNRVOVariable()) {
+      // NRVO variables don't have allocas and won't have the same issue.
+      auto *GroAlloca = dyn_cast_or_null<llvm::AllocaInst>(
+          GroEmission.getOriginalAllocatedAddress().getPointer());
+      assert(GroAlloca && "expected alloca to be emitted");
+      GroAlloca->setMetadata(llvm::LLVMContext::MD_coro_outside_frame,
+                             llvm::MDNode::get(CGF.CGM.getLLVMContext(), {}));
+    }
 
     // Remember the top of EHStack before emitting the cleanup.
     auto old_top = CGF.EHStack.stable_begin();
diff --git a/clang/test/CodeGenCoroutines/coro-gro.cpp b/clang/test/CodeGenCoroutines/coro-gro.cpp
index b62134317cef2..037fd03349e76 100644
--- a/clang/test/CodeGenCoroutines/coro-gro.cpp
+++ b/clang/test/CodeGenCoroutines/coro-gro.cpp
@@ -106,4 +106,31 @@ invoker g() {
   // CHECK: call void @_ZN7invoker15invoker_promise17get_return_objectEv({{.*}} %[[AggRes]]
   co_return;
 }
-// CHECK: ![[OutFrameMetadata]] = !{}
\ No newline at end of file
+
+namespace gh148953 {
+
+struct Task {
+  struct promise_type {
+    Task get_return_object();
+    std::suspend_always initial_suspend() { return {}; }
+    std::suspend_always final_suspend() noexcept { return {}; }
+    void return_void() {}
+    void unhandled_exception() {}
+  };
+  Task() {}
+  // Different from `invoker`, this Task is copy constructible.
+  Task(const Task&) {};
+};
+
+// NRVO on const qualified return type should work.
+// CHECK: define{{.*}} void @_ZN8gh1489537exampleEv({{.*}} sret(%"struct.gh148953::Task") align 1 %[[NrvoRes:.+]])
+const Task example() {
+  // CHECK: %[[ResultPtr:.+]] = alloca ptr
+  // CHECK: store ptr %[[NrvoRes]], ptr %[[ResultPtr]]
+  // CHECK: coro.init:
+  // CHECK: call void @_ZN8gh1489534Task12promise_type17get_return_objectEv({{.*}} %[[NrvoRes:.+]], {{.*}})
+  co_return;
+}
+
+} // namespace gh148953
+// CHECK: ![[OutFrameMetadata]] = !{}

[bcardosolopes]

LGTM

[vogelsgesang]

Afaict, we missed the clang branch cut by a couple of hours.
I think this might be worth backporting instead of waiting for clang-22? Or do you see any specific risks with backporting, @yuxuanchen1997 ?

[yuxuanchen1997]

Afaict, we missed the clang branch cut by a couple of hours.

I think this might be worth backporting instead of waiting for clang-22? Or do you see any specific risks with backporting, @yuxuanchen1997 ?

Would like to. Please advise next steps.

[efriedma-quic]

/cherry-pick c36156d

[llvmbot]

/pull-request #149013

[vogelsgesang]

Would like to. Please advise next steps.

efriedma already took care of creating the PR for the backport via the /cherry-pick c36156d command. It's now up to the original approver (@bcardosolopes) to also approve that backport PR. The goal of that second review is primarily to judge if this change is carries any risk to destabilize the upcoming release. (Usually, I think bug fixes, in particular early during the release cycle, carry little risk. That being said, I didn't look at the actual code changes here)