[Coroutines] Create C++ noop coroutine with default function attributes #134878
Conversation
This change makes the C++ noop coroutine to be built with attributes based on the module flags. Among other things, this adds function attributes related to the Pointer Authentication and Branch Target Enforcement module flags, which are set by command-line options. Before this patch, C++ programs built with either PAC-RET or BTI that had the noop coroutine emitted could have a runtime error because this function wasn't compatible with the rest of the program.
|
@llvm/pr-subscribers-coroutines Author: Victor Campos (vhscampos) ChangesThis change makes the C++ noop coroutine to be built with attributes based on the module flags. Among other things, this adds function attributes related to the Pointer Authentication and Branch Target Enforcement module flags, which are set by command-line options. Before this patch, C++ programs built with either PAC-RET or BTI that had the noop coroutine emitted could have a runtime error because this function wasn't compatible with the rest of the program. Full diff: https://github.com/llvm/llvm-project/pull/134878.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp
index 5375448d2d2e2..fd0c5e2e240c2 100644
--- a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp
@@ -130,9 +130,10 @@ void Lowerer::lowerCoroNoop(IntrinsicInst *II) {
StructType::create({FnPtrTy, FnPtrTy}, "NoopCoro.Frame");
// Create a Noop function that does nothing.
- Function *NoopFn =
- Function::Create(FnTy, GlobalValue::LinkageTypes::PrivateLinkage,
- "__NoopCoro_ResumeDestroy", &M);
+ Function *NoopFn = Function::createWithDefaultAttr(
+ FnTy, GlobalValue::LinkageTypes::PrivateLinkage,
+ M.getDataLayout().getProgramAddressSpace(), "__NoopCoro_ResumeDestroy",
+ &M);
NoopFn->setCallingConv(CallingConv::Fast);
buildDebugInfoForNoopResumeDestroyFunc(NoopFn);
auto *Entry = BasicBlock::Create(C, "entry", NoopFn);
diff --git a/llvm/test/Transforms/Coroutines/coro-noop-pacbti.ll b/llvm/test/Transforms/Coroutines/coro-noop-pacbti.ll
new file mode 100644
index 0000000000000..4f302a6acc649
--- /dev/null
+++ b/llvm/test/Transforms/Coroutines/coro-noop-pacbti.ll
@@ -0,0 +1,23 @@
+
+; RUN: opt < %s -S -passes=coro-early | FileCheck %s
+
+; CHECK: define private fastcc void @__NoopCoro_ResumeDestroy(ptr %0) #1 {
+; CHECK-NEXT: entry:
+; CHECK-NEXT: ret void
+; CHECK-NEXT: }
+
+; CHECK: attributes #1 = { "branch-target-enforcement" "sign-return-address"="all" "sign-return-address-key"="a_key" }
+
+define ptr @noop() {
+entry:
+ %n = call ptr @llvm.coro.noop()
+ ret ptr %n
+}
+
+declare ptr @llvm.coro.noop()
+
+!llvm.module.flags = !{!0, !1, !2}
+
+!0 = !{i32 8, !"branch-target-enforcement", i32 1}
+!1 = !{i32 8, !"sign-return-address", i32 1}
+!2 = !{i32 8, !"sign-return-address-all", i32 1}
|
|
@llvm/pr-subscribers-llvm-transforms Author: Victor Campos (vhscampos) ChangesThis change makes the C++ noop coroutine to be built with attributes based on the module flags. Among other things, this adds function attributes related to the Pointer Authentication and Branch Target Enforcement module flags, which are set by command-line options. Before this patch, C++ programs built with either PAC-RET or BTI that had the noop coroutine emitted could have a runtime error because this function wasn't compatible with the rest of the program. Full diff: https://github.com/llvm/llvm-project/pull/134878.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp
index 5375448d2d2e2..fd0c5e2e240c2 100644
--- a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp
@@ -130,9 +130,10 @@ void Lowerer::lowerCoroNoop(IntrinsicInst *II) {
StructType::create({FnPtrTy, FnPtrTy}, "NoopCoro.Frame");
// Create a Noop function that does nothing.
- Function *NoopFn =
- Function::Create(FnTy, GlobalValue::LinkageTypes::PrivateLinkage,
- "__NoopCoro_ResumeDestroy", &M);
+ Function *NoopFn = Function::createWithDefaultAttr(
+ FnTy, GlobalValue::LinkageTypes::PrivateLinkage,
+ M.getDataLayout().getProgramAddressSpace(), "__NoopCoro_ResumeDestroy",
+ &M);
NoopFn->setCallingConv(CallingConv::Fast);
buildDebugInfoForNoopResumeDestroyFunc(NoopFn);
auto *Entry = BasicBlock::Create(C, "entry", NoopFn);
diff --git a/llvm/test/Transforms/Coroutines/coro-noop-pacbti.ll b/llvm/test/Transforms/Coroutines/coro-noop-pacbti.ll
new file mode 100644
index 0000000000000..4f302a6acc649
--- /dev/null
+++ b/llvm/test/Transforms/Coroutines/coro-noop-pacbti.ll
@@ -0,0 +1,23 @@
+
+; RUN: opt < %s -S -passes=coro-early | FileCheck %s
+
+; CHECK: define private fastcc void @__NoopCoro_ResumeDestroy(ptr %0) #1 {
+; CHECK-NEXT: entry:
+; CHECK-NEXT: ret void
+; CHECK-NEXT: }
+
+; CHECK: attributes #1 = { "branch-target-enforcement" "sign-return-address"="all" "sign-return-address-key"="a_key" }
+
+define ptr @noop() {
+entry:
+ %n = call ptr @llvm.coro.noop()
+ ret ptr %n
+}
+
+declare ptr @llvm.coro.noop()
+
+!llvm.module.flags = !{!0, !1, !2}
+
+!0 = !{i32 8, !"branch-target-enforcement", i32 1}
+!1 = !{i32 8, !"sign-return-address", i32 1}
+!2 = !{i32 8, !"sign-return-address-all", i32 1}
|
|
Thanks for the review @ChuanqiXu9. I am also tagging @efriedma-quic because this PR is somewhat similar to the other one regarding C++ module initializers (#133716) |
There was a problem hiding this comment.
The cases where Function::createWithDefaultAttr is really necessary are cases where it isn't possible to set appropriate per-function attributes. Lowering is generating the function from scratch, so it has to work "everywhere", even if we don't have access to the full set of target flags from the frontend. Like during LTO.
We really don't want to generate code this way if we can avoid it: we don't get any debug info, the assumed target is the bare minimum for the triple, we're skipping over all the normal ABI handling for the type. So... I'd prefer not to add new code depending on the "default-attrs" infrastructure if we can avoid it, but I guess it isn't a big deal in this context.
Really, why aren't we generating this in the frontend? I don't see the benefit to having a special "no-op coroutine", as opposed to just generating a coroutine that's a no-op.
On a related note, what's going on with the calling-convention stuff? Passing around function pointers to CallingConv::Fast functions makes me very concerned: CallingConv::Fast does not have a stable ABI.
I am not sure. This is historic. The people who implemented it is not active now. The only reason I can see may be the possible optimization. e.g., if we know we're going to resume/destroy a noop coroutine, we can save one memory access. |
|
@efriedma-quic Thanks for the review. Honestly C++ coroutines and their codegen are out of my depth, so if possible I'd like to proceed with a small fix here. If you believe it's more suitable, I can change the code to add function attributes of pac-ret and bti only, instead of calling |
This change makes the C++ noop coroutine to be built with attributes based on the module flags. Among other things, this adds function attributes related to the Pointer Authentication and Branch Target Enforcement module flags, which are set by command-line options.
Before this patch, C++ programs built with either PAC-RET or BTI that had the noop coroutine emitted could have a runtime error because this function wasn't compatible with the rest of the program.