vsk created this revision.
C requires the operands of arithmetic expressions to be promoted if
their types are smaller than an int. Ubsan emits overflow checks when
this sort of type promotion occurs, even if there is no way to actually
get an overflow with the promoted type.
This patch teaches clang how to omit the superflous overflow checks
(addressing PR20193). To my knowledge, this patch only misses the case
where we have multiplications with promoted unsigned operands. E.g, in
this case, we don't need an overflow check when targeting a platform
with >=32-bit ints:
uint8_t a, b;
a * b;
Testing: check-clang and check-ubsan.
https://reviews.llvm.org/D29369
Files:
lib/CodeGen/CGExprScalar.cpp
test/CodeGen/compound-assign-overflow.c
test/CodeGen/ubsan-promoted-arith.c
test/CodeGen/unsigned-promotion.c
Index: test/CodeGen/unsigned-promotion.c
===================================================================
--- test/CodeGen/unsigned-promotion.c
+++ test/CodeGen/unsigned-promotion.c
@@ -9,52 +9,6 @@
unsigned short si, sj, sk;
unsigned char ci, cj, ck;
-extern void opaqueshort(unsigned short);
-extern void opaquechar(unsigned char);
-
-// CHECKS-LABEL: define void @testshortadd()
-// CHECKU-LABEL: define void @testshortadd()
-void testshortadd() {
- // CHECKS: load i16, i16* @sj
- // CHECKS: load i16, i16* @sk
- // CHECKS: [[T1:%.*]] = call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
- // CHECKS-NEXT: [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
- // CHECKS-NEXT: [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
- // CHECKS: call void @__ubsan_handle_add_overflow
- //
- // CHECKU: [[T1:%.*]] = load i16, i16* @sj
- // CHECKU: [[T2:%.*]] = zext i16 [[T1]]
- // CHECKU: [[T3:%.*]] = load i16, i16* @sk
- // CHECKU: [[T4:%.*]] = zext i16 [[T3]]
- // CHECKU-NOT: llvm.sadd
- // CHECKU-NOT: llvm.uadd
- // CHECKU: [[T5:%.*]] = add nsw i32 [[T2]], [[T4]]
-
- si = sj + sk;
-}
-
-// CHECKS-LABEL: define void @testshortsub()
-// CHECKU-LABEL: define void @testshortsub()
-void testshortsub() {
-
- // CHECKS: load i16, i16* @sj
- // CHECKS: load i16, i16* @sk
- // CHECKS: [[T1:%.*]] = call { i32, i1 } @llvm.ssub.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
- // CHECKS-NEXT: [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
- // CHECKS-NEXT: [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
- // CHECKS: call void @__ubsan_handle_sub_overflow
- //
- // CHECKU: [[T1:%.*]] = load i16, i16* @sj
- // CHECKU: [[T2:%.*]] = zext i16 [[T1]]
- // CHECKU: [[T3:%.*]] = load i16, i16* @sk
- // CHECKU: [[T4:%.*]] = zext i16 [[T3]]
- // CHECKU-NOT: llvm.ssub
- // CHECKU-NOT: llvm.usub
- // CHECKU: [[T5:%.*]] = sub nsw i32 [[T2]], [[T4]]
-
- si = sj - sk;
-}
-
// CHECKS-LABEL: define void @testshortmul()
// CHECKU-LABEL: define void @testshortmul()
void testshortmul() {
@@ -76,50 +30,6 @@
si = sj * sk;
}
-// CHECKS-LABEL: define void @testcharadd()
-// CHECKU-LABEL: define void @testcharadd()
-void testcharadd() {
-
- // CHECKS: load i8, i8* @cj
- // CHECKS: load i8, i8* @ck
- // CHECKS: [[T1:%.*]] = call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
- // CHECKS-NEXT: [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
- // CHECKS-NEXT: [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
- // CHECKS: call void @__ubsan_handle_add_overflow
- //
- // CHECKU: [[T1:%.*]] = load i8, i8* @cj
- // CHECKU: [[T2:%.*]] = zext i8 [[T1]]
- // CHECKU: [[T3:%.*]] = load i8, i8* @ck
- // CHECKU: [[T4:%.*]] = zext i8 [[T3]]
- // CHECKU-NOT: llvm.sadd
- // CHECKU-NOT: llvm.uadd
- // CHECKU: [[T5:%.*]] = add nsw i32 [[T2]], [[T4]]
-
- ci = cj + ck;
-}
-
-// CHECKS-LABEL: define void @testcharsub()
-// CHECKU-LABEL: define void @testcharsub()
-void testcharsub() {
-
- // CHECKS: load i8, i8* @cj
- // CHECKS: load i8, i8* @ck
- // CHECKS: [[T1:%.*]] = call { i32, i1 } @llvm.ssub.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
- // CHECKS-NEXT: [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
- // CHECKS-NEXT: [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
- // CHECKS: call void @__ubsan_handle_sub_overflow
- //
- // CHECKU: [[T1:%.*]] = load i8, i8* @cj
- // CHECKU: [[T2:%.*]] = zext i8 [[T1]]
- // CHECKU: [[T3:%.*]] = load i8, i8* @ck
- // CHECKU: [[T4:%.*]] = zext i8 [[T3]]
- // CHECKU-NOT: llvm.ssub
- // CHECKU-NOT: llvm.usub
- // CHECKU: [[T5:%.*]] = sub nsw i32 [[T2]], [[T4]]
-
- ci = cj - ck;
-}
-
// CHECKS-LABEL: define void @testcharmul()
// CHECKU-LABEL: define void @testcharmul()
void testcharmul() {
Index: test/CodeGen/ubsan-promoted-arith.c
===================================================================
--- /dev/null
+++ test/CodeGen/ubsan-promoted-arith.c
@@ -0,0 +1,83 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=signed-integer-overflow,unsigned-integer-overflow | FileCheck %s
+
+typedef unsigned char uchar;
+
+// CHECK-LABEL: define signext i8 @add1
+// CHECK-NOT: sadd.with.overflow
+char add1(char c) { return c + c; }
+
+// CHECK-LABEL: define zeroext i8 @add2
+// CHECK-NOT: uadd.with.overflow
+uchar add2(uchar uc) { return uc + uc; }
+
+// CHECK-LABEL: define signext i8 @sub1
+// CHECK-NOT: ssub.with.overflow
+char sub1(char c) { return c - c; }
+
+// CHECK-LABEL: define zeroext i8 @sub2
+// CHECK-NOT: usub.with.overflow
+uchar sub2(uchar uc) { return uc - uc; }
+
+// CHECK-LABEL: define signext i8 @sub3
+// CHECK-NOT: ssub.with.overflow
+char sub3(char c) { return -c; }
+
+// Note: -INT_MIN can overflow.
+//
+// CHECK-LABEL: define i32 @sub4
+// CHECK: ssub.with.overflow
+int sub4(int i) { return -i; }
+
+// CHECK-LABEL: define signext i8 @mul1
+// CHECK-NOT: smul.with.overflow
+char mul1(char c) { return c * c; }
+
+// FIXME: There is room for improvement here: we can elide the overflow check.
+//
+// CHECK-LABEL: define zeroext i8 @mul2
+// CHECK: smul.with.overflow
+uchar mul2(uchar uc) { return uc * uc; }
+
+// CHECK-LABEL: define signext i8 @div1
+// CHECK-NOT: ubsan_handle_divrem_overflow
+char div1(char c) { return c / c; }
+
+// CHECK-LABEL: define zeroext i8 @div2
+// CHECK-NOT: ubsan_handle_divrem_overflow
+uchar div2(uchar uc) { return uc / uc; }
+
+// Note: -INT_MIN / -1 can overflow.
+//
+// CHECK-LABEL: define signext i8 @div3
+// CHECK: ubsan_handle_divrem_overflow
+char div3(int i, char c) { return i / c; }
+
+// CHECK-LABEL: define signext i8 @rem1
+// CHECK-NOT: ubsan_handle_divrem_overflow
+char rem1(char c) { return c % c; }
+
+// CHECK-LABEL: define zeroext i8 @rem2
+// CHECK-NOT: ubsan_handle_divrem_overflow
+uchar rem2(uchar uc) { return uc % uc; }
+
+// FIXME: This is a long-standing false negative.
+//
+// CHECK-LABEL: define signext i8 @rem3
+// rdar30301609: ubsan_handle_divrem_overflow
+char rem3(int i, char c) { return i % c; }
+
+// CHECK-LABEL: define signext i8 @inc1
+// CHECK-NOT: sadd.with.overflow
+char inc1(char c) { return c++ + (char)0; }
+
+// CHECK-LABEL: define zeroext i8 @inc2
+// CHECK-NOT: uadd.with.overflow
+uchar inc2(uchar uc) { return uc++ + (uchar)0; }
+
+// CHECK-LABEL: define void @inc3
+// CHECK-NOT: sadd.with.overflow
+void inc3(char c) { c++; }
+
+// CHECK-LABEL: define void @inc4
+// CHECK-NOT: uadd.with.overflow
+void inc4(uchar uc) { uc++; }
Index: test/CodeGen/compound-assign-overflow.c
===================================================================
--- test/CodeGen/compound-assign-overflow.c
+++ test/CodeGen/compound-assign-overflow.c
@@ -25,11 +25,9 @@
// CHECK: @__ubsan_handle_add_overflow(i8* bitcast ({{.*}} @[[LINE_200]] to i8*), {{.*}})
}
-int8_t a, b;
-
// CHECK: @compdiv
void compdiv() {
#line 300
- a /= b;
+ x /= x;
// CHECK: @__ubsan_handle_divrem_overflow(i8* bitcast ({{.*}} @[[LINE_300]] to i8*), {{.*}})
}
Index: lib/CodeGen/CGExprScalar.cpp
===================================================================
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -58,6 +58,35 @@
return E->getType()->isNullPtrType();
}
+/// Check if \p E is a promoted integer.
+static bool IsPromotedInteger(const Expr *E) {
+ return E->IgnoreImpCasts()->getType()->isPromotableIntegerType();
+}
+
+/// Check if we can skip the overflow check for \p Op.
+static bool CanElideOverflowCheck(const BinOpInfo &Op) {
+ assert(isa<UnaryOperator>(Op.E) ||
+ isa<BinaryOperator>(Op.E) && "Expected a unary or binary operator");
+
+ if (const auto *UO = dyn_cast<UnaryOperator>(Op.E))
+ return IsPromotedInteger(UO->getSubExpr());
+
+ const auto *BO = cast<BinaryOperator>(Op.E);
+ QualType LHSTy = BO->getLHS()->IgnoreImpCasts()->getType();
+ QualType RHSTy = BO->getRHS()->IgnoreImpCasts()->getType();
+ if (!LHSTy->isPromotableIntegerType() || !RHSTy->isPromotableIntegerType())
+ return false;
+
+ // We don't elide overflow checks for multiplication operations with unsigned
+ // operands because, e.g u16(-SHRT_MIN) * u16(-SHRT_MIN) can overflow. Maybe
+ // this should be a little less conservative?
+ if (Op.Opcode == BO_Mul || Op.Opcode == BO_MulAssign)
+ if (LHSTy->isUnsignedIntegerType() && RHSTy->isUnsignedIntegerType())
+ return false;
+
+ return true;
+}
+
class ScalarExprEmitter
: public StmtVisitor<ScalarExprEmitter, Value*> {
CodeGenFunction &CGF;
@@ -456,14 +485,14 @@
// Binary Operators.
Value *EmitMul(const BinOpInfo &Ops) {
if (Ops.Ty->isSignedIntegerOrEnumerationType()) {
- switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
- case LangOptions::SOB_Defined:
+ auto SOB = CGF.getLangOpts().getSignedOverflowBehavior();
+ if (SOB == LangOptions::SOB_Defined) {
return Builder.CreateMul(Ops.LHS, Ops.RHS, "mul");
- case LangOptions::SOB_Undefined:
- if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
- return Builder.CreateNSWMul(Ops.LHS, Ops.RHS, "mul");
- // Fall through.
- case LangOptions::SOB_Trapping:
+ } else if ((SOB == LangOptions::SOB_Undefined &&
+ !CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow)) ||
+ CanElideOverflowCheck(Ops)) {
+ return Builder.CreateNSWMul(Ops.LHS, Ops.RHS, "mul");
+ } else {
return EmitOverflowCheckedBinOp(Ops);
}
}
@@ -1637,17 +1666,16 @@
llvm::Value *Amount =
llvm::ConstantInt::get(InVal->getType(), IsInc ? 1 : -1, true);
StringRef Name = IsInc ? "inc" : "dec";
- switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
- case LangOptions::SOB_Defined:
+ auto SOB = CGF.getLangOpts().getSignedOverflowBehavior();
+ if (SOB == LangOptions::SOB_Defined) {
return Builder.CreateAdd(InVal, Amount, Name);
- case LangOptions::SOB_Undefined:
- if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
- return Builder.CreateNSWAdd(InVal, Amount, Name);
- // Fall through.
- case LangOptions::SOB_Trapping:
+ } else if ((SOB == LangOptions::SOB_Undefined &&
+ !CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow)) ||
+ IsPromotedInteger(E->getSubExpr())) {
+ return Builder.CreateNSWAdd(InVal, Amount, Name);
+ } else {
return EmitOverflowCheckedBinOp(createBinOpInfoFromIncDec(E, InVal, IsInc));
}
- llvm_unreachable("Unknown SignedOverflowBehaviorTy");
}
llvm::Value *
@@ -2263,8 +2291,10 @@
SanitizerKind::IntegerDivideByZero));
}
+ const auto *BO = cast<BinaryOperator>(Ops.E);
if (CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow) &&
- Ops.Ty->hasSignedIntegerRepresentation()) {
+ Ops.Ty->hasSignedIntegerRepresentation() &&
+ !IsPromotedInteger(BO->getLHS())) {
llvm::IntegerType *Ty = cast<llvm::IntegerType>(Zero->getType());
llvm::Value *IntMin =
@@ -2608,20 +2638,21 @@
return emitPointerArithmetic(CGF, op, /*subtraction*/ false);
if (op.Ty->isSignedIntegerOrEnumerationType()) {
- switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
- case LangOptions::SOB_Defined:
+ auto SOB = CGF.getLangOpts().getSignedOverflowBehavior();
+ if (SOB == LangOptions::SOB_Defined) {
return Builder.CreateAdd(op.LHS, op.RHS, "add");
- case LangOptions::SOB_Undefined:
- if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
- return Builder.CreateNSWAdd(op.LHS, op.RHS, "add");
- // Fall through.
- case LangOptions::SOB_Trapping:
+ } else if ((SOB == LangOptions::SOB_Undefined &&
+ !CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow)) ||
+ CanElideOverflowCheck(op)) {
+ return Builder.CreateNSWAdd(op.LHS, op.RHS, "add");
+ } else {
return EmitOverflowCheckedBinOp(op);
}
}
if (op.Ty->isUnsignedIntegerType() &&
- CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow))
+ CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow) &&
+ !CanElideOverflowCheck(op))
return EmitOverflowCheckedBinOp(op);
if (op.LHS->getType()->isFPOrFPVectorTy()) {
@@ -2639,20 +2670,21 @@
// The LHS is always a pointer if either side is.
if (!op.LHS->getType()->isPointerTy()) {
if (op.Ty->isSignedIntegerOrEnumerationType()) {
- switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
- case LangOptions::SOB_Defined:
+ auto SOB = CGF.getLangOpts().getSignedOverflowBehavior();
+ if (SOB == LangOptions::SOB_Defined) {
return Builder.CreateSub(op.LHS, op.RHS, "sub");
- case LangOptions::SOB_Undefined:
- if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
- return Builder.CreateNSWSub(op.LHS, op.RHS, "sub");
- // Fall through.
- case LangOptions::SOB_Trapping:
+ } else if ((SOB == LangOptions::SOB_Undefined &&
+ !CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow)) ||
+ CanElideOverflowCheck(op)) {
+ return Builder.CreateNSWSub(op.LHS, op.RHS, "sub");
+ } else {
return EmitOverflowCheckedBinOp(op);
}
}
if (op.Ty->isUnsignedIntegerType() &&
- CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow))
+ CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow) &&
+ !CanElideOverflowCheck(op))
return EmitOverflowCheckedBinOp(op);
if (op.LHS->getType()->isFPOrFPVectorTy()) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
