Re-apply "[ubsan] Sanity-check shift amounts before truncation"

vedantk · vedantk · commit d3a601b06b83 · 2017-01-30T23:38:54.000Z
This re-applies r293343 (reverts commit r293475) with a fix for an assertion failure caused by a missing integer cast. I tested this patch by using the built compiler to compile X86FastISel.cpp.o with ubsan. Original commit message: Ubsan does not report UB shifts in some cases where the shift exponent needs to be truncated to match the type of the shift base. We perform a range check on the truncated shift amount, leading to false negatives. Fix the issue (PR27271) by performing the range check on the original shift amount. Differential Revision: https://reviews.llvm.org/D29234 llvm-svn: 293572
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -2751,8 +2751,8 @@ Value *ScalarExprEmitter::EmitShl(const BinOpInfo &Ops) {
            isa<llvm::IntegerType>(Ops.LHS->getType())) {
     CodeGenFunction::SanitizerScope SanScope(&CGF);
     SmallVector<std::pair<Value *, SanitizerMask>, 2> Checks;
-    llvm::Value *WidthMinusOne = GetWidthMinusOneValue(Ops.LHS, RHS);
-    llvm::Value *ValidExponent = Builder.CreateICmpULE(RHS, WidthMinusOne);
+    llvm::Value *WidthMinusOne = GetWidthMinusOneValue(Ops.LHS, Ops.RHS);
+    llvm::Value *ValidExponent = Builder.CreateICmpULE(Ops.RHS, WidthMinusOne);
 
     if (SanitizeExponent) {
       Checks.push_back(
@@ -2767,12 +2767,14 @@ Value *ScalarExprEmitter::EmitShl(const BinOpInfo &Ops) {
       llvm::BasicBlock *Cont = CGF.createBasicBlock("cont");
       llvm::BasicBlock *CheckShiftBase = CGF.createBasicBlock("check");
       Builder.CreateCondBr(ValidExponent, CheckShiftBase, Cont);
+      llvm::Value *PromotedWidthMinusOne =
+          (RHS == Ops.RHS) ? WidthMinusOne
+                           : GetWidthMinusOneValue(Ops.LHS, RHS);
       CGF.EmitBlock(CheckShiftBase);
-      llvm::Value *BitsShiftedOff =
-        Builder.CreateLShr(Ops.LHS,
-                           Builder.CreateSub(WidthMinusOne, RHS, "shl.zeros",
-                                             /*NUW*/true, /*NSW*/true),
-                           "shl.check");
+      llvm::Value *BitsShiftedOff = Builder.CreateLShr(
+          Ops.LHS, Builder.CreateSub(PromotedWidthMinusOne, RHS, "shl.zeros",
+                                     /*NUW*/ true, /*NSW*/ true),
+          "shl.check");
       if (CGF.getLangOpts().CPlusPlus) {
         // In C99, we are not permitted to shift a 1 bit into the sign bit.
         // Under C++11's rules, shifting a 1 bit into the sign bit is
diff --git a/clang/test/CodeGen/ubsan-shift.c b/clang/test/CodeGen/ubsan-shift.c
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple=x86_64-apple-darwin -fsanitize=shift-exponent,shift-base -emit-llvm %s -o - | FileCheck %s
+
+// CHECK-LABEL: define i32 @f1
+int f1(int c, int shamt) {
+// CHECK: icmp ule i32 %{{.*}}, 31, !nosanitize
+// CHECK: icmp ule i32 %{{.*}}, 31, !nosanitize
+  return 1 << (c << shamt);
+}
+
+// CHECK-LABEL: define i32 @f2
+int f2(long c, int shamt) {
+// CHECK: icmp ule i32 %{{.*}}, 63, !nosanitize
+// CHECK: icmp ule i64 %{{.*}}, 31, !nosanitize
+  return 1 << (c << shamt);
+}
+
+// CHECK-LABEL: define i32 @f3
+unsigned f3(unsigned c, int shamt) {
+// CHECK: icmp ule i32 %{{.*}}, 31, !nosanitize
+// CHECK: icmp ule i32 %{{.*}}, 31, !nosanitize
+  return 1U << (c << shamt);
+}
+
+// CHECK-LABEL: define i32 @f4
+unsigned f4(unsigned long c, int shamt) {
+// CHECK: icmp ule i32 %{{.*}}, 63, !nosanitize
+// CHECK: icmp ule i64 %{{.*}}, 31, !nosanitize
+  return 1U << (c << shamt);
+}
+
+// CHECK-LABEL: define i32 @f5
+int f5(int c, long long shamt) {
+// CHECK: icmp ule i64 %{{[0-9]+}}, 31, !nosanitize
+//
+// CHECK: sub nuw nsw i32 31, %sh_prom, !nosanitize
+// CHECK: lshr i32 %{{.*}}, %shl.zeros, !nosanitize
+  return c << shamt;
+}
+
+// CHECK-LABEL: define i32 @f6
+int f6(int c, int shamt) {
+// CHECK: icmp ule i32 %[[WIDTH:.*]], 31, !nosanitize
+//
+// CHECK: sub nuw nsw i32 31, %[[WIDTH]], !nosanitize
+// CHECK: lshr i32 %{{.*}}, %shl.zeros, !nosanitize
+  return c << shamt;
+}
