[ubsan] Detect signed overflow UB in remainder operations

[vsk]

Summary

Teach ubsan to diagnose remainder operations which have undefined behavior due to signed overflow.

My copy of the C11 spec draft (6.5.5.6) says that: if the quotient a/b is representable, (a/b)*b + a%b shall equal a; otherwise, the behavior of both a/b and a%b is undefined. I take this to mean INT_MIN % -1 has signed overflow UB, so we should check for that.

Loosely depends on: https://reviews.llvm.org/D29369

[regehr]

Does this check need to be sensitive to the dialect of C/C++ that the user asked for? I know that it used to be the case that the standard could be read either way for this case, but as you observe it is now unambiguously UB.

[vsk]

In D29437#664379, @regehr wrote:

Does this check need to be sensitive to the dialect of C/C++ that the user asked for? I know that it used to be the case that the standard could be read either way for this case, but as you observe it is now unambiguously UB.

No, I don't think the check should be sensitive to language dialect.

You're right about the C99 spec not being very explicit about what 'a % b' means if 'a / b' is not representable. It's simply the "remainder" of 'a / b'. However, clang treats 'INT_MIN % -1' as having UB in C99 mode (rightly, imho), so it'd be nice to have a diagnostic for it.

[efriedma]

lib/CodeGen/CGExprScalar.cpp

2380

I don't think you need the isIntegerType check here?

[vsk]

• Add a small test that shows why the 'isIntegerType' check is required (we'd crash otherwise).

[efriedma]

LGTM.

lib/CodeGen/CGExprScalar.cpp

Value *ScalarExprEmitter::EmitDiv(const BinOpInfo &Ops) {
     } else if (CGF.SanOpts.has(SanitizerKind::FloatDivideByZero) &&
                Ops.Ty->isRealFloatingType()) {
       llvm::Value *Zero = llvm::Constant::getNullValue(ConvertType(Ops.Ty));
       llvm::Value *NonZero = Builder.CreateFCmpUNE(Ops.RHS, Zero);
       EmitBinOpCheck(std::make_pair(NonZero, SanitizerKind::FloatDivideByZero),
                      Ops);
     }
   }
 
   if (Ops.LHS->getType()->isFPOrFPVectorTy()) {
     llvm::Value *Val = Builder.CreateFDiv(Ops.LHS, Ops.RHS, "div");
     if (CGF.getLangOpts().OpenCL &&
         !CGF.CGM.getCodeGenOpts().CorrectlyRoundedDivSqrt) {
       // OpenCL v1.1 s7.4: minimum accuracy of single precision / is 2.5ulp
       // OpenCL v1.2 s5.6.4.2: The -cl-fp32-correctly-rounded-divide-sqrt
       // build option allows an application to specify that single precision
       // floating-point divide (x/y and 1/x) and sqrt used in the program
Value *ScalarExprEmitter::EmitDiv(const BinOpInfo &Ops) {
   else if (Ops.Ty->hasUnsignedIntegerRepresentation())
     return Builder.CreateUDiv(Ops.LHS, Ops.RHS, "div");
   else
     return Builder.CreateSDiv(Ops.LHS, Ops.RHS, "div");
 }
 
 Value *ScalarExprEmitter::EmitRem(const BinOpInfo &Ops) {
   // Rem in C can't be a floating point type: C99 6.5.5p2.
-  if (CGF.SanOpts.has(SanitizerKind::IntegerDivideByZero)) {
+  if ((CGF.SanOpts.has(SanitizerKind::IntegerDivideByZero) ||
+       CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow)) &&
+      Ops.Ty->isIntegerType()) {
     CodeGenFunction::SanitizerScope SanScope(&CGF);
     llvm::Value *Zero = llvm::Constant::getNullValue(ConvertType(Ops.Ty));
-
-    if (Ops.Ty->isIntegerType())
-      EmitUndefinedBehaviorIntegerDivAndRemCheck(Ops, Zero, false);
   }
 
   if (Ops.Ty->hasUnsignedIntegerRepresentation())
     return Builder.CreateURem(Ops.LHS, Ops.RHS, "rem");
   else
     return Builder.CreateSRem(Ops.LHS, Ops.RHS, "rem");
 }
 

test/CodeGen/ubsan-promoted-arith.cpp

 // 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;
+typedef int int4 __attribute__((ext_vector_type(4)));
 
 enum E1 : int {
   a
 };
 
 enum E2 : char {
   b
 };

 // 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
+// CHECK: ubsan_handle_divrem_overflow
 char rem3(int i, char c) { return i % c; }
 
+// CHECK-LABEL: define signext i8 @_Z4rem4
+// CHECK-NOT: ubsan_handle_divrem_overflow
+char rem4(char c, int i) { return c % i; }
+
 // 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++; }
+
+// CHECK-LABEL: define <4 x i32> @_Z4vremDv4_iS_
+// CHECK-NOT: ubsan_handle_divrem_overflow
+int4 vrem(int4 a, int4 b) { return a % b; }