[GlobalOpt] Bail out on non-ConstExprs in isSimpleEnoughtToCommit.

[fhahn]

Bail out for non ConstantExpr constants in isSimpleEnoughValueToCommitHelper to prevent crash for non-ConstantExpr constants

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

Bail out for ConstantPtrAuth constants in isSimpleEnoughValueToCommitHelper to prevent crash in cast<ConstantExpr> below.

---

Full diff: https://github.com/llvm/llvm-project/pull/143400.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Utils/Evaluator.cpp (+3)
• (added) llvm/test/Transforms/GlobalOpt/global-constructor-ptrauth-constant.ll (+27)

diff --git a/llvm/lib/Transforms/Utils/Evaluator.cpp b/llvm/lib/Transforms/Utils/Evaluator.cpp
index 2af447aadce22..ecdcdf55d5089 100644
--- a/llvm/lib/Transforms/Utils/Evaluator.cpp
+++ b/llvm/lib/Transforms/Utils/Evaluator.cpp
@@ -74,6 +74,9 @@ isSimpleEnoughValueToCommitHelper(Constant *C,
     return true;
   }
 
+  if (isa<ConstantPtrAuth>(C))
+    return false;
+
   // We don't know exactly what relocations are allowed in constant expressions,
   // so we allow &global+constantoffset, which is safe and uniformly supported
   // across targets.
diff --git a/llvm/test/Transforms/GlobalOpt/global-constructor-ptrauth-constant.ll b/llvm/test/Transforms/GlobalOpt/global-constructor-ptrauth-constant.ll
new file mode 100644
index 0000000000000..ef64b3eb1147c
--- /dev/null
+++ b/llvm/test/Transforms/GlobalOpt/global-constructor-ptrauth-constant.ll
@@ -0,0 +1,27 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals all --version 5
+; RUN: opt -p globalopt -S %s | FileCheck %s
+
+@llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @ctor, ptr null }]
+
+@foo = internal global ptr null
+
+declare void @user(ptr)
+
+;.
+; CHECK: @llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @ctor, ptr null }]
+; CHECK: @foo = internal global ptr null
+;.
+define void @ctor() {
+; CHECK-LABEL: define void @ctor() {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[DST:%.*]] = alloca ptr, align 8
+; CHECK-NEXT:    store ptr ptrauth (ptr @foo, i32 0), ptr [[DST]], align 8
+; CHECK-NEXT:    call void @user(ptr [[DST]])
+; CHECK-NEXT:    ret void
+;
+entry:
+  %dst = alloca ptr, align 8
+  store ptr ptrauth (ptr @foo, i32 0), ptr %dst, align 8
+  call void @user(ptr %dst)
+  ret void
+}

[nikic]

Should probably bail out on all non-ConstantExpr (use dyn_cast below)? There are a bunch of other special constants like dso_local_equivalent and no_cfi.

[ojhunt]

Obviously the early return works, but dyn_cast certainly seems like the robust solution - there doesn't seem to be any obvious indication that C is required to be a specific set of subclasses of Constant.

I don't know the impact of return false vs true - I don't know backend concepts so I don't know what it means to commit a helper here?

[fhahn]

Updated to use dyn_cast, thanks.

This checks if the constant can be lowered to something constant (or almost constant) in the backend. I'm not 100% sure how exactly the result is used, but presumably to decide whether it is worth propagating the constant to multiple places.

I think in general, ConstanPtrAuth will lowered to a set of instructions to materlize the address, followed by a ptrauth instruction, e.g. for the one in the example:

        adrp	x16, _foo@PAGE
	add	x16, x16, _foo@PAGEOFF
	paciza	x16

[nikic]

The check exists to determine whether the result is a relocatable constant that can be used as a global variable initializer.

[ojhunt]

@fhahn what is the [Obj-]C[++] code that led to this error? if it's a global constant we may want to see if it can be turned into a loader fixup (not something that needs to be fixed in this patch of course).

This LGTM, but would like to be sure @nikic agrees before it's merged.

[fhahn]

Added tests with the different constants

[ojhunt]

Approved but I'd like @nikic to approve as well.

I wonder if you can also construct tests for the cases @nikic mentioned (dso_local_equivalent, etc)

[ojhunt]

@fhahn did you verify the additional tests also fail without the fix (Oliver's test case paranoia :D)

[fhahn]

@fhahn did you verify the additional tests also fail without the fix (Oliver's test case paranoia :D)

Yep

[ojhunt]

@fhahn did you verify the additional tests also fail without the fix (Oliver's test case paranoia :D)

Yep

even more approved than before :D