[clang][UBSan] Add implicit conversion check for bitfields

This patch implements the implicit truncation and implicit
sign change checks for bitfields.

Author: Zonotora

clang/docs/ReleaseNotes.rst

@@ -122,6 +122,26 @@ Non-comprehensive list of changes in this release
 
 New Compiler Flags
 ------------------
+- ``-fsanitize=implicit-unsigned-bitfield-truncation`` catches implicit
+  unsigned conversions involving bitfields.
+- ``-fsanitize=implicit-signed-bitfield-truncation`` catches implicit
+  signed conversions involving bitfields.
+- ``-fsanitize=implicit-bitfield-sign-change`` catches implicit
+  conversions involving bitfields that result in a sign change.
+- ``-fsanitize=implicit-bitfield-truncation`` a group to include both
+  ``-fsanitize=implicit-unsigned-bitfield-truncation`` and
+  ``-fsanitize=implicit-signed-bitfield-truncation``.
+- ``-fsanitize=implicit-bitfield-arithmetic-value-change`` a group to
+  include both ``implicit-signed-bitfield-truncation`` and
+  ``implicit-bitfield-sign-change``.
+- ``-fsanitize=implicit-bitfield-conversion`` a group to include
+  ``-fsanitize=implicit-unsigned-bitfield-truncation``,
+  ``-fsanitize=implicit-signed-bitfield-truncation`` and
+  ``implicit-bitfield-sign-change``.
+- ``-fsanitize=implicit-integer-conversion`` a group to include
+  ``-fsanitize=implicit-unsigned-integer-truncation``,
+  ``-fsanitize=implicit-signed-integer-truncation`` and
+  ``implicit-integer-sign-change``.
 
 .. _target_os_detail:
 
@@ -139,6 +159,11 @@ Deprecated Compiler Flags
 
 Modified Compiler Flags
 -----------------------
+- ``-fsanitize=implicit-conversion`` is now a group for both
+  ``-fsanitize=implicit-integer-conversion`` and
+  ``-fsanitize=implicit-bitfield-conversion``. Hence,
+  ``-fsanitize=implicit-integer-conversion`` has replaced what previously
+  was ``-fsanitize=implicit-conversion``.
 
 Removed Compiler Flags
 -------------------------

clang/docs/UndefinedBehaviorSanitizer.rst

@@ -148,6 +148,23 @@ Available checks are:
      Issues caught by this sanitizer are not undefined behavior,
      but are often unintentional.
   -  ``-fsanitize=integer-divide-by-zero``: Integer division by zero.
+  -  ``-fsanitize=implicit-unsigned-bitfield-truncation``,
+     ``-fsanitize=implicit-signed-bitfield-truncation``: Implicit conversion from
+     integer of larger bit width to smaller bitfield, if that results in data
+     loss. That is, if the demoted value, after casting back to the original
+     width, is not equal to the original value before the downcast.
+     The ``-fsanitize=implicit-unsigned-bitfield-truncation`` handles conversions
+     between two ``unsigned`` types, while
+     ``-fsanitize=implicit-signed-bitfield-truncation`` handles the rest of the
+     conversions - when either one, or both of the types are signed.
+     Issues caught by these sanitizers are not undefined behavior,
+     but are often unintentional.
+  -  ``-fsanitize=implicit-bitfield-sign-change``: Implicit conversion from
+     integer of larger bit width to smaller bitfield, if that changes the
+     sign of the value. That is, if the original value was negative and the
+     new value is positive (or zero), or the original value was positive,
+     and the new value is negative.  Issues caught by this sanitizer are not
+     undefined behavior, but are often unintentional.
   -  ``-fsanitize=nonnull-attribute``: Passing null pointer as a function
      parameter which is declared to never be null.
   -  ``-fsanitize=null``: Use of a null pointer or creation of a null
@@ -192,16 +209,16 @@ Available checks are:
      This includes all the checks covered by ``-ftrapv``, as well as checks for
      signed division overflow (``INT_MIN/-1``), but not checks for
      lossy implicit conversions performed before the computation
-     (see ``-fsanitize=implicit-conversion``). Both of these two issues are
-     handled by ``-fsanitize=implicit-conversion`` group of checks.
+     (see ``-fsanitize=implicit-integer-conversion``). Both of these two issues are
+     handled by ``-fsanitize=implicit-integer-conversion`` group of checks.
   -  ``-fsanitize=unreachable``: If control flow reaches an unreachable
      program point.
   -  ``-fsanitize=unsigned-integer-overflow``: Unsigned integer overflow, where
      the result of an unsigned integer computation cannot be represented in its
      type. Unlike signed integer overflow, this is not undefined behavior, but
      it is often unintentional. This sanitizer does not check for lossy implicit
      conversions performed before such a computation
-     (see ``-fsanitize=implicit-conversion``).
+     (see ``-fsanitize=implicit-integer-conversion``).
   -  ``-fsanitize=vla-bound``: A variable-length array whose bound
      does not evaluate to a positive value.
   -  ``-fsanitize=vptr``: Use of an object whose vptr indicates that it is of
@@ -213,8 +230,9 @@ Available checks are:
 You can also use the following check groups:
   -  ``-fsanitize=undefined``: All of the checks listed above other than
      ``float-divide-by-zero``, ``unsigned-integer-overflow``,
-     ``implicit-conversion``, ``local-bounds`` and the ``nullability-*`` group
-     of checks.
+     ``implicit-integer-conversion``, ``implicit-bitfield-conversion``,
+     ``implicit-conversion``, ``local-bounds`` and the ``nullability-*``
+     group of checks.
   -  ``-fsanitize=undefined-trap``: Deprecated alias of
      ``-fsanitize=undefined``.
   -  ``-fsanitize=implicit-integer-truncation``: Catches lossy integral
@@ -223,11 +241,26 @@ You can also use the following check groups:
   -  ``-fsanitize=implicit-integer-arithmetic-value-change``: Catches implicit
      conversions that change the arithmetic value of the integer. Enables
      ``implicit-signed-integer-truncation`` and ``implicit-integer-sign-change``.
-  -  ``-fsanitize=implicit-conversion``: Checks for suspicious
-     behavior of implicit conversions. Enables
+  -  ``-fsanitize=implicit-integer-conversion``: Checks for suspicious
+     behavior of implicit integer conversions. Enables
      ``implicit-unsigned-integer-truncation``,
      ``implicit-signed-integer-truncation``, and
      ``implicit-integer-sign-change``.
+  -  ``-fsanitize=implicit-bitfield-truncation``: Catches lossy bitfield
+     conversions. Enables ``implicit-signed-bitfield-truncation`` and
+     ``implicit-unsigned-bitfield-truncation``.
+  -  ``-fsanitize=implicit-bitfield-arithmetic-value-change``: Catches implicit
+     conversions that change the arithmetic value of the bitfield. Enables
+     ``implicit-signed-bitfield-truncation`` and ``implicit-bitfield-sign-change``.
+  -  ``-fsanitize=implicit-bitfield-conversion``: Checks for suspicious
+     behavior of implicit bitfield conversions. Enables
+     ``implicit-unsigned-bitfield-truncation``,
+     ``implicit-signed-bitfield-truncation``, and
+     ``implicit-bitfield-sign-change``.
+  -  ``-fsanitize=implicit-conversion``: Checks for suspicious
+     behavior of implicit conversions. Enables
+     ``implicit-integer-conversion``, and
+     ``implicit-bitfield-conversion``.
   -  ``-fsanitize=integer``: Checks for undefined or suspicious integer
      behavior (e.g. unsigned integer overflow).
      Enables ``signed-integer-overflow``, ``unsigned-integer-overflow``,

clang/include/clang/Basic/Sanitizers.def

@@ -163,24 +163,38 @@ SANITIZER_GROUP("implicit-integer-arithmetic-value-change",
                 ImplicitIntegerArithmeticValueChange,
                 ImplicitIntegerSignChange | ImplicitSignedIntegerTruncation)
 
-SANITIZER("objc-cast", ObjCCast)
+SANITIZER_GROUP("implicit-integer-conversion", ImplicitIntegerConversion,
+                ImplicitIntegerArithmeticValueChange |
+                    ImplicitUnsignedIntegerTruncation)
+
+// Implicit bitfield sanitizers
+SANITIZER("implicit-unsigned-bitfield-truncation", ImplicitUnsignedBitfieldTruncation)
+SANITIZER("implicit-signed-bitfield-truncation", ImplicitSignedBitfieldTruncation)
+SANITIZER_GROUP("implicit-bitfield-truncation", ImplicitBitfieldTruncation,
+                ImplicitUnsignedBitfieldTruncation |
+                    ImplicitSignedBitfieldTruncation)
+
+SANITIZER("implicit-bitfield-sign-change", ImplicitBitfieldSignChange)
 
-// FIXME:
-//SANITIZER_GROUP("implicit-integer-conversion", ImplicitIntegerConversion,
-//                ImplicitIntegerArithmeticValueChange |
-//                    ImplicitUnsignedIntegerTruncation)
-//SANITIZER_GROUP("implicit-conversion", ImplicitConversion,
-//                ImplicitIntegerConversion)
+SANITIZER_GROUP("implicit-bitfield-arithmetic-value-change",
+                ImplicitBitfieldArithmeticValueChange,
+                ImplicitBitfieldSignChange | ImplicitSignedBitfieldTruncation)
+
+SANITIZER_GROUP("implicit-bitfield-conversion", ImplicitBitfieldConversion,
+                ImplicitBitfieldArithmeticValueChange |
+                  ImplicitUnsignedBitfieldTruncation)
 
 SANITIZER_GROUP("implicit-conversion", ImplicitConversion,
-                ImplicitIntegerArithmeticValueChange |
-                    ImplicitUnsignedIntegerTruncation)
+                ImplicitIntegerConversion |
+                    ImplicitBitfieldConversion)
 
 SANITIZER_GROUP("integer", Integer,
-                ImplicitConversion | IntegerDivideByZero | Shift |
+                ImplicitIntegerConversion | IntegerDivideByZero | Shift |
                     SignedIntegerOverflow | UnsignedIntegerOverflow |
                     UnsignedShiftBase)
 
+SANITIZER("objc-cast", ObjCCast)
+
 SANITIZER("local-bounds", LocalBounds)
 SANITIZER_GROUP("bounds", Bounds, ArrayBounds | LocalBounds)
 

clang/lib/CodeGen/CGExpr.cpp

@@ -5570,11 +5570,44 @@ LValue CodeGenFunction::EmitBinaryOperatorLValue(const BinaryOperator *E) {
       break;
     }
 
-    RValue RV = EmitAnyExpr(E->getRHS());
+    llvm::Value *Previous = nullptr;
+    RValue RV;
+    QualType SrcType = E->getRHS()->getType();
+    // If LHS refers to a bitfield we want to retrieve the value before
+    // implicit conversion between the bitfield type and the RHS type
+    // and evaluate RHS without integer sanitizer checks (if passed)
+    if (auto *ICE = RetrieveImplicitCastExprForBitfieldSanitizer(E)) {
+      SrcType = ICE->getSubExpr()->getType();
+      Previous = EmitScalarExpr(ICE->getSubExpr());
+      // Pass default ScalarConversionOpts so that sanitizer check is
+      // not emitted here
+      llvm::Value *RHS = EmitScalarConversion(Previous, SrcType, ICE->getType(),
+                                              ICE->getExprLoc());
+      RV = RValue::get(RHS);
+    }
+
+    if (!Previous)
+      RV = EmitAnyExpr(E->getRHS());
+
     LValue LV = EmitCheckedLValue(E->getLHS(), TCK_Store);
+
     if (RV.isScalar())
       EmitNullabilityCheck(LV, RV.getScalarVal(), E->getExprLoc());
-    EmitStoreThroughLValue(RV, LV);
+
+    // Passing a pointer EmitStoreThroughBitfieldLValue will emit the result
+    // If sanitizer checks are not used, we do not use the result
+    // Hence, use EmitStoreThroughLValue instead
+    if (SanOpts.hasOneOf(SanitizerKind::ImplicitBitfieldConversion) &&
+        LV.isBitField() && RV.isScalar()) {
+      llvm::Value *Src = Previous ? Previous : RV.getScalarVal();
+      QualType DstType = E->getLHS()->getType();
+      llvm::Value *Result;
+      EmitStoreThroughBitfieldLValue(RV, LV, &Result);
+      EmitBitfieldConversionCheck(Src, SrcType, Result, DstType,
+                                  LV.getBitFieldInfo(), E->getExprLoc());
+    } else
+      EmitStoreThroughLValue(RV, LV);
+
     if (getLangOpts().OpenMP)
       CGM.getOpenMPRuntime().checkAndEmitLastprivateConditional(*this,
                                                                 E->getLHS());