[LLVM][ConstantFold] Undefined values are not constant #130713
Conversation
llvm.is.constant (and therefore Clang's __builtin_constant_p()) need to report undefined values as non-constant or future DCE choices end up making no sense. This was encountered while building the Linux kernel which uses __builtin_constant_p() while trying to evaluate if it is safe to use a compile-time constant resolution for string lengths or if it must kick over to a full runtime call to strlen(). Obviously an undefined variable cannot be known at compile-time, so __builtin_constant_p() needs to return false. This change will also mean that Clang will match GCC's behavior under the same conditions. Fixes llvm#130649
|
@llvm/pr-subscribers-llvm-transforms Author: Kees Cook (kees) Changesllvm.is.constant (and therefore Clang's __builtin_constant_p()) need to report undefined values as non-constant or future DCE choices end up making no sense. This was encountered while building the Linux kernel which uses __builtin_constant_p() while trying to evaluate if it is safe to use a compile-time constant resolution for string lengths or if it must kick over to a full runtime call to strlen(). Obviously an undefined variable cannot be known at compile-time, so __builtin_constant_p() needs to return false. This change will also mean that Clang will match GCC's behavior under the same conditions. Fixes #130649 Full diff: https://github.com/llvm/llvm-project/pull/130713.diff 2 Files Affected:
diff --git a/llvm/lib/IR/Constants.cpp b/llvm/lib/IR/Constants.cpp
index 9e3e739fae3dc..36f4136457617 100644
--- a/llvm/lib/IR/Constants.cpp
+++ b/llvm/lib/IR/Constants.cpp
@@ -841,6 +841,8 @@ Constant *Constant::mergeUndefsWith(Constant *C, Constant *Other) {
}
bool Constant::isManifestConstant() const {
+ if (isa<UndefValue>(this))
+ return false;
if (isa<ConstantData>(this))
return true;
if (isa<ConstantAggregate>(this) || isa<ConstantExpr>(this)) {
diff --git a/llvm/test/Transforms/LowerConstantIntrinsics/constant-intrinsics.ll b/llvm/test/Transforms/LowerConstantIntrinsics/constant-intrinsics.ll
index 32bd2fff2732e..2801c3d2a9770 100644
--- a/llvm/test/Transforms/LowerConstantIntrinsics/constant-intrinsics.ll
+++ b/llvm/test/Transforms/LowerConstantIntrinsics/constant-intrinsics.ll
@@ -120,3 +120,21 @@ define i1 @global_array() {
%1 = call i1 @llvm.is.constant.i64(i64 ptrtoint (ptr @real_mode_blob_end to i64))
ret i1 %1
}
+
+;; Ensure that is.constant of undef gets lowered reasonably to "false" in
+;; optimized codegen: specifically that the "true" branch is eliminated.
+;; CHECK-NOT: tail call i32 @subfun_1()
+;; CHECK: tail call i32 @subfun_2()
+;; CHECK-NOT: tail call i32 @subfun_1()
+define i32 @test_undef_branch() nounwind {
+ %v = call i1 @llvm.is.constant.i32(i32 undef)
+ br i1 %v, label %True, label %False
+
+True:
+ %call1 = tail call i32 @subfun_1()
+ ret i32 %call1
+
+False:
+ %call2 = tail call i32 @subfun_2()
+ ret i32 %call2
+}
|
|
@llvm/pr-subscribers-llvm-ir Author: Kees Cook (kees) Changesllvm.is.constant (and therefore Clang's __builtin_constant_p()) need to report undefined values as non-constant or future DCE choices end up making no sense. This was encountered while building the Linux kernel which uses __builtin_constant_p() while trying to evaluate if it is safe to use a compile-time constant resolution for string lengths or if it must kick over to a full runtime call to strlen(). Obviously an undefined variable cannot be known at compile-time, so __builtin_constant_p() needs to return false. This change will also mean that Clang will match GCC's behavior under the same conditions. Fixes #130649 Full diff: https://github.com/llvm/llvm-project/pull/130713.diff 2 Files Affected:
diff --git a/llvm/lib/IR/Constants.cpp b/llvm/lib/IR/Constants.cpp
index 9e3e739fae3dc..36f4136457617 100644
--- a/llvm/lib/IR/Constants.cpp
+++ b/llvm/lib/IR/Constants.cpp
@@ -841,6 +841,8 @@ Constant *Constant::mergeUndefsWith(Constant *C, Constant *Other) {
}
bool Constant::isManifestConstant() const {
+ if (isa<UndefValue>(this))
+ return false;
if (isa<ConstantData>(this))
return true;
if (isa<ConstantAggregate>(this) || isa<ConstantExpr>(this)) {
diff --git a/llvm/test/Transforms/LowerConstantIntrinsics/constant-intrinsics.ll b/llvm/test/Transforms/LowerConstantIntrinsics/constant-intrinsics.ll
index 32bd2fff2732e..2801c3d2a9770 100644
--- a/llvm/test/Transforms/LowerConstantIntrinsics/constant-intrinsics.ll
+++ b/llvm/test/Transforms/LowerConstantIntrinsics/constant-intrinsics.ll
@@ -120,3 +120,21 @@ define i1 @global_array() {
%1 = call i1 @llvm.is.constant.i64(i64 ptrtoint (ptr @real_mode_blob_end to i64))
ret i1 %1
}
+
+;; Ensure that is.constant of undef gets lowered reasonably to "false" in
+;; optimized codegen: specifically that the "true" branch is eliminated.
+;; CHECK-NOT: tail call i32 @subfun_1()
+;; CHECK: tail call i32 @subfun_2()
+;; CHECK-NOT: tail call i32 @subfun_1()
+define i32 @test_undef_branch() nounwind {
+ %v = call i1 @llvm.is.constant.i32(i32 undef)
+ br i1 %v, label %True, label %False
+
+True:
+ %call1 = tail call i32 @subfun_1()
+ ret i32 %call1
+
+False:
+ %call2 = tail call i32 @subfun_2()
+ ret i32 %call2
+}
|
You can test this locally with the following command:git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef[^a-zA-Z0-9_-]|UndefValue::get)' a5a33d82931def5a2f6f4dcfca834574190b4b50 3dea51b6a139cab729fbcb1027e13c13fd80b65e llvm/lib/IR/Constants.cpp llvm/test/Transforms/LowerConstantIntrinsics/constant-intrinsics.llThe following files introduce new uses of undef:
Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields In tests, avoid using For example, this is considered a bad practice: define void @fn() {
...
br i1 undef, ...
}Please use the following instead: define void @fn(i1 %cond) {
...
br i1 %cond, ...
}Please refer to the Undefined Behavior Manual for more information. |
AIUI, this test really does want to use |
kees
requested review from
nickdesaulniers,
nathanchance and
efriedma-quic
and removed request for
nikic
IMO |
Agreed. To explain to anyone else who isn't following... suppose you have Of course, that's true with or without this patch, so I guess you could argue that this patch is an improvement anyway. But really, we need to fix Linux code so it doesn't depend on this behavior, or we'll trip over it again later. |
|
(ah, my reviewers addition collided with the bot and @nikic got removed -- I've fix that now.) Okay, it sounds like there isn't really an issue for this commit, just generally that |
It seems that Clang thinks __builtin_constant_p() of undefined variables should return true[1]. This is being fixed separately[2], but in the meantime, expand the fortify tests to help track this kind of thing down faster in the future. Link: ClangBuiltLinux#2073 [1] Link: llvm/llvm-project#130713 [2] Signed-off-by: Kees Cook <[email protected]>
| ;; optimized codegen: specifically that the "true" branch is eliminated. | ||
| ;; CHECK-NOT: tail call i32 @subfun_1() | ||
| ;; CHECK: tail call i32 @subfun_2() | ||
| ;; CHECK-NOT: tail call i32 @subfun_1() |
There was a problem hiding this comment.
| ;; CHECK-NOT: tail call i32 @subfun_1() |
There was a problem hiding this comment.
Based on my understanding of this CHECK pattern at the top of the file (where I copied this test style from) the second CHECK-NOT is to make sure there is no re-ordering happening: i.e. it must be fully missing, not just present later.
llvm.is.constant (and therefore Clang's __builtin_constant_p()) need to report undefined values as non-constant or future DCE choices end up making no sense. This was encountered while building the Linux kernel which uses __builtin_constant_p() while trying to evaluate if it is safe to use a compile-time constant resolution for string lengths or if it must kick over to a full runtime call to strlen(). Obviously an undefined variable cannot be known at compile-time, so __builtin_constant_p() needs to return false. This change will also mean that Clang will match GCC's behavior under the same conditions. Fixes llvm#130649
llvm.is.constant (and therefore Clang's __builtin_constant_p()) need to report undefined values as non-constant or future DCE choices end up making no sense. This was encountered while building the Linux kernel which uses __builtin_constant_p() while trying to evaluate if it is safe to use a compile-time constant resolution for string lengths or if it must kick over to a full runtime call to strlen(). Obviously an undefined variable cannot be known at compile-time, so __builtin_constant_p() needs to return false. This change will also mean that Clang will match GCC's behavior under the same conditions.
Fixes #130649