[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, |
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
c9cf318 to
a79f5dc
Compare
There was a problem hiding this comment.
Have you considered moving this comparison to the switch statement at the end of the function? At first glance, this would be more in line with other cases and would make the code a bit simpler by making use of the fact that both L and R must be castable to ConstantPtrAuth at that point.
Considering special handling of GlobalValue and ConstantDataSequential outside of the switch statement in the existing code - maybe this could be related to less-trivial implementation of static bool classof(const Value *V) in these classes.
There was a problem hiding this comment.
Have you considered auto-generating the CHECK lines with llvm/utils/update_test_checks.py?
[nit] It may be better to pass the input file by redirecting it to stdin:
; RUN: opt -passes=mergefunc -S < %s | FileCheck %s
There was a problem hiding this comment.
[nit] It may be worth adding a comment that different callees are required to prevent unintended merging of unrelated functions like @g_ptr_baz and @f_key0 (if I got it right). Or maybe just add an integer parameter that can be different in unrelated pairs of test functions. Or, assuming the functions cannot be partially merged, just return different integer values from the test function. I guess the latter approach would be the most error-proof, as it should be much easier to spot if the two functions consisting a single negative test case accidentally differ:
What would misspelled callee name look like now:
define void @f_addr_null() {
entry:
call void @callee1(ptr ptrauth (ptr @baz, i32 0))
ret void
}
define void @g_addr_ADDR() {
entry:
call void @callee2(ptr ptrauth (ptr @baz, i32 0, i64 0, ptr @ADDR))
ret void
}Misspelled integer argument:
define void @f_addr_null() {
entry:
call void @callee(i32 1, ptr ptrauth (ptr @baz, i32 0))
ret void
}
define void @g_addr_ADDR() {
entry:
call void @callee(i32 2, ptr ptrauth (ptr @baz, i32 0, i64 0, ptr @ADDR))
ret void
}Misspelled return value:
define i32 @f_addr_null() {
entry:
call void @callee(ptr ptrauth (ptr @baz, i32 0))
ret i32 1
}
define i32 @g_addr_ADDR() {
entry:
call void @callee(ptr ptrauth (ptr @baz, i32 0, i64 0, ptr @ADDR))
ret i32 2
}(Feel free to ignore)
6 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: