[clang] [clang][CodeGen] Fix shift-exponent ubsan check for signed _BitInt (PR #88004)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 8 08:33:58 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-codegen
Author: Björn Pettersson (bjope)
<details>
<summary>Changes</summary>
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/88004.diff
2 Files Affected:
- (modified) clang/lib/CodeGen/CGExprScalar.cpp (+23-8)
- (modified) clang/test/CodeGen/ubsan-shift-bitint.c (+31-2)
``````````diff
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;
+}
+
``````````
</details>
https://github.com/llvm/llvm-project/pull/88004
More information about the cfe-commits
mailing list