[clang] [clang][CodeGen] Fix shift-exponent ubsan check for signed _BitInt (PR #88004)
Björn Pettersson via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 8 08:33:29 PDT 2024
https://github.com/bjope created https://github.com/llvm/llvm-project/pull/88004
Commit 5f87957fefb21d454f2f (pull-requst #80515) corrected some codegen problems related to _BitInt types being used as shift exponents. But it did not fix it properly for the special case when the shift count operand is a signed _BitInt.
The basic problem is the same as the one solved for unsigned _BitInt. As we use an unsigned comparison to see if the shift exponent is out-of-bounds, then we need to find an unsigned maximum allowed shift amount to use in the check. Normally the shift amount is limited by bitwidth of the LHS of the shift. However, when the RHS type is small in relation to the LHS then we need to use a value that fits inside the bitwidth of the RHS instead.
The earlier fix simply used the unsigned maximum when deterining the max shift amount based on the RHS type. It did however not take into consideration that the RHS type could have a signed representation. In such situations we need to use the signed maximum instead. Otherwise we do not recognize a negative shift exponent as UB.
>From df9c1202ec1e9ceb7c1079201b94bae8576cd73f Mon Sep 17 00:00:00 2001
From: Bjorn Pettersson <bjorn.a.pettersson at ericsson.com>
Date: Mon, 8 Apr 2024 14:33:07 +0200
Subject: [PATCH] [clang][CodeGen] Fix shift-exponent ubsan check for signed
_BitInt
Commit 5f87957fefb21d454f2f (pull-requst #80515) corrected some
codegen problems related to _BitInt types being used as shift
exponents. But it did not fix it properly for the special
case when the shift count operand is a signed _BitInt.
The basic problem is the same as the one solved for unsigned
_BitInt. As we use an unsigned comparison to see if the shift
exponent is out-of-bounds, then we need to find an unsigned
maximum allowed shift amount to use in the check. Normally the
shift amount is limited by bitwidth of the LHS of the shift.
However, when the RHS type is small in relation to the LHS then
we need to use a value that fits inside the bitwidth of the RHS
instead.
The earlier fix simply used the unsigned maximum when deterining
the max shift amount based on the RHS type. It did however not
take into consideration that the RHS type could have a signed
representation. In such situations we need to use the signed
maximum instead. Otherwise we do not recognize a negative shift
exponent as UB.
---
clang/lib/CodeGen/CGExprScalar.cpp | 31 +++++++++++++++++------
clang/test/CodeGen/ubsan-shift-bitint.c | 33 +++++++++++++++++++++++--
2 files changed, 54 insertions(+), 10 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 397b4977acc3e9..12721c9ac0d26e 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -146,6 +146,15 @@ struct BinOpInfo {
return UnOp->getSubExpr()->getType()->isFixedPointType();
return false;
}
+
+ /// Check if the RHS has a signed integer representation.
+ bool rhsHasSignedIntegerRepresentation() const {
+ if (const auto *BinOp = dyn_cast<BinaryOperator>(E)) {
+ QualType RHSType = BinOp->getRHS()->getType();
+ return RHSType->hasSignedIntegerRepresentation();
+ }
+ return false;
+ }
};
static bool MustVisitNullValue(const Expr *E) {
@@ -780,7 +789,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, bool RHSIsSigned);
// Used for shifting constraints for OpenCL, do mask for powers of 2, URem for
// non powers of two.
@@ -4181,7 +4190,8 @@ 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,
+ bool RHSIsSigned) {
llvm::IntegerType *Ty;
if (llvm::VectorType *VT = dyn_cast<llvm::VectorType>(LHS->getType()))
Ty = cast<llvm::IntegerType>(VT->getElementType());
@@ -4192,7 +4202,9 @@ Value *ScalarExprEmitter::GetMaximumShiftAmount(Value *LHS, Value *RHS) {
// 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());
+ llvm::APInt RHSMax =
+ RHSIsSigned ? llvm::APInt::getSignedMaxValue(RHSTy->getScalarSizeInBits())
+ : llvm::APInt::getMaxValue(RHSTy->getScalarSizeInBits());
if (RHSMax.ult(Ty->getBitWidth()))
return llvm::ConstantInt::get(RHSTy, RHSMax);
return llvm::ConstantInt::get(RHSTy, Ty->getBitWidth() - 1);
@@ -4207,7 +4219,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, false), Name);
return Builder.CreateURem(
RHS, llvm::ConstantInt::get(RHS->getType(), Ty->getBitWidth()), Name);
@@ -4240,7 +4252,9 @@ 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 = GetMaximumShiftAmount(Ops.LHS, Ops.RHS);
+ bool RHSIsSigned = Ops.rhsHasSignedIntegerRepresentation();
+ llvm::Value *WidthMinusOne =
+ GetMaximumShiftAmount(Ops.LHS, Ops.RHS, RHSIsSigned);
llvm::Value *ValidExponent = Builder.CreateICmpULE(Ops.RHS, WidthMinusOne);
if (SanitizeExponent) {
@@ -4258,7 +4272,7 @@ Value *ScalarExprEmitter::EmitShl(const BinOpInfo &Ops) {
Builder.CreateCondBr(ValidExponent, CheckShiftBase, Cont);
llvm::Value *PromotedWidthMinusOne =
(RHS == Ops.RHS) ? WidthMinusOne
- : GetMaximumShiftAmount(Ops.LHS, RHS);
+ : GetMaximumShiftAmount(Ops.LHS, RHS, RHSIsSigned);
CGF.EmitBlock(CheckShiftBase);
llvm::Value *BitsShiftedOff = Builder.CreateLShr(
Ops.LHS, Builder.CreateSub(PromotedWidthMinusOne, RHS, "shl.zeros",
@@ -4308,8 +4322,9 @@ Value *ScalarExprEmitter::EmitShr(const BinOpInfo &Ops) {
else if (CGF.SanOpts.has(SanitizerKind::ShiftExponent) &&
isa<llvm::IntegerType>(Ops.LHS->getType())) {
CodeGenFunction::SanitizerScope SanScope(&CGF);
- llvm::Value *Valid =
- Builder.CreateICmpULE(Ops.RHS, GetMaximumShiftAmount(Ops.LHS, Ops.RHS));
+ bool RHSIsSigned = Ops.rhsHasSignedIntegerRepresentation();
+ llvm::Value *Valid = Builder.CreateICmpULE(
+ Ops.RHS, GetMaximumShiftAmount(Ops.LHS, Ops.RHS, RHSIsSigned));
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 844d5c4ad84618..af65ed60918b08 100644
--- a/clang/test/CodeGen/ubsan-shift-bitint.c
+++ b/clang/test/CodeGen/ubsan-shift-bitint.c
@@ -6,14 +6,14 @@
// 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
+ // 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
+ // CHECK: icmp ule [[E_SIZE]] [[E_REG]], 1,
return b >> e;
}
@@ -34,3 +34,32 @@ int test_right_literal(unsigned _BitInt(2) b) {
// CHECK: br i1 false, label %cont, label %handler.shift_out_of_bounds
return b >> 4uwb;
}
+
+// CHECK-LABEL: define{{.*}} i32 @test_signed_left_variable
+int test_signed_left_variable(unsigned _BitInt(15) b, _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_signed_right_variable
+int test_signed_right_variable(unsigned _BitInt(32) b, _BitInt(4) e) {
+ // CHECK: [[E_REG:%.+]] = load [[E_SIZE:i4]]
+ // CHECK: icmp ule [[E_SIZE]] [[E_REG]], 7,
+ return b >> e;
+}
+
+// CHECK-LABEL: define{{.*}} i32 @test_signed_left_literal
+int test_signed_left_literal(unsigned _BitInt(16) 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 << (_BitInt(4))-2wb;
+}
+
+// CHECK-LABEL: define{{.*}} i32 @test_signed_right_literal
+int test_signed_right_literal(unsigned _BitInt(16) 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 >> (_BitInt(4))-8wb;
+}
+
More information about the cfe-commits
mailing list