[PtrAuth] Add ConstantPtrAuth comparator to FunctionComparator.cpp #159480
Conversation
|
@llvm/pr-subscribers-llvm-transforms Author: Oskar Wirga (oskarwirga) ChangesWhen building rustc std for arm64e, core fails to compile successfully with the error: This is a result of function merging so I modified FunctionComparator.cpp as the ConstantPtrAuth value would go unchecked in the switch statement. The test case is a reduction from the failure in core and fails on main with: Full diff: https://github.com/llvm/llvm-project/pull/159480.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Utils/FunctionComparator.cpp b/llvm/lib/Transforms/Utils/FunctionComparator.cpp
index 6d4026e8209de..4148ac9d4442d 100644
--- a/llvm/lib/Transforms/Utils/FunctionComparator.cpp
+++ b/llvm/lib/Transforms/Utils/FunctionComparator.cpp
@@ -353,6 +353,19 @@ int FunctionComparator::cmpConstants(const Constant *L,
return 1;
if (!L->isNullValue() && R->isNullValue())
return -1;
+
+ // Handle authenticated pointer constants produced by ConstantPtrAuth::get.
+ if (auto *PA1 = dyn_cast<ConstantPtrAuth>(L)) {
+ auto *PA2 = dyn_cast<ConstantPtrAuth>(R);
+ if (!PA2)
+ return cmpNumbers(L->getValueID(), R->getValueID());
+
+ if (int Res = cmpConstants(PA1->getPointer(), PA2->getPointer()))
+ return Res;
+ if (int Res = cmpConstants(PA1->getKey(), PA2->getKey()))
+ return Res;
+ return cmpConstants(PA1->getDiscriminator(), PA2->getDiscriminator());
+ }
auto GlobalValueL = const_cast<GlobalValue *>(dyn_cast<GlobalValue>(L));
auto GlobalValueR = const_cast<GlobalValue *>(dyn_cast<GlobalValue>(R));
diff --git a/llvm/test/Transforms/MergeFunc/ptrauth-const-compare.ll b/llvm/test/Transforms/MergeFunc/ptrauth-const-compare.ll
new file mode 100644
index 0000000000000..75ebb834c6219
--- /dev/null
+++ b/llvm/test/Transforms/MergeFunc/ptrauth-const-compare.ll
@@ -0,0 +1,67 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --include-generated-funcs --version 2
+;; Check the case for ConstantPtrAuth to be compared properly in FunctionComparator.cpp
+; RUN: opt -S -passes=mergefunc < %s | FileCheck %s
+target triple = "arm64e-apple-ios14.0.0"
+
+define { ptr, i64 } @"foo"() {
+start:
+ %func = alloca [8 x i8], align 8
+ br i1 false, label %bb5, label %bb9
+
+bb9: ; preds = %bb2, %start
+ %self = load i8, ptr null, align 1
+ br i1 false, label %bb2, label %bb5
+
+bb5: ; preds = %bb9, %start
+ ret { ptr, i64 } zeroinitializer
+
+bb2: ; preds = %bb9
+ call void ptrauth (ptr null, i32 0)(ptr null, i8 0)
+ br label %bb9
+}
+
+define { ptr, i64 } @"bar"() {
+start:
+ %func = alloca [8 x i8], align 8
+ br i1 false, label %bb5, label %bb9
+
+bb9: ; preds = %bb2, %start
+ %self = load i8, ptr null, align 1
+ br i1 false, label %bb2, label %bb5
+
+bb5: ; preds = %bb9, %start
+ ret { ptr, i64 } zeroinitializer
+
+bb2: ; preds = %bb9
+ call void ptrauth (ptr @"baz", i32 0)(ptr null, i8 0)
+ br label %bb9
+}
+
+declare void @"baz"()
+; CHECK-LABEL: define { ptr, i64 } @foo() {
+; CHECK-NEXT: start:
+; CHECK-NEXT: [[FUNC:%.*]] = alloca [8 x i8], align 8
+; CHECK-NEXT: br i1 false, label [[BB5:%.*]], label [[BB9:%.*]]
+; CHECK: bb9:
+; CHECK-NEXT: [[SELF:%.*]] = load i8, ptr null, align 1
+; CHECK-NEXT: br i1 false, label [[BB2:%.*]], label [[BB5]]
+; CHECK: bb5:
+; CHECK-NEXT: ret { ptr, i64 } zeroinitializer
+; CHECK: bb2:
+; CHECK-NEXT: call void ptrauth (ptr null, i32 0)(ptr null, i8 0)
+; CHECK-NEXT: br label [[BB9]]
+;
+;
+; CHECK-LABEL: define { ptr, i64 } @bar() {
+; CHECK-NEXT: start:
+; CHECK-NEXT: [[FUNC:%.*]] = alloca [8 x i8], align 8
+; CHECK-NEXT: br i1 false, label [[BB5:%.*]], label [[BB9:%.*]]
+; CHECK: bb9:
+; CHECK-NEXT: [[SELF:%.*]] = load i8, ptr null, align 1
+; CHECK-NEXT: br i1 false, label [[BB2:%.*]], label [[BB5]]
+; CHECK: bb5:
+; CHECK-NEXT: ret { ptr, i64 } zeroinitializer
+; CHECK: bb2:
+; CHECK-NEXT: call void ptrauth (ptr @baz, i32 0)(ptr null, i8 0)
+; CHECK-NEXT: br label [[BB9]]
+;
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
oskarwirga
force-pushed
the
arm64e-rust-fixes
branch
from
66cb663 to
095255e
Compare
oskarwirga
force-pushed
the
arm64e-rust-fixes
branch
from
095255e to
10f5e77
Compare
oskarwirga
force-pushed
the
arm64e-rust-fixes
branch
from
10f5e77 to
c9cf318
Compare
|
@ahmedbougacha I think you're better looking at this one |
|
In a call instruction like this call void ptrauth (ptr @baz, i32 0)(ptr null)a signed pointer to This is probably acceptable in this kind of tests (except for that I'm not sure if calling signed null pointer in call void @callee(ptr ptrauth (ptr @baz, i32 0))This way, |
oskarwirga
force-pushed
the
arm64e-rust-fixes
branch
from
c9cf318 to
a79f5dc
Compare
atrosinenko
left a comment
atrosinenko
left a comment
There was a problem hiding this comment.
Thank you for all the updates!
The PR description seems to be out of sync with the latest patch, as ptrauth-const-compare.ll has many synthetic test cases now.
|
@oskarwirga Could you please clarify if you want to make some enhancements to your PR prior to merging or if you are happy with merging this "as is"? I think it would be very nice to get this merged relatively soon in order to have the fix included in the upcoming llvm release. @atrosinenko As far as I can see, your unresolved comments are "feel free to ignore". Is my understanding correct or is there something you find crucial to fix before merging? |
|
I've hit the same unreachable when working with rust and pauth calls. While it's not a blocker, as rust targets can opt out of merge functions ( |
oskarwirga
force-pushed
the
arm64e-rust-fixes
branch
from
a79f5dc to
72d8ad7
Compare
|
Sorry about the delay everyone! @atrosinenko thank you for your reviews, I've addressed those comments and feel good about where this diff is now. @kovdan01 Now, I'm ready to merge this :)
I made progress on this earlier this year but very heavy draft (see this discussion for more details: rust-lang/rust#73628 (comment)). If you have something to share, I'd be happy to test/take a look/admire. |
oskarwirga
force-pushed
the
arm64e-rust-fixes
branch
from
72d8ad7 to
6c862f2
Compare
atrosinenko
left a comment
atrosinenko
left a comment
There was a problem hiding this comment.
@oskarwirga Thank you for the update! All my comments are addressed now, except for getting rid of numbered @callees (this basically means my original suggestion of returning obscure pairwise-equal numbers turned out not to be as reliably as it should be).
When building rustc std for arm64e, the optimizations result in devirtualization of indirect calls. This can happen during function merging so I modified FunctionComparator.cpp as the ConstantPtrAuth value would go unchecked in the switch statement.
oskarwirga
force-pushed
the
arm64e-rust-fixes
branch
from
6c862f2 to
a298b74
Compare
7 participants
When building rustc std for arm64e, core fails to compile successfully with the error:
This is a result of function merging so I modified FunctionComparator.cpp as the ConstantPtrAuth value would go unchecked in the switch statement.
The test case is a reduction from the failure in core and fails on main with: