[clang] [clang][CodeGen][UBSan] Fixing shift-exponent generation for _BitInt (PR #80515)
Adam Magier via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 5 15:33:13 PST 2024
https://github.com/AdamMagierFOSS updated https://github.com/llvm/llvm-project/pull/80515
>From 4e1c37ae83dec050fc9b7aa172db01fa0b2b6d68 Mon Sep 17 00:00:00 2001
From: Adam Magier <adam.magier at ericsson.com>
Date: Sat, 3 Feb 2024 00:38:54 +0100
Subject: [PATCH 1/3] [clang][CodeGen][UBSan] Fixing shift-exponent generation
for _BitInt
Testing the shift-exponent check with small width _BitInt values exposed
a bug in ScalarExprEmitter::GetWidthMinusOneValue when using the result
to determine valid exponent sizes. False positives were reported for
some left shifts when width(LHS)-1 > range(RHS) and false negatives were
reported for right shifts when value(RHS) > range(LHS). This patch caps
the maximum value of GetWidthMinusOneValue to fit within range(RHS) to
fix the issue with left shifts and fixes a code generation in EmitShr to
fix the issue with right shifts.
---
clang/lib/CodeGen/CGExprScalar.cpp | 9 ++++++-
clang/test/CodeGen/ubsan-shift-bitint.c | 36 +++++++++++++++++++++++++
2 files changed, 44 insertions(+), 1 deletion(-)
create mode 100644 clang/test/CodeGen/ubsan-shift-bitint.c
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 5502f685f64743..e2e3ed839714a2 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -4121,6 +4121,13 @@ Value *ScalarExprEmitter::GetWidthMinusOneValue(Value* LHS,Value* RHS) {
Ty = cast<llvm::IntegerType>(VT->getElementType());
else
Ty = cast<llvm::IntegerType>(LHS->getType());
+ // Testing with small _BitInt types has shown that Ty->getBitwidth() - 1
+ // can sometimes overflow the capacity of RHS->getType(), cap the value
+ // to be the largest RHS->getType() can hold
+ llvm::APInt RHSMax =
+ llvm::APInt::getMaxValue(RHS->getType()->getScalarSizeInBits());
+ if (RHSMax.ult(Ty->getBitWidth()))
+ return llvm::ConstantInt::get(RHS->getType(), RHSMax);
return llvm::ConstantInt::get(RHS->getType(), Ty->getBitWidth() - 1);
}
@@ -4235,7 +4242,7 @@ Value *ScalarExprEmitter::EmitShr(const BinOpInfo &Ops) {
isa<llvm::IntegerType>(Ops.LHS->getType())) {
CodeGenFunction::SanitizerScope SanScope(&CGF);
llvm::Value *Valid =
- Builder.CreateICmpULE(RHS, GetWidthMinusOneValue(Ops.LHS, RHS));
+ Builder.CreateICmpULE(Ops.RHS, GetWidthMinusOneValue(Ops.LHS, Ops.RHS));
EmitBinOpCheck(std::make_pair(Valid, SanitizerKind::ShiftExponent), Ops);
}
diff --git a/clang/test/CodeGen/ubsan-shift-bitint.c b/clang/test/CodeGen/ubsan-shift-bitint.c
new file mode 100644
index 00000000000000..8ca94b7de5a42d
--- /dev/null
+++ b/clang/test/CodeGen/ubsan-shift-bitint.c
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 %s -O0 -fsanitize=shift-exponent -emit-llvm -o - | FileCheck %s
+
+// Checking that the code generation is using the unextended/untruncated
+// exponent values and capping the values accordingly
+
+// CHECK-LABEL: define{{.*}} i32 @test_left_variable
+int test_left_variable(unsigned _BitInt(5) b, unsigned _BitInt(2) e) {
+ // CHECK: [[E_REG:%.+]] = load [[E_SIZE:i2]]
+ // CHECK: icmp ule [[E_SIZE]] [[E_REG]], -1
+ return b << e;
+}
+
+// CHECK-LABEL: define{{.*}} i32 @test_right_variable
+int test_right_variable(unsigned _BitInt(2) b, unsigned _BitInt(3) e) {
+ // CHECK: [[E_REG:%.+]] = load [[E_SIZE:i3]]
+ // CHECK: icmp ule [[E_SIZE]] [[E_REG]], 1
+ return b >> e;
+}
+
+// Old code generation would give false positives on left shifts when:
+// value(e) > (width(b) - 1 % 2 ** width(e))
+// CHECK-LABEL: define{{.*}} i32 @test_left_literal
+int test_left_literal(unsigned _BitInt(5) b) {
+ // CHECK-NOT: br i1 false, label %cont, label %handler.shift_out_of_bounds
+ // CHECK: br i1 true, label %cont, label %handler.shift_out_of_bounds
+ return b << 3uwb;
+}
+
+// Old code generation would give false positives on right shifts when:
+// (value(e) % 2 ** width(b)) < width(b)
+// CHECK-LABEL: define{{.*}} i32 @test_right_literal
+int test_right_literal(unsigned _BitInt(2) b) {
+ // CHECK-NOT: br i1 true, label %cont, label %handler.shift_out_of_bounds
+ // CHECK: br i1 false, label %cont, label %handler.shift_out_of_bounds
+ return b >> 4uwb;
+}
>From c3be3ebd7a8ae57d319eedbb97ab85324c814db2 Mon Sep 17 00:00:00 2001
From: Adam Magier <adam.magier at ericsson.com>
Date: Tue, 6 Feb 2024 00:11:19 +0100
Subject: [PATCH 2/3] Updating with feedback
---
clang/lib/CodeGen/CGExprScalar.cpp | 27 +++++++++++++------------
clang/test/CodeGen/ubsan-shift-bitint.c | 4 ++--
2 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index e2e3ed839714a2..f08fb8d4e3b349 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -774,7 +774,7 @@ class ScalarExprEmitter
void EmitUndefinedBehaviorIntegerDivAndRemCheck(const BinOpInfo &Ops,
llvm::Value *Zero,bool isDiv);
// Common helper for getting how wide LHS of shift is.
- static Value *GetWidthMinusOneValue(Value* LHS,Value* RHS);
+ static Value *GetMaximumShiftAmount(Value* LHS,Value* RHS);
// Used for shifting constraints for OpenCL, do mask for powers of 2, URem for
// non powers of two.
@@ -4115,20 +4115,21 @@ Value *ScalarExprEmitter::EmitSub(const BinOpInfo &op) {
return Builder.CreateExactSDiv(diffInChars, divisor, "sub.ptr.div");
}
-Value *ScalarExprEmitter::GetWidthMinusOneValue(Value* LHS,Value* RHS) {
+Value *ScalarExprEmitter::GetMaximumShiftAmount(Value* LHS,Value* RHS) {
llvm::IntegerType *Ty;
if (llvm::VectorType *VT = dyn_cast<llvm::VectorType>(LHS->getType()))
Ty = cast<llvm::IntegerType>(VT->getElementType());
else
Ty = cast<llvm::IntegerType>(LHS->getType());
- // Testing with small _BitInt types has shown that Ty->getBitwidth() - 1
- // can sometimes overflow the capacity of RHS->getType(), cap the value
- // to be the largest RHS->getType() can hold
- llvm::APInt RHSMax =
- llvm::APInt::getMaxValue(RHS->getType()->getScalarSizeInBits());
+ // For a given type of LHS the maximum shift amount is width(LHS)-1, however
+ // it can occur that width(LHS)-1 > range(RHS). Since there is no check for
+ // this in ConstantInt::get, this results in the value getting truncated.
+ // Constrain the return value to be max(RHS) in this case.
+ llvm::Type *RHSTy = RHS->getType();
+ llvm::APInt RHSMax = llvm::APInt::getMaxValue(RHSTy->getScalarSizeInBits());
if (RHSMax.ult(Ty->getBitWidth()))
- return llvm::ConstantInt::get(RHS->getType(), RHSMax);
- return llvm::ConstantInt::get(RHS->getType(), Ty->getBitWidth() - 1);
+ return llvm::ConstantInt::get(RHSTy, RHSMax);
+ return llvm::ConstantInt::get(RHSTy, Ty->getBitWidth() - 1);
}
Value *ScalarExprEmitter::ConstrainShiftValue(Value *LHS, Value *RHS,
@@ -4140,7 +4141,7 @@ Value *ScalarExprEmitter::ConstrainShiftValue(Value *LHS, Value *RHS,
Ty = cast<llvm::IntegerType>(LHS->getType());
if (llvm::isPowerOf2_64(Ty->getBitWidth()))
- return Builder.CreateAnd(RHS, GetWidthMinusOneValue(LHS, RHS), Name);
+ return Builder.CreateAnd(RHS, GetMaximumShiftAmount(LHS, RHS), Name);
return Builder.CreateURem(
RHS, llvm::ConstantInt::get(RHS->getType(), Ty->getBitWidth()), Name);
@@ -4173,7 +4174,7 @@ Value *ScalarExprEmitter::EmitShl(const BinOpInfo &Ops) {
isa<llvm::IntegerType>(Ops.LHS->getType())) {
CodeGenFunction::SanitizerScope SanScope(&CGF);
SmallVector<std::pair<Value *, SanitizerMask>, 2> Checks;
- llvm::Value *WidthMinusOne = GetWidthMinusOneValue(Ops.LHS, Ops.RHS);
+ llvm::Value *WidthMinusOne = GetMaximumShiftAmount(Ops.LHS, Ops.RHS);
llvm::Value *ValidExponent = Builder.CreateICmpULE(Ops.RHS, WidthMinusOne);
if (SanitizeExponent) {
@@ -4191,7 +4192,7 @@ Value *ScalarExprEmitter::EmitShl(const BinOpInfo &Ops) {
Builder.CreateCondBr(ValidExponent, CheckShiftBase, Cont);
llvm::Value *PromotedWidthMinusOne =
(RHS == Ops.RHS) ? WidthMinusOne
- : GetWidthMinusOneValue(Ops.LHS, RHS);
+ : GetMaximumShiftAmount(Ops.LHS, RHS);
CGF.EmitBlock(CheckShiftBase);
llvm::Value *BitsShiftedOff = Builder.CreateLShr(
Ops.LHS, Builder.CreateSub(PromotedWidthMinusOne, RHS, "shl.zeros",
@@ -4242,7 +4243,7 @@ Value *ScalarExprEmitter::EmitShr(const BinOpInfo &Ops) {
isa<llvm::IntegerType>(Ops.LHS->getType())) {
CodeGenFunction::SanitizerScope SanScope(&CGF);
llvm::Value *Valid =
- Builder.CreateICmpULE(Ops.RHS, GetWidthMinusOneValue(Ops.LHS, Ops.RHS));
+ Builder.CreateICmpULE(Ops.RHS, GetMaximumShiftAmount(Ops.LHS, Ops.RHS));
EmitBinOpCheck(std::make_pair(Valid, SanitizerKind::ShiftExponent), Ops);
}
diff --git a/clang/test/CodeGen/ubsan-shift-bitint.c b/clang/test/CodeGen/ubsan-shift-bitint.c
index 8ca94b7de5a42d..8276a80c335606 100644
--- a/clang/test/CodeGen/ubsan-shift-bitint.c
+++ b/clang/test/CodeGen/ubsan-shift-bitint.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -O0 -fsanitize=shift-exponent -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -O0 -fsanitize=shift-exponent -emit-llvm -std=c2x -triple=x86_64-unknown-linux -o - | FileCheck %s
// Checking that the code generation is using the unextended/untruncated
// exponent values and capping the values accordingly
@@ -12,7 +12,7 @@ int test_left_variable(unsigned _BitInt(5) b, unsigned _BitInt(2) e) {
// CHECK-LABEL: define{{.*}} i32 @test_right_variable
int test_right_variable(unsigned _BitInt(2) b, unsigned _BitInt(3) e) {
- // CHECK: [[E_REG:%.+]] = load [[E_SIZE:i3]]
+ // CHECK: [[E_REG:%.+]] = load [[E_SIZE:i2]]
// CHECK: icmp ule [[E_SIZE]] [[E_REG]], 1
return b >> e;
}
>From 3721afa6f3f91a311c66dc96708b33a00146f7ab Mon Sep 17 00:00:00 2001
From: Adam Magier <adam.magier at ericsson.com>
Date: Tue, 6 Feb 2024 00:33:01 +0100
Subject: [PATCH 3/3] Fix formatting
---
clang/lib/CodeGen/CGExprScalar.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index f08fb8d4e3b349..df8f71cf1d9008 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -774,7 +774,7 @@ class ScalarExprEmitter
void EmitUndefinedBehaviorIntegerDivAndRemCheck(const BinOpInfo &Ops,
llvm::Value *Zero,bool isDiv);
// Common helper for getting how wide LHS of shift is.
- static Value *GetMaximumShiftAmount(Value* LHS,Value* RHS);
+ static Value *GetMaximumShiftAmount(Value *LHS, Value *RHS);
// Used for shifting constraints for OpenCL, do mask for powers of 2, URem for
// non powers of two.
@@ -4115,7 +4115,7 @@ Value *ScalarExprEmitter::EmitSub(const BinOpInfo &op) {
return Builder.CreateExactSDiv(diffInChars, divisor, "sub.ptr.div");
}
-Value *ScalarExprEmitter::GetMaximumShiftAmount(Value* LHS,Value* RHS) {
+Value *ScalarExprEmitter::GetMaximumShiftAmount(Value *LHS, Value *RHS) {
llvm::IntegerType *Ty;
if (llvm::VectorType *VT = dyn_cast<llvm::VectorType>(LHS->getType()))
Ty = cast<llvm::IntegerType>(VT->getElementType());
@@ -4141,7 +4141,7 @@ Value *ScalarExprEmitter::ConstrainShiftValue(Value *LHS, Value *RHS,
Ty = cast<llvm::IntegerType>(LHS->getType());
if (llvm::isPowerOf2_64(Ty->getBitWidth()))
- return Builder.CreateAnd(RHS, GetMaximumShiftAmount(LHS, RHS), Name);
+ return Builder.CreateAnd(RHS, GetMaximumShiftAmount(LHS, RHS), Name);
return Builder.CreateURem(
RHS, llvm::ConstantInt::get(RHS->getType(), Ty->getBitWidth()), Name);
More information about the cfe-commits
mailing list