[ubsan] Skip range checks for width-limited values

[vsk]

Summary

UBSan can check that scalar loads provide in-range values. When we load
a value from a bitfield, we know that the range of the value is
constrained by the bitfield's width. This patch teaches UBSan how to use
that information to skip emitting some range checks.

This depends on / is a follow-up to: https://reviews.llvm.org/D30423

[vsk]

• Fix an incorrect comment on the field "e2" in struct S. We emit a check for it because 0b11 = -1 = 3.
• Skip the range check on the field "e2" in struct S2, because the range of the bitfield is the same as the range clang infers for the enum.

[arphaman]

You should also add a test for enum E : unsigned.

[filcab]

test/CodeGenCXX/ubsan-bitfields.cpp

39

Add checks/check-nots to make sure the thing you don't want isn't emitted, not just !nosanitize
The checks help document what you're trying to get in each test case.
Here I'd prefer to have a CHECK-NOT: __ubsan_handle_load_invalid_value than a check-not for !nosanitize, since checking for the handler call is more explicit.

[vsk]

In D30729#695463, @arphaman wrote:

You should also add a test for enum E : unsigned.

Ubsan doesn't try to infer the range of enums with a fixed, underlying type. I'll add a test case to make sure we don't insert a check.

test/CodeGenCXX/ubsan-bitfields.cpp

39

Good point, I will update the patch shortly.

[vsk]

• Make check-not's a bit stricter per Filipe's comment.
• Add a negative test for fixed enums.

lib/CodeGen/CGExpr.cpp

if (!getRangeForType(*this, Ty, Min, End, CGM.getCodeGenOpts().StrictEnums,
                        hasBooleanRepresentation(Ty)))
     return nullptr;
 
   llvm::MDBuilder MDHelper(getLLVMContext());
   return MDHelper.createRange(Min, End);
 }
 
 bool CodeGenFunction::EmitScalarRangeCheck(llvm::Value *Value, QualType Ty,
-                                           SourceLocation Loc) {
+                                           SourceLocation Loc,
+                                           Optional<unsigned> BitWidth) {
   bool HasBoolCheck = SanOpts.has(SanitizerKind::Bool);
   bool HasEnumCheck = SanOpts.has(SanitizerKind::Enum);
   if (!HasBoolCheck && !HasEnumCheck)
     return false;
 
   bool IsBool = hasBooleanRepresentation(Ty) ||
                 NSAPI(CGM.getContext()).isObjCBOOLType(Ty);
   bool NeedsBoolCheck = HasBoolCheck && IsBool;
   bool NeedsEnumCheck = HasEnumCheck && Ty->getAs<EnumType>();
   if (!NeedsBoolCheck && !NeedsEnumCheck)
     return false;
 
   llvm::APInt Min, End;
   if (!getRangeForType(*this, Ty, Min, End, /*StrictEnums=*/true, IsBool))
     return true;
 
+  --End;
+  if (BitWidth) {
+    if (!Min) {
+      // If End > MaxValueForWidth, then Value < End. Skip the range check.
+      auto MaxValueForWidth =
+          llvm::APInt::getMaxValue(*BitWidth).zextOrSelf(End.getBitWidth());
+      if (End.ugt(MaxValueForWidth))
+        return false;
+    } else {
+      assert(Min.eq(-(End + 1)) &&
+             "The full range check assumes Min = -(End + 1)");
+      assert(Ty->hasSignedIntegerRepresentation() &&
+             "The full range check works on signed integers only");
+
+      // If Min <= MinValueForWidth, then Value >= Min. Since Min = -(End + 1),
+      // End >= MaxValueForWidth, and Value <= End. Skip the range check.
+      auto MinValueForWidth =
+          llvm::APInt::getSignedMinValue(*BitWidth).sextOrSelf(
+              End.getBitWidth());
+      if (Min.sle(MinValueForWidth))
+        return false;
+    }
+  }
+
   SanitizerScope SanScope(this);
   llvm::Value *Check;
-  --End;
   if (!Min) {
     Check = Builder.CreateICmpULE(
         Value, llvm::ConstantInt::get(getLLVMContext(), End));
   } else {
     llvm::Value *Upper = Builder.CreateICmpSLE(
         Value, llvm::ConstantInt::get(getLLVMContext(), End));
     llvm::Value *Lower = Builder.CreateICmpSGE(
         Value, llvm::ConstantInt::get(getLLVMContext(), Min));
if (Info.IsSigned) {
     if (Info.Offset)
       Val = Builder.CreateLShr(Val, Info.Offset, "bf.lshr");
     if (static_cast<unsigned>(Info.Offset) + Info.Size < Info.StorageSize)
       Val = Builder.CreateAnd(Val, llvm::APInt::getLowBitsSet(Info.StorageSize,
                                                               Info.Size),
                               "bf.clear");
   }
   Val = Builder.CreateIntCast(Val, ResLTy, Info.IsSigned, "bf.cast");
-  EmitScalarRangeCheck(Val, LV.getType(), Loc);
+  EmitScalarRangeCheck(Val, LV.getType(), Loc, Info.Size);
   return RValue::get(Val);
 }
 
 // If this is a reference to a subset of the elements of a vector, create an
 // appropriate shufflevector.
 RValue CodeGenFunction::EmitLoadOfExtVectorElementLValue(LValue LV) {
   llvm::Value *Vec = Builder.CreateLoad(LV.getExtVectorAddress(),
                                         LV.isVolatileQualified());

lib/CodeGen/CodeGenFunction.h

public:
   /// representation to its in-memory representation.
   llvm::Value *EmitToMemory(llvm::Value *Value, QualType Ty);
 
   /// EmitFromMemory - Change a scalar value from its memory
   /// representation to its value representation.
   llvm::Value *EmitFromMemory(llvm::Value *Value, QualType Ty);
 
   /// Check if the scalar \p Value is within the valid range for the given
-  /// type \p Ty.
+  /// type \p Ty. If \p BitWidth is provided, it must be the bit width of the
+  /// storage for the scalar.
   ///
-  /// Returns true if a check is needed (even if the range is unknown).
-  bool EmitScalarRangeCheck(llvm::Value *Value, QualType Ty,
-                            SourceLocation Loc);
+  /// Returns true if range metadata for the scalar should be dropped.
+  bool EmitScalarRangeCheck(llvm::Value *Value, QualType Ty, SourceLocation Loc,
+                            Optional<unsigned> BitWidth = None);
 
   /// EmitLoadOfScalar - Load a scalar value from an address, taking
   /// care to appropriately convert from the memory representation to
   /// the LLVM value representation.
   llvm::Value *EmitLoadOfScalar(Address Addr, bool Volatile, QualType Ty,
                                 SourceLocation Loc,
                                 AlignmentSource AlignSource =
                                   AlignmentSource::Type,

test/CodeGenCXX/ubsan-bitfields.cpp

 // RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=enum | FileCheck %s
 
 enum E {
   a = 1,
   b = 2,
   c = 3
 };
 
 struct S {
-  E e1 : 10;
+  E e1 : 10; //< Wide enough for the unsigned range check.
+  E e2 : 2; //< Emit the check, since -1 = 3 = 0b11.
+  E e3 : 1; //< Skip the check.
 };
 
 // CHECK-LABEL: define i32 @_Z4loadP1S
 E load(S *s) {
   // CHECK: [[LOAD:%.*]] = load i16, i16* {{.*}}
   // CHECK: [[CLEAR:%.*]] = and i16 [[LOAD]], 1023
   // CHECK: [[CAST:%.*]] = zext i16 [[CLEAR]] to i32
   // CHECK: icmp ule i32 [[CAST]], 3, !nosanitize
   // CHECK: call void @__ubsan_handle_load_invalid_value
   return s->e1;
 }
+
+// CHECK-LABEL: define i32 @_Z5load2P1S
+E load2(S *s) {
+  // CHECK: [[LOAD:%.*]] = load i16, i16* {{.*}}
+  // CHECK: [[LSHR:%.*]] = lshr i16 [[LOAD]], 10
+  // CHECK: [[CLEAR:%.*]] = and i16 [[LSHR]], 3
+  // CHECK: [[CAST:%.*]] = zext i16 [[CLEAR]] to i32
+  // CHECK: icmp ule i32 [[CAST]], 3, !nosanitize
+  // CHECK: call void @__ubsan_handle_load_invalid_value
+  return s->e2;
+}
+
+// CHECK-LABEL: define i32 @_Z5load3P1S
+E load3(S *s) {
+  // CHECK-NOT: __ubsan_handle_load_invalid_value
+  // CHECK-NOT: !nosanitize
+  return s->e3;
+}
+
+enum E2 {
+  x = -3,
+  y = 3
+};
+
+struct S2 {
+  E2 e1 : 4; //< Wide enough for signed range checks.
+  E2 e2 : 3; //< Skip the check, since the range of the bitfield is the same
+             //  as the range of the enum: [-4, 3].
+  E2 e3 : 2; //< Still skip the check.
+};
+
+// CHECK-LABEL: define i32 @_Z5load4P2S2
+E2 load4(S2 *s) {
+  // CHECK: [[LOAD:%.*]] = load i16, i16* {{.*}}
+  // CHECK: [[SHL:%.*]] = shl i16 [[LOAD]], 12
+  // CHECK: [[ASHR:%.*]] = ashr i16 [[SHL]], 12
+  // CHECK: [[CAST:%.*]] = sext i16 [[ASHR]] to i32
+  // CHECK: [[UPPER_BOUND:%.*]] = icmp sle i32 [[CAST]], 3, !nosanitize
+  // CHECK: [[LOWER_BOUND:%.*]] = icmp sge i32 [[CAST]], -4, !nosanitize
+  // CHECK: [[BOUND:%.*]] = and i1 [[UPPER_BOUND]], [[LOWER_BOUND]], !nosanitize
+  // CHECK: br i1 [[BOUND]], {{.*}}, !nosanitize
+  // CHECK: call void @__ubsan_handle_load_invalid_value
+  return s->e1;
+}
+
+// CHECK-LABEL: define i32 @_Z5load5P2S2
+E2 load5(S2 *s) {
+  // CHECK-NOT: __ubsan_handle_load_invalid_value
+  // CHECK-NOT: !nosanitize
+  return s->e2;
+}
+
+// CHECK-LABEL: define i32 @_Z5load6P2S2
+E2 load6(S2 *s) {
+  // CHECK-NOT: __ubsan_handle_load_invalid_value
+  // CHECK-NOT: !nosanitize
+  return s->e3;
+}
+
+// ubsan does not attempt to guess the range of fixed enums, i.e enums with a
+// fixed, underlying type. Check that we don't emit range checks for it.
+enum E3 : unsigned {
+  q = 1,
+  p = 3
+};
+
+struct S3 {
+  E3 e1 : 3;
+  E3 e2 : 2;
+};
+
+// CHECK-LABEL: define i32 @_Z5load7P2S3
+E3 load7(S3 *s) {
+  // CHECK-NOT: __ubsan_handle_load_invalid_value
+  // CHECK-NOT: !nosanitize
+  return s->e1;
+}
+
+// CHECK-LABEL: define i32 @_Z5load8P2S3
+E3 load8(S3 *s) {
+  // CHECK-NOT: __ubsan_handle_load_invalid_value
+  // CHECK-NOT: !nosanitize
+  return s->e2;
+}