Disable PGO instrumentation on naked function #75224
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We only allow for assembly code in naked function, and PGO instrumentation (esp. temporal instrumentation that introduces a function call) can wreak havoc in this. Fix llvm#74573
llvmbot
added
clang
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-pgo Author: None (serge-sans-paille) ChangesWe only allow for assembly code in naked function, and PGO instrumentation (esp. temporal instrumentation that introduces a function call) can wreak havoc in this. Fix #74573 Full diff: https://github.com/llvm/llvm-project/pull/75224.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 2199d7b58fb96..6689ddc19cb1c 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -892,6 +892,10 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,
}
}
+ if (FD->hasAttr<NakedAttr>()) {
+ Fn->addFnAttr(llvm::Attribute::NoProfile);
+ }
+
unsigned Count, Offset;
if (const auto *Attr =
D ? D->getAttr<PatchableFunctionEntryAttr>() : nullptr) {
diff --git a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
index 4a5a0b25bebba..57ff1648788f9 100644
--- a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -1783,6 +1783,8 @@ static bool skipPGOUse(const Function &F) {
static bool skipPGOGen(const Function &F) {
if (skipPGOUse(F))
return true;
+ if (F.hasFnAttribute(llvm::Attribute::Naked))
+ return true;
if (F.hasFnAttribute(llvm::Attribute::NoProfile))
return true;
if (F.hasFnAttribute(llvm::Attribute::SkipProfile))
diff --git a/llvm/test/Transforms/PGOProfile/timestamp.ll b/llvm/test/Transforms/PGOProfile/timestamp.ll
index 8d6095a031a66..d5d1233070f7f 100644
--- a/llvm/test/Transforms/PGOProfile/timestamp.ll
+++ b/llvm/test/Transforms/PGOProfile/timestamp.ll
@@ -3,10 +3,21 @@
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
+; CHECK-LABEL: define void @foo(
define void @foo() {
entry:
; CHECK: call void @llvm.instrprof.timestamp({{.*}})
ret void
}
-; CHECK: declare void @llvm.instrprof.timestamp(
+; CHECK-LABEL: define void @bar(
+define void @bar() #0 {
+entry:
+ ; CHECK-NOT: call void @llvm.instrprof.timestamp({{.*}})
+ call void asm sideeffect "retq;", "~{dirflag},~{fpsr},~{flags}"()
+ unreachable
+}
+
+; CHECK-LABEL: declare void @llvm.instrprof.timestamp(
+
+attributes #0 = { naked }
|
|
@llvm/pr-subscribers-clang Author: None (serge-sans-paille) ChangesWe only allow for assembly code in naked function, and PGO instrumentation (esp. temporal instrumentation that introduces a function call) can wreak havoc in this. Fix #74573 Full diff: https://github.com/llvm/llvm-project/pull/75224.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 2199d7b58fb96..6689ddc19cb1c 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -892,6 +892,10 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,
}
}
+ if (FD->hasAttr<NakedAttr>()) {
+ Fn->addFnAttr(llvm::Attribute::NoProfile);
+ }
+
unsigned Count, Offset;
if (const auto *Attr =
D ? D->getAttr<PatchableFunctionEntryAttr>() : nullptr) {
diff --git a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
index 4a5a0b25bebba..57ff1648788f9 100644
--- a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -1783,6 +1783,8 @@ static bool skipPGOUse(const Function &F) {
static bool skipPGOGen(const Function &F) {
if (skipPGOUse(F))
return true;
+ if (F.hasFnAttribute(llvm::Attribute::Naked))
+ return true;
if (F.hasFnAttribute(llvm::Attribute::NoProfile))
return true;
if (F.hasFnAttribute(llvm::Attribute::SkipProfile))
diff --git a/llvm/test/Transforms/PGOProfile/timestamp.ll b/llvm/test/Transforms/PGOProfile/timestamp.ll
index 8d6095a031a66..d5d1233070f7f 100644
--- a/llvm/test/Transforms/PGOProfile/timestamp.ll
+++ b/llvm/test/Transforms/PGOProfile/timestamp.ll
@@ -3,10 +3,21 @@
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
+; CHECK-LABEL: define void @foo(
define void @foo() {
entry:
; CHECK: call void @llvm.instrprof.timestamp({{.*}})
ret void
}
-; CHECK: declare void @llvm.instrprof.timestamp(
+; CHECK-LABEL: define void @bar(
+define void @bar() #0 {
+entry:
+ ; CHECK-NOT: call void @llvm.instrprof.timestamp({{.*}})
+ call void asm sideeffect "retq;", "~{dirflag},~{fpsr},~{flags}"()
+ unreachable
+}
+
+; CHECK-LABEL: declare void @llvm.instrprof.timestamp(
+
+attributes #0 = { naked }
|
| @@ -892,6 +892,10 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy, | |||
| } | |||
| } | |||
|
|
|||
| if (FD->hasAttr<NakedAttr>()) { | |||
There was a problem hiding this comment.
Is this change needed given the separate change in LLVM skipPGOGen?
There was a problem hiding this comment.
This should not be needed. The llvm change is sufficient to work with all frontends including clang.
There was a problem hiding this comment.
not at all, that was a leftover, sorry about that.
MaskRay
approved these changes
Dec 12, 2023
| @@ -1783,6 +1783,8 @@ static bool skipPGOUse(const Function &F) { | |||
| static bool skipPGOGen(const Function &F) { | |||
| if (skipPGOUse(F)) | |||
| return true; | |||
| if (F.hasFnAttribute(llvm::Attribute::Naked)) | |||
There was a problem hiding this comment.
Makes sense. Similar to https://reviews.llvm.org/D149721
| @@ -892,6 +892,10 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy, | |||
| } | |||
| } | |||
|
|
|||
| if (FD->hasAttr<NakedAttr>()) { | |||
There was a problem hiding this comment.
This should not be needed. The llvm change is sufficient to work with all frontends including clang.
teresajohnson
approved these changes
Dec 12, 2023
|
Thanks @MaskRay & @teresajohnson for the quick review o/ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We only allow for assembly code in naked function, and PGO instrumentation (esp. temporal instrumentation that introduces a function call) can wreak havoc in this.
Fix #74573