[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
https://github.com/bcardosolopes closed https://github.com/llvm/llvm-project/pull/66706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
https://github.com/ChuanqiXu9 approved this pull request. LGTM. Thanks. https://github.com/llvm/llvm-project/pull/66706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
apolloww wrote: Thanks for the fix! I think originally the deferred conversion is placed after `coro.end` so that it won't be counted as "use across suspend point", but it doesn't stop optimizer from breaking that assumption. https://github.com/llvm/llvm-project/pull/66706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
bcardosolopes wrote: Applied suggested changes and updated PR. https://github.com/llvm/llvm-project/pull/66706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
https://github.com/bcardosolopes resolved https://github.com/llvm/llvm-project/pull/66706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
@@ -104,3 +105,5 @@ invoker g() { // CHECK: call void @_ZN7invoker15invoker_promise17get_return_objectEv({{.*}} %[[AggRes]] co_return; } + +// CHECK: ![[OutFrameMetadata]] = !{} bcardosolopes wrote: Done https://github.com/llvm/llvm-project/pull/66706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
https://github.com/bcardosolopes resolved https://github.com/llvm/llvm-project/pull/66706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
@@ -6691,6 +6691,22 @@ sections that the user does not want removed after linking. ... !0 = !{} +'``coro.outside.frame``' Metadata +^^ + +``coro.outside.frame`` metadata may be attached to an alloca instruction to +to signify that it shouldn't be promoted to the coroutine frame, useful for +filtering allocas out by the frontend when emitting internal control mechanisms. +Additionally, this metadata is only used as a flag, so the associated +node must be empty. + +.. code-block:: text + + %__coro_gro = alloca %struct.GroType, align 1, !coro.outside.frame !0 + + ... + !0 = !{} + bcardosolopes wrote: Done! https://github.com/llvm/llvm-project/pull/66706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
https://github.com/bcardosolopes updated https://github.com/llvm/llvm-project/pull/66706 >From 6312dd56ed3a2f47e7291ae32ca044622a317259 Mon Sep 17 00:00:00 2001 From: Bruno Cardoso Lopes Date: Wed, 20 Sep 2023 15:00:06 -0700 Subject: [PATCH 1/2] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise When dealing with short-circuiting coroutines (e.g. expected), the deferred calls that resolve the get_return_object are currently being emitted after we delete the coroutine frame. This was caught by ASAN when using optimizations -O1 and above: optimizations after inlining would place the __coro_gro in the heap, and subsequent delete of the coroframe followed by the conversion -> BOOM. This patch forbids the GRO to be placed in the coroutine frame, by adding a new metadata node that can be attached to `alloca` instructions. This fixes #49843. --- clang/lib/CodeGen/CGCoroutine.cpp| 5 + clang/test/CodeGenCoroutines/coro-gro.cpp| 5 - llvm/docs/LangRef.rst| 16 llvm/include/llvm/IR/FixedMetadataKinds.def | 1 + llvm/lib/Transforms/Coroutines/CoroFrame.cpp | 5 + 5 files changed, 31 insertions(+), 1 deletion(-) diff --git a/clang/lib/CodeGen/CGCoroutine.cpp b/clang/lib/CodeGen/CGCoroutine.cpp index 448943084acedf3..58310216ecff1f5 100644 --- a/clang/lib/CodeGen/CGCoroutine.cpp +++ b/clang/lib/CodeGen/CGCoroutine.cpp @@ -535,6 +535,11 @@ struct GetReturnObjectManager { Builder.CreateStore(Builder.getFalse(), GroActiveFlag); GroEmission = CGF.EmitAutoVarAlloca(*GroVarDecl); +auto *GroAlloca = dyn_cast_or_null( +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 b48b769950ae871..ba872e39f4e3de8 100644 --- a/clang/test/CodeGenCoroutines/coro-gro.cpp +++ b/clang/test/CodeGenCoroutines/coro-gro.cpp @@ -30,12 +30,13 @@ void doSomething() noexcept; int f() { // CHECK: %[[RetVal:.+]] = alloca i32 // CHECK: %[[GroActive:.+]] = alloca i1 + // CHECK: %[[CoroGro:.+]] = alloca %struct.GroType, {{.*}} !coro.outside.frame ![[OutFrameMetadata:.+]] // CHECK: %[[Size:.+]] = call i64 @llvm.coro.size.i64() // CHECK: call noalias noundef nonnull ptr @_Znwm(i64 noundef %[[Size]]) // CHECK: store i1 false, ptr %[[GroActive]] // CHECK: call void @_ZNSt16coroutine_traitsIiJEE12promise_typeC1Ev( - // CHECK: call void @_ZNSt16coroutine_traitsIiJEE12promise_type17get_return_objectEv( + // CHECK: call void @_ZNSt16coroutine_traitsIiJEE12promise_type17get_return_objectEv({{.*}} %[[CoroGro]] // CHECK: store i1 true, ptr %[[GroActive]] Cleanup cleanup; @@ -104,3 +105,5 @@ invoker g() { // CHECK: call void @_ZN7invoker15invoker_promise17get_return_objectEv({{.*}} %[[AggRes]] co_return; } + +// CHECK: ![[OutFrameMetadata]] = !{} \ No newline at end of file diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst index f542e70bcfee810..9e3f8962ce1b52e 100644 --- a/llvm/docs/LangRef.rst +++ b/llvm/docs/LangRef.rst @@ -6691,6 +6691,22 @@ sections that the user does not want removed after linking. ... !0 = !{} +'``coro.outside.frame``' Metadata +^^ + +``coro.outside.frame`` metadata may be attached to an alloca instruction to +to signify that it shouldn't be promoted to the coroutine frame, useful for +filtering allocas out by the frontend when emitting internal control mechanisms. +Additionally, this metadata is only used as a flag, so the associated +node must be empty. + +.. code-block:: text + + %__coro_gro = alloca %struct.GroType, align 1, !coro.outside.frame !0 + + ... + !0 = !{} + '``unpredictable``' Metadata diff --git a/llvm/include/llvm/IR/FixedMetadataKinds.def b/llvm/include/llvm/IR/FixedMetadataKinds.def index 8723bf2a0680c77..b375d0f0912060f 100644 --- a/llvm/include/llvm/IR/FixedMetadataKinds.def +++ b/llvm/include/llvm/IR/FixedMetadataKinds.def @@ -50,3 +50,4 @@ LLVM_FIXED_MD_KIND(MD_callsite, "callsite", 35) LLVM_FIXED_MD_KIND(MD_kcfi_type, "kcfi_type", 36) LLVM_FIXED_MD_KIND(MD_pcsections, "pcsections", 37) LLVM_FIXED_MD_KIND(MD_DIAssignID, "DIAssignID", 38) +LLVM_FIXED_MD_KIND(MD_coro_outside_frame, "coro.outside.frame", 39) diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp index a12dd710174f3f8..9c1ee322ce0b177 100644 --- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp +++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp @@ -2804,6 +2804,11 @@ static void collectFrameAlloca(AllocaInst *AI, coro::Shape , if (AI
[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
https://github.com/ChuanqiXu9 edited https://github.com/llvm/llvm-project/pull/66706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
https://github.com/ChuanqiXu9 commented: Thanks. It looks good if we can add llvm test in llvm/test/Transforms/Coroutines for the new metadata. https://github.com/llvm/llvm-project/pull/66706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
@@ -104,3 +105,5 @@ invoker g() { // CHECK: call void @_ZN7invoker15invoker_promise17get_return_objectEv({{.*}} %[[AggRes]] co_return; } + +// CHECK: ![[OutFrameMetadata]] = !{} ChuanqiXu9 wrote: nit: no newline https://github.com/llvm/llvm-project/pull/66706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
@@ -6691,6 +6691,22 @@ sections that the user does not want removed after linking. ... !0 = !{} +'``coro.outside.frame``' Metadata +^^ + +``coro.outside.frame`` metadata may be attached to an alloca instruction to +to signify that it shouldn't be promoted to the coroutine frame, useful for +filtering allocas out by the frontend when emitting internal control mechanisms. +Additionally, this metadata is only used as a flag, so the associated +node must be empty. + +.. code-block:: text + + %__coro_gro = alloca %struct.GroType, align 1, !coro.outside.frame !0 + + ... + !0 = !{} + ChuanqiXu9 wrote: Maybe it is better to add this to a new section of `Coroutines.rst`. We add the coroutines related things there generally. https://github.com/llvm/llvm-project/pull/66706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
https://github.com/ChuanqiXu9 edited https://github.com/llvm/llvm-project/pull/66706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
bcardosolopes wrote: Thanks for the clarifications > By forcing the GRO not living on the coroutine frames, it shouldn't be a > problem if the lifetime of `__coro_gro` outlives the `__promise`. The only > limit is that the initialization of `__coro_gro` should be in the lifetime of > `__promise`. Applied your suggestion based on #49843, and edited description + title. https://github.com/llvm/llvm-project/pull/66706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][LLVM][Coroutines] Prevent __coro_gro from outliving __promise (PR #66706)
https://github.com/bcardosolopes edited https://github.com/llvm/llvm-project/pull/66706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits