-
Notifications
You must be signed in to change notification settings - Fork 15.4k
Skip stack protectors on alloca's which have new metadata to opt out #170229
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
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-backend-aarch64 Author: None (cooperp) ChangesThis is the LLVM piece of this work. There is also a clang piece, which adds this metadata to AllocaInst when the source does We already have Full diff: https://github.com/llvm/llvm-project/pull/170229.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/StackProtector.cpp b/llvm/lib/CodeGen/StackProtector.cpp
index 5fd5d6cce23df..66ea09975f412 100644
--- a/llvm/lib/CodeGen/StackProtector.cpp
+++ b/llvm/lib/CodeGen/StackProtector.cpp
@@ -431,6 +431,11 @@ bool SSPLayoutAnalysis::requiresStackProtector(Function *F,
for (const BasicBlock &BB : *F) {
for (const Instruction &I : BB) {
if (const AllocaInst *AI = dyn_cast<AllocaInst>(&I)) {
+ if (MDNode* MD = AI->getMetadata("stack-protector")) {
+ auto* CI = mdconst::dyn_extract<ConstantInt>(MD->getOperand(0));
+ if (CI->isZero())
+ continue;
+ }
if (AI->isArrayAllocation()) {
auto RemarkBuilder = [&]() {
return OptimizationRemark(DEBUG_TYPE, "StackProtectorAllocaOrArray",
diff --git a/llvm/test/CodeGen/AArch64/stack-protector-metadata.ll b/llvm/test/CodeGen/AArch64/stack-protector-metadata.ll
new file mode 100644
index 0000000000000..a0aa8859b1887
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/stack-protector-metadata.ll
@@ -0,0 +1,58 @@
+; RUN: llc -mtriple=aarch64-apple-darwin < %s -o - | FileCheck %s
+
+target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-n32:64-S128-Fn32"
+target triple = "arm64-apple-darwin25.1.0"
+
+@.str = private unnamed_addr constant [4 x i8] c"%s\0A\00", align 1
+
+; CHECK-LABEL: test1:
+; CHECK-NOT: ___stack_chk_guard
+
+; Function Attrs: noinline nounwind optnone
+define void @test1(ptr noundef %msg) #0 {
+entry:
+ %msg.addr = alloca ptr, align 8
+ %a = alloca [1000 x i8], align 1, !stack-protector !2
+ store ptr %msg, ptr %msg.addr, align 8
+ %arraydecay = getelementptr inbounds [1000 x i8], ptr %a, i64 0, i64 0
+ %0 = load ptr, ptr %msg.addr, align 8
+ %call = call ptr @strcpy(ptr noundef %arraydecay, ptr noundef %0) #3
+ %arraydecay1 = getelementptr inbounds [1000 x i8], ptr %a, i64 0, i64 0
+ %call2 = call i32 (ptr, ...) @printf(ptr noundef @.str, ptr noundef %arraydecay1)
+ ret void
+}
+
+
+; CHECK-LABEL: test2:
+; CHECK: ___stack_chk_guard
+
+; Function Attrs: noinline nounwind optnone
+define void @test2(ptr noundef %msg) #0 {
+entry:
+ %msg.addr = alloca ptr, align 8
+ %b = alloca [1000 x i8], align 1
+ store ptr %msg, ptr %msg.addr, align 8
+ %arraydecay = getelementptr inbounds [1000 x i8], ptr %b, i64 0, i64 0
+ %0 = load ptr, ptr %msg.addr, align 8
+ %call = call ptr @strcpy(ptr noundef %arraydecay, ptr noundef %0) #3
+ %arraydecay1 = getelementptr inbounds [1000 x i8], ptr %b, i64 0, i64 0
+ %call2 = call i32 (ptr, ...) @printf(ptr noundef @.str, ptr noundef %arraydecay1)
+ ret void
+}
+
+; Function Attrs: nounwind
+declare ptr @strcpy(ptr noundef, ptr noundef) #1
+
+declare i32 @printf(ptr noundef, ...) #2
+
+attributes #0 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" ssp }
+attributes #1 = { nounwind "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
+attributes #2 = { "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
+attributes #3 = { nounwind }
+
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{!"clang version 22.0.0"}
+!2 = !{i32 0}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ributzka
left a comment
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.
Otherwise LGTM
llvm/lib/CodeGen/StackProtector.cpp
Outdated
| for (const BasicBlock &BB : *F) { | ||
| for (const Instruction &I : BB) { | ||
| if (const AllocaInst *AI = dyn_cast<AllocaInst>(&I)) { | ||
| if (MDNode* MD = AI->getMetadata("stack-protector")) { |
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.
What if stack-protector is 1? Should it force a stack protector, or is that an unexpected value?
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 like the idea too, though this is meant to support __attribute__((no_stack_protector)) on locals and I don't think there's a matching __attribute__((stack_protector)) to piggyback on. Could be useful in some paranoid cases, though.
nikic
left a comment
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.
The metadata needs to be documented in LangRef.
…otection. This is the LLVM piece of this work. The clang work permitted the no_stack_protector attribute on local variables. That generated new metadata which is used here to skip any alloca's which would otherwise require a stack protector.
c81e7db to
eb3cd8e
Compare
🐧 Linux x64 Test Results
Failed Tests(click on a test name to see its output) lldb-apilldb-api.tools/lldb-dap/output/TestDAP_output.pyIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
This is the LLVM piece of this work. There is also a clang piece, which adds this metadata to AllocaInst when the source does
__attribute__((no_stack_protector))on a variable.We already have
__attribute__((no_stack_protector))on functions, but opting out the whole function might be too heavy a hammer. Instead this allows us to opt out of stack protectors on specific allocations we might have audited an know to be safe, but still allow the function to generate a stack protector if other allocations necessitate it.