From 82ee16beb8fe91b1f78416470faf71922e5efbeb Mon Sep 17 00:00:00 2001 From: Vedant Kumar Date: Sat, 25 Feb 2017 00:43:36 +0000 Subject: [PATCH] [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193) 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). Testing: check-clang and check-ubsan. Differential Revision: https://reviews.llvm.org/D29369 llvm-svn: 296213 --- clang/lib/CodeGen/CGExprScalar.cpp | 75 ++++++++++- clang/test/CodeGen/compound-assign-overflow.c | 4 +- clang/test/CodeGen/ubsan-promoted-arith.cpp | 124 ++++++++++++++++++ clang/test/CodeGen/unsigned-promotion.c | 113 ---------------- 4 files changed, 196 insertions(+), 120 deletions(-) create mode 100644 clang/test/CodeGen/ubsan-promoted-arith.cpp diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp index 63031d44644c..c14e6d12da82 100644 --- a/clang/lib/CodeGen/CGExprScalar.cpp +++ b/clang/lib/CodeGen/CGExprScalar.cpp @@ -24,6 +24,7 @@ #include "clang/AST/StmtVisitor.h" #include "clang/Basic/TargetInfo.h" #include "clang/Frontend/CodeGenOptions.h" +#include "llvm/ADT/Optional.h" #include "llvm/IR/CFG.h" #include "llvm/IR/Constants.h" #include "llvm/IR/DataLayout.h" @@ -58,6 +59,59 @@ static bool MustVisitNullValue(const Expr *E) { return E->getType()->isNullPtrType(); } +/// If \p E is a widened promoted integer, get its base (unpromoted) type. +static llvm::Optional getUnwidenedIntegerType(const ASTContext &Ctx, + const Expr *E) { + const Expr *Base = E->IgnoreImpCasts(); + if (E == Base) + return llvm::None; + + QualType BaseTy = Base->getType(); + if (!BaseTy->isPromotableIntegerType() || + Ctx.getTypeSize(BaseTy) >= Ctx.getTypeSize(E->getType())) + return llvm::None; + + return BaseTy; +} + +/// Check if \p E is a widened promoted integer. +static bool IsWidenedIntegerOp(const ASTContext &Ctx, const Expr *E) { + return getUnwidenedIntegerType(Ctx, E).hasValue(); +} + +/// Check if we can skip the overflow check for \p Op. +static bool CanElideOverflowCheck(const ASTContext &Ctx, 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 IsWidenedIntegerOp(Ctx, UO->getSubExpr()); + + const auto *BO = cast(Op.E); + auto OptionalLHSTy = getUnwidenedIntegerType(Ctx, BO->getLHS()); + if (!OptionalLHSTy) + return false; + + auto OptionalRHSTy = getUnwidenedIntegerType(Ctx, BO->getRHS()); + if (!OptionalRHSTy) + return false; + + QualType LHSTy = *OptionalLHSTy; + QualType RHSTy = *OptionalRHSTy; + + // We usually don't need overflow checks for binary operations with widened + // operands. Multiplication with promoted unsigned operands is a special case. + if ((Op.Opcode != BO_Mul && Op.Opcode != BO_MulAssign) || + !LHSTy->isUnsignedIntegerType() || !RHSTy->isUnsignedIntegerType()) + return true; + + // The overflow check can be skipped if either one of the unpromoted types + // are less than half the size of the promoted type. + unsigned PromotedSize = Ctx.getTypeSize(Op.E->getType()); + return (2 * Ctx.getTypeSize(LHSTy)) < PromotedSize || + (2 * Ctx.getTypeSize(RHSTy)) < PromotedSize; +} + class ScalarExprEmitter : public StmtVisitor { CodeGenFunction &CGF; @@ -482,12 +536,15 @@ public: return Builder.CreateNSWMul(Ops.LHS, Ops.RHS, "mul"); // Fall through. case LangOptions::SOB_Trapping: + if (CanElideOverflowCheck(CGF.getContext(), Ops)) + return Builder.CreateNSWMul(Ops.LHS, Ops.RHS, "mul"); return EmitOverflowCheckedBinOp(Ops); } } if (Ops.Ty->isUnsignedIntegerType() && - CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow)) + CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow) && + !CanElideOverflowCheck(CGF.getContext(), Ops)) return EmitOverflowCheckedBinOp(Ops); if (Ops.LHS->getType()->isFPOrFPVectorTy()) @@ -1663,6 +1720,8 @@ llvm::Value *ScalarExprEmitter::EmitIncDecConsiderOverflowBehavior( return Builder.CreateNSWAdd(InVal, Amount, Name); // Fall through. case LangOptions::SOB_Trapping: + if (IsWidenedIntegerOp(CGF.getContext(), E->getSubExpr())) + return Builder.CreateNSWAdd(InVal, Amount, Name); return EmitOverflowCheckedBinOp(createBinOpInfoFromIncDec(E, InVal, IsInc)); } llvm_unreachable("Unknown SignedOverflowBehaviorTy"); @@ -2281,8 +2340,10 @@ void ScalarExprEmitter::EmitUndefinedBehaviorIntegerDivAndRemCheck( SanitizerKind::IntegerDivideByZero)); } + const auto *BO = cast(Ops.E); if (CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow) && - Ops.Ty->hasSignedIntegerRepresentation()) { + Ops.Ty->hasSignedIntegerRepresentation() && + !IsWidenedIntegerOp(CGF.getContext(), BO->getLHS())) { llvm::IntegerType *Ty = cast(Zero->getType()); llvm::Value *IntMin = @@ -2634,12 +2695,15 @@ Value *ScalarExprEmitter::EmitAdd(const BinOpInfo &op) { return Builder.CreateNSWAdd(op.LHS, op.RHS, "add"); // Fall through. case LangOptions::SOB_Trapping: + if (CanElideOverflowCheck(CGF.getContext(), op)) + return Builder.CreateNSWAdd(op.LHS, op.RHS, "add"); return EmitOverflowCheckedBinOp(op); } } if (op.Ty->isUnsignedIntegerType() && - CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow)) + CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow) && + !CanElideOverflowCheck(CGF.getContext(), op)) return EmitOverflowCheckedBinOp(op); if (op.LHS->getType()->isFPOrFPVectorTy()) { @@ -2665,12 +2729,15 @@ Value *ScalarExprEmitter::EmitSub(const BinOpInfo &op) { return Builder.CreateNSWSub(op.LHS, op.RHS, "sub"); // Fall through. case LangOptions::SOB_Trapping: + if (CanElideOverflowCheck(CGF.getContext(), op)) + return Builder.CreateNSWSub(op.LHS, op.RHS, "sub"); return EmitOverflowCheckedBinOp(op); } } if (op.Ty->isUnsignedIntegerType() && - CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow)) + CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow) && + !CanElideOverflowCheck(CGF.getContext(), op)) return EmitOverflowCheckedBinOp(op); if (op.LHS->getType()->isFPOrFPVectorTy()) { diff --git a/clang/test/CodeGen/compound-assign-overflow.c b/clang/test/CodeGen/compound-assign-overflow.c index f126bb05d53c..92ae249eb9ff 100644 --- a/clang/test/CodeGen/compound-assign-overflow.c +++ b/clang/test/CodeGen/compound-assign-overflow.c @@ -25,11 +25,9 @@ void compaddunsigned() { // 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*), {{.*}}) } diff --git a/clang/test/CodeGen/ubsan-promoted-arith.cpp b/clang/test/CodeGen/ubsan-promoted-arith.cpp new file mode 100644 index 000000000000..19cbc0ebdabe --- /dev/null +++ b/clang/test/CodeGen/ubsan-promoted-arith.cpp @@ -0,0 +1,124 @@ +// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=signed-integer-overflow,unsigned-integer-overflow | FileCheck %s + +typedef unsigned char uchar; +typedef unsigned short ushort; + +enum E1 : int { + a +}; + +enum E2 : char { + b +}; + +// CHECK-LABEL: define signext i8 @_Z4add1 +// CHECK-NOT: sadd.with.overflow +char add1(char c) { return c + c; } + +// CHECK-LABEL: define zeroext i8 @_Z4add2 +// CHECK-NOT: uadd.with.overflow +uchar add2(uchar uc) { return uc + uc; } + +// CHECK-LABEL: define i32 @_Z4add3 +// CHECK: sadd.with.overflow +int add3(E1 e) { return e + a; } + +// CHECK-LABEL: define signext i8 @_Z4add4 +// CHECK-NOT: sadd.with.overflow +char add4(E2 e) { return e + b; } + +// CHECK-LABEL: define signext i8 @_Z4sub1 +// CHECK-NOT: ssub.with.overflow +char sub1(char c) { return c - c; } + +// CHECK-LABEL: define zeroext i8 @_Z4sub2 +// CHECK-NOT: usub.with.overflow +uchar sub2(uchar uc) { return uc - uc; } + +// CHECK-LABEL: define signext i8 @_Z4sub3 +// CHECK-NOT: ssub.with.overflow +char sub3(char c) { return -c; } + +// Note: -INT_MIN can overflow. +// +// CHECK-LABEL: define i32 @_Z4sub4 +// CHECK: ssub.with.overflow +int sub4(int i) { return -i; } + +// CHECK-LABEL: define signext i8 @_Z4mul1 +// CHECK-NOT: smul.with.overflow +char mul1(char c) { return c * c; } + +// CHECK-LABEL: define zeroext i8 @_Z4mul2 +// CHECK-NOT: smul.with.overflow +uchar mul2(uchar uc) { return uc * uc; } + +// Note: USHRT_MAX * USHRT_MAX can overflow. +// +// CHECK-LABEL: define zeroext i16 @_Z4mul3 +// CHECK: smul.with.overflow +ushort mul3(ushort us) { return us * us; } + +// CHECK-LABEL: define i32 @_Z4mul4 +// CHECK: smul.with.overflow +int mul4(int i, char c) { return i * c; } + +// CHECK-LABEL: define i32 @_Z4mul5 +// CHECK: smul.with.overflow +int mul5(int i, char c) { return c * i; } + +// CHECK-LABEL: define signext i16 @_Z4mul6 +// CHECK-NOT: smul.with.overflow +short mul6(short s) { return s * s; } + +// CHECK-LABEL: define signext i8 @_Z4div1 +// CHECK-NOT: ubsan_handle_divrem_overflow +char div1(char c) { return c / c; } + +// CHECK-LABEL: define zeroext i8 @_Z4div2 +// CHECK-NOT: ubsan_handle_divrem_overflow +uchar div2(uchar uc) { return uc / uc; } + +// CHECK-LABEL: define signext i8 @_Z4div3 +// CHECK-NOT: ubsan_handle_divrem_overflow +char div3(char c, int i) { return c / i; } + +// CHECK-LABEL: define signext i8 @_Z4div4 +// CHECK: ubsan_handle_divrem_overflow +char div4(int i, char c) { return i / c; } + +// Note: INT_MIN / -1 can overflow. +// +// CHECK-LABEL: define signext i8 @_Z4div5 +// CHECK: ubsan_handle_divrem_overflow +char div5(int i, char c) { return i / c; } + +// CHECK-LABEL: define signext i8 @_Z4rem1 +// CHECK-NOT: ubsan_handle_divrem_overflow +char rem1(char c) { return c % c; } + +// CHECK-LABEL: define zeroext i8 @_Z4rem2 +// 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 @_Z4rem3 +// rdar30301609: ubsan_handle_divrem_overflow +char rem3(int i, char c) { return i % c; } + +// CHECK-LABEL: define signext i8 @_Z4inc1 +// CHECK-NOT: sadd.with.overflow +char inc1(char c) { return c++ + (char)0; } + +// CHECK-LABEL: define zeroext i8 @_Z4inc2 +// CHECK-NOT: uadd.with.overflow +uchar inc2(uchar uc) { return uc++ + (uchar)0; } + +// CHECK-LABEL: define void @_Z4inc3 +// CHECK-NOT: sadd.with.overflow +void inc3(char c) { c++; } + +// CHECK-LABEL: define void @_Z4inc4 +// CHECK-NOT: uadd.with.overflow +void inc4(uchar uc) { uc++; } diff --git a/clang/test/CodeGen/unsigned-promotion.c b/clang/test/CodeGen/unsigned-promotion.c index 4e7a4426a03e..4b13f68781b9 100644 --- a/clang/test/CodeGen/unsigned-promotion.c +++ b/clang/test/CodeGen/unsigned-promotion.c @@ -7,53 +7,6 @@ // RUN: -fsanitize=unsigned-integer-overflow | FileCheck %s --check-prefix=CHECKU 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() @@ -75,69 +28,3 @@ void testshortmul() { // CHECKU: [[T5:%.*]] = mul nsw i32 [[T2]], [[T4]] 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() { - - // CHECKS: load i8, i8* @cj - // CHECKS: load i8, i8* @ck - // CHECKS: [[T1:%.*]] = call { i32, i1 } @llvm.smul.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_mul_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.smul - // CHECKU-NOT: llvm.umul - // CHECKU: [[T5:%.*]] = mul nsw i32 [[T2]], [[T4]] - - ci = cj * ck; -}