vsk updated this revision to Diff 86739.
vsk marked an inline comment as done.
vsk added a comment.
- Per Eli's comment: check that integers are actually widened, instead of
incorrectly assuming they are always widened. I added some test cases for this.
- Address the 'fixme' regarding multiplication with unsigned operands.
https://reviews.llvm.org/D29369
Files:
lib/CodeGen/CGExprScalar.cpp
test/CodeGen/compound-assign-overflow.c
test/CodeGen/ubsan-promoted-arith.cpp
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.cpp
===================================================================
--- /dev/null
+++ test/CodeGen/ubsan-promoted-arith.cpp
@@ -0,0 +1,116 @@
+// 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: -SHRT_MIN * -SHRT_MIN 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; }
+
+// Note: -INT_MIN / -1 can overflow.
+//
+// CHECK-LABEL: define signext i8 @_Z4div3
+// CHECK: ubsan_handle_divrem_overflow
+char div3(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++; }
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
@@ -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,54 @@
return E->getType()->isNullPtrType();
}
+/// If \p E is a widened promoted integer, get its base (unpromoted) type.
+static llvm::Optional<QualType> 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 we can skip the overflow check for \p Op.
+static bool CanElideOverflowCheck(const ASTContext &Ctx, 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 getUnwidenedIntegerType(Ctx, UO->getSubExpr()).hasValue();
+
+ const auto *BO = cast<BinaryOperator>(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<ScalarExprEmitter, Value*> {
CodeGenFunction &CGF;
@@ -456,20 +505,21 @@
// 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(CGF.getContext(), Ops)) {
+ return Builder.CreateNSWMul(Ops.LHS, Ops.RHS, "mul");
+ } else {
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())
@@ -1637,17 +1687,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)) ||
+ getUnwidenedIntegerType(CGF.getContext(), E->getSubExpr())) {
+ return Builder.CreateNSWAdd(InVal, Amount, Name);
+ } else {
return EmitOverflowCheckedBinOp(createBinOpInfoFromIncDec(E, InVal, IsInc));
}
- llvm_unreachable("Unknown SignedOverflowBehaviorTy");
}
llvm::Value *
@@ -2263,8 +2312,10 @@
SanitizerKind::IntegerDivideByZero));
}
+ const auto *BO = cast<BinaryOperator>(Ops.E);
if (CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow) &&
- Ops.Ty->hasSignedIntegerRepresentation()) {
+ Ops.Ty->hasSignedIntegerRepresentation() &&
+ !getUnwidenedIntegerType(CGF.getContext(), BO->getLHS())) {
llvm::IntegerType *Ty = cast<llvm::IntegerType>(Zero->getType());
llvm::Value *IntMin =
@@ -2608,20 +2659,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(CGF.getContext(), 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(CGF.getContext(), op))
return EmitOverflowCheckedBinOp(op);
if (op.LHS->getType()->isFPOrFPVectorTy()) {
@@ -2639,20 +2691,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(CGF.getContext(), 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(CGF.getContext(), 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
