-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[Coroutines] Avoid copying memory attributes to generated functions #155611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-coroutines Author: Christian Ulmann (Dinistro) ChangesThis commit ensures that the coro split does not blindly copy memory attributes to generated coroutine functions. Copying things like Full diff: https://github.com/llvm/llvm-project/pull/155611.diff 2 Files Affected:
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" }
|
There was a problem hiding this 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?
@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. |
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.