Skip to content

Conversation

Dinistro
Copy link
Contributor

This commit ensures that the coro split does not blindly copy memory attributes to generated coroutine functions. Copying things like memory(none) to functions that read or write memory causes UB.

This commit ensures that the coro split does not blindly copy memory
attributes to generated coroutine functions. Copying things like
`memory(none)` to functions that read or write memory causes UB.
@Dinistro Dinistro requested a review from jmorse August 27, 2025 12:28
@llvmbot
Copy link
Member

llvmbot commented Aug 27, 2025

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-coroutines

Author: Christian Ulmann (Dinistro)

Changes

This commit ensures that the coro split does not blindly copy memory attributes to generated coroutine functions. Copying things like memory(none) to functions that read or write memory causes UB.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/Coroutines/CoroSplit.cpp (+8-5)
  • (added) llvm/test/Transforms/Coroutines/coro-split-func-attrs.ll (+40)
diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
index 180ac9c61e7df..c349c8412d8a1 100644
--- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
@@ -619,8 +619,7 @@ static void replaceSwiftErrorOps(Function &F, coro::Shape &Shape,
 }
 
 /// Returns all debug records in F.
-static SmallVector<DbgVariableRecord *>
-collectDbgVariableRecords(Function &F) {
+static SmallVector<DbgVariableRecord *> collectDbgVariableRecords(Function &F) {
   SmallVector<DbgVariableRecord *> DbgVariableRecords;
   for (auto &I : instructions(F)) {
     for (DbgVariableRecord &DVR : filterDbgVars(I.getDbgRecordRange()))
@@ -933,15 +932,19 @@ void coro::BaseCloner::create() {
   auto NewAttrs = AttributeList();
 
   switch (Shape.ABI) {
-  case coro::ABI::Switch:
+  case coro::ABI::Switch: {
     // Bootstrap attributes by copying function attributes from the
     // original function.  This should include optimization settings and so on.
-    NewAttrs = NewAttrs.addFnAttributes(
-        Context, AttrBuilder(Context, OrigAttrs.getFnAttrs()));
+    AttrBuilder Builder(Context, OrigAttrs.getFnAttrs());
+    // Remove all the memory attribute, as copying too strict attributes caused
+    // UB.
+    Builder.removeAttribute(Attribute::AttrKind::Memory);
+    NewAttrs = NewAttrs.addFnAttributes(Context, std::move(Builder));
 
     addFramePointerAttrs(NewAttrs, Context, 0, Shape.FrameSize,
                          Shape.FrameAlign, /*NoAlias=*/false);
     break;
+  }
   case coro::ABI::Async: {
     auto *ActiveAsyncSuspend = cast<CoroSuspendAsyncInst>(ActiveSuspend);
     if (OrigF.hasParamAttribute(Shape.AsyncLowering.ContextArgNo,
diff --git a/llvm/test/Transforms/Coroutines/coro-split-func-attrs.ll b/llvm/test/Transforms/Coroutines/coro-split-func-attrs.ll
new file mode 100644
index 0000000000000..c1b0fba18101b
--- /dev/null
+++ b/llvm/test/Transforms/Coroutines/coro-split-func-attrs.ll
@@ -0,0 +1,40 @@
+; RUN: opt %s -passes='cgscc(coro-split),simplifycfg,early-cse' -S | FileCheck %s
+
+; Verify that memory attributes are not copied blindly to generated functions.
+
+declare ptr @aligned_alloc(i64, i64)
+declare void @free(ptr %mem)
+
+define ptr @f(i32 %a, i32 %b) presplitcoroutine memory(none) "keep" {
+entry:
+  %promise = alloca i32
+  %id = call token @llvm.coro.id(i32 0, ptr %promise, ptr null, ptr null)
+  %size = call i64 @llvm.coro.size.i64()
+  %align = call i64 @llvm.coro.align.i64()
+  %alloc = call ptr @aligned_alloc(i64 %size, i64 %align)
+  %hdl = call noalias ptr @llvm.coro.begin(token %id, ptr %alloc)
+  %s0 = call i8 @llvm.coro.suspend(token none, i1 false)
+  switch i8 %s0, label %suspend [i8 0, label %one_block
+                                 i8 1, label %cleanup]
+one_block:
+  %c = add nuw i32 %a, %b
+  store i32 %c, ptr %promise
+  %s1 = call i8 @llvm.coro.suspend(token none, i1 true)
+  switch i8 %s1, label %suspend [i8 0, label %trap
+                                 i8 1, label %cleanup]
+cleanup:
+  %mem = call ptr @llvm.coro.free(token %id, ptr %hdl)
+  call void @free(ptr %mem)
+  br label %suspend
+suspend:
+  %unused = call i1 @llvm.coro.end(ptr %hdl, i1 false, token none)
+  ret ptr %hdl
+trap:
+  call void @llvm.trap()
+  unreachable
+}
+
+; CHECK: @f.resume({{.*}}) #[[RESUME_ATTRS:[[:alnum:]]+]]
+; CHECK: @f.destroy({{.*}}) #[[RESUME_ATTRS]]
+; CHECK: @f.cleanup({{.*}}) #[[RESUME_ATTRS]]
+; CHECK: attributes #[[RESUME_ATTRS]] = { "keep" }

Copy link
Contributor

@NewSigma NewSigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand this pattern. Could you explain why do we add memory attributes to presplit coroutine?

@Dinistro
Copy link
Contributor Author

Dinistro commented Aug 29, 2025

@NewSigma I think we made a mistake in a downstream project. We are generating presplit coroutines from some other constructs and we accidentally forwarded the attributes there. I'll close this PR.

Sorry for the noise.

@Dinistro Dinistro closed this Aug 29, 2025
@Dinistro Dinistro deleted the users/dinistro/coro-func-attrs branch August 29, 2025 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants