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(Op.E) || + isa(Op.E) && "Expected a unary or binary operator"); + + if (const auto *UO = dyn_cast(Op.E)) + return IsPromotedInteger(UO->getSubExpr()); + + const auto *BO = cast(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 { 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(Ops.E); if (CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow) && - Ops.Ty->hasSignedIntegerRepresentation()) { + Ops.Ty->hasSignedIntegerRepresentation() && + !IsPromotedInteger(BO->getLHS())) { llvm::IntegerType *Ty = cast(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()) { 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: 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/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() {