llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Axel Lundberg (Zonotora) <details> <summary>Changes</summary> Fix since #<!-- -->75481 got reverted. - Explicitly set BitfieldBits to 0 to avoid uninitialized field member for the integer checks: ```diff - llvm::ConstantInt::get(Builder.getInt8Ty(), Check.first)}; + llvm::ConstantInt::get(Builder.getInt8Ty(), Check.first), + llvm::ConstantInt::get(Builder.getInt32Ty(), 0)}; ``` - `Value **Previous` was erroneously `Value *Previous` in `CodeGenFunction::EmitWithOriginalRHSBitfieldAssignment`, fixed now. - Assert instead of check: ```diff - if (Kind == CK_IntegralCast) { + assert(Kind == CK_IntegralCast || Kind == CK_LValueToRValue && + "Should be either CK_IntegralCast or CK_LValueToRValue"); ``` CK_LValueToRValue when going from, e.g., char to char, and CK_IntegralCast otherwise. - Make sure that `Value *Previous = nullptr;` is initialized (see https://github.com/llvm/llvm-project/commit/1189e87951e59a81ee097eae847c06008276fef1) - Add another extensive testcase `ubsan/TestCases/ImplicitConversion/bitfield-conversion.c` --- Patch is 72.25 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/87761.diff 12 Files Affected: - (modified) clang/docs/ReleaseNotes.rst (+7) - (modified) clang/docs/UndefinedBehaviorSanitizer.rst (+14-5) - (modified) clang/include/clang/Basic/Sanitizers.def (+10-10) - (modified) clang/lib/CodeGen/CGExpr.cpp (+35-2) - (modified) clang/lib/CodeGen/CGExprScalar.cpp (+230-34) - (modified) clang/lib/CodeGen/CodeGenFunction.h (+15) - (added) clang/test/CodeGen/ubsan-bitfield-conversion.c (+61) - (added) clang/test/CodeGenCXX/ubsan-bitfield-conversion.cpp (+94) - (modified) clang/test/Driver/fsanitize.c (+14-14) - (modified) compiler-rt/lib/ubsan/ubsan_handlers.cpp (+17-10) - (modified) compiler-rt/lib/ubsan/ubsan_handlers.h (+1) - (added) compiler-rt/test/ubsan/TestCases/ImplicitConversion/bitfield-conversion.c (+641) ``````````diff diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 28e8ddb3c41c3e9..abf15c8dc49d9e7 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -198,6 +198,10 @@ Non-comprehensive list of changes in this release New Compiler Flags ------------------ +- ``-fsanitize=implicit-bitfield-conversion`` checks implicit truncation and + sign change. +- ``-fsanitize=implicit-integer-conversion`` a group that replaces the previous + group ``-fsanitize=implicit-conversion``. - ``-Wmissing-designated-field-initializers``, grouped under ``-Wmissing-field-initializers``. This diagnostic can be disabled to make ``-Wmissing-field-initializers`` behave @@ -211,6 +215,9 @@ Modified Compiler Flags - Added a new diagnostic flag ``-Wreturn-mismatch`` which is grouped under ``-Wreturn-type``, and moved some of the diagnostics previously controlled by ``-Wreturn-type`` under this new flag. Fixes #GH72116. +- ``-fsanitize=implicit-conversion`` is now a group for both + ``-fsanitize=implicit-integer-conversion`` and + ``-fsanitize=implicit-bitfield-conversion``. - Added ``-Wcast-function-type-mismatch`` under the ``-Wcast-function-type`` warning group. Moved the diagnostic previously controlled by diff --git a/clang/docs/UndefinedBehaviorSanitizer.rst b/clang/docs/UndefinedBehaviorSanitizer.rst index 8f58c92bd2a1634..531d56e313826c7 100644 --- a/clang/docs/UndefinedBehaviorSanitizer.rst +++ b/clang/docs/UndefinedBehaviorSanitizer.rst @@ -148,6 +148,11 @@ 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-bitfield-conversion``: Implicit conversion from + integer of larger bit width to smaller bitfield, if that results in data + loss. This includes unsigned/signed truncations and sign changes, similarly + to how the ``-fsanitize=implicit-integer-conversion`` group works, but + explicitly for bitfields. - ``-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 @@ -193,8 +198,8 @@ Available checks are: signed division overflow (``INT_MIN/-1``). Note that checks are still added even when ``-fwrapv`` is enabled. This sanitizer does not check 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. + ``-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 @@ -202,7 +207,7 @@ Available checks are: 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 @@ -224,11 +229,15 @@ 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-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``, diff --git a/clang/include/clang/Basic/Sanitizers.def b/clang/include/clang/Basic/Sanitizers.def index c2137e3f61f6450..b228ffd07ee7454 100644 --- a/clang/include/clang/Basic/Sanitizers.def +++ b/clang/include/clang/Basic/Sanitizers.def @@ -163,24 +163,24 @@ SANITIZER_GROUP("implicit-integer-arithmetic-value-change", ImplicitIntegerArithmeticValueChange, ImplicitIntegerSignChange | ImplicitSignedIntegerTruncation) -SANITIZER("objc-cast", ObjCCast) +SANITIZER_GROUP("implicit-integer-conversion", ImplicitIntegerConversion, + ImplicitIntegerArithmeticValueChange | + ImplicitUnsignedIntegerTruncation) -// FIXME: -//SANITIZER_GROUP("implicit-integer-conversion", ImplicitIntegerConversion, -// ImplicitIntegerArithmeticValueChange | -// ImplicitUnsignedIntegerTruncation) -//SANITIZER_GROUP("implicit-conversion", ImplicitConversion, -// ImplicitIntegerConversion) +// Implicit bitfield sanitizers +SANITIZER("implicit-bitfield-conversion", ImplicitBitfieldConversion) 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) diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp index 54432353e7420d1..02dc2cfce00d820 100644 --- a/clang/lib/CodeGen/CGExpr.cpp +++ b/clang/lib/CodeGen/CGExpr.cpp @@ -5580,11 +5580,44 @@ LValue CodeGenFunction::EmitBinaryOperatorLValue(const BinaryOperator *E) { break; } - RValue RV = EmitAnyExpr(E->getRHS()); + // TODO: Can we de-duplicate this code with the corresponding code in + // CGExprScalar, similar to the way EmitCompoundAssignmentLValue works? + RValue RV; + llvm::Value *Previous = nullptr; + QualType SrcType = E->getRHS()->getType(); + // Check if LHS is a bitfield, if RHS contains an implicit cast expression + // we want to extract that value and potentially (if the bitfield sanitizer + // is enabled) use it to check for an implicit conversion. + if (E->getLHS()->refersToBitField()) { + llvm::Value *RHS = + EmitWithOriginalRHSBitfieldAssignment(E, &Previous, &SrcType); + RV = RValue::get(RHS); + } else + RV = EmitAnyExpr(E->getRHS()); + LValue LV = EmitCheckedLValue(E->getLHS(), TCK_Store); + if (RV.isScalar()) EmitNullabilityCheck(LV, RV.getScalarVal(), E->getExprLoc()); - EmitStoreThroughLValue(RV, LV); + + if (LV.isBitField()) { + llvm::Value *Result = nullptr; + // If bitfield sanitizers are enabled we want to use the result + // to check whether a truncation or sign change has occurred. + if (SanOpts.has(SanitizerKind::ImplicitBitfieldConversion)) + EmitStoreThroughBitfieldLValue(RV, LV, &Result); + else + EmitStoreThroughBitfieldLValue(RV, LV); + + // If the expression contained an implicit conversion, make sure + // to use the value before the scalar conversion. + llvm::Value *Src = Previous ? Previous : RV.getScalarVal(); + QualType DstType = E->getLHS()->getType(); + EmitBitfieldConversionCheck(Src, SrcType, Result, DstType, + LV.getBitFieldInfo(), E->getExprLoc()); + } else + EmitStoreThroughLValue(RV, LV); + if (getLangOpts().OpenMP) CGM.getOpenMPRuntime().checkAndEmitLastprivateConditional(*this, E->getLHS()); diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp index 397b4977acc3e9d..b6bae4f83247244 100644 --- a/clang/lib/CodeGen/CGExprScalar.cpp +++ b/clang/lib/CodeGen/CGExprScalar.cpp @@ -15,6 +15,7 @@ #include "CGDebugInfo.h" #include "CGObjCRuntime.h" #include "CGOpenMPRuntime.h" +#include "CGRecordLayout.h" #include "CodeGenFunction.h" #include "CodeGenModule.h" #include "ConstantEmitter.h" @@ -308,6 +309,7 @@ class ScalarExprEmitter llvm::Type *DstTy, SourceLocation Loc); /// Known implicit conversion check kinds. + /// This is used for bitfield conversion checks as well. /// Keep in sync with the enum of the same name in ubsan_handlers.h enum ImplicitConversionCheckKind : unsigned char { ICCK_IntegerTruncation = 0, // Legacy, was only used by clang 7. @@ -1098,11 +1100,28 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value *Src, QualType SrcType, llvm::Constant *StaticArgs[] = { CGF.EmitCheckSourceLocation(Loc), CGF.EmitCheckTypeDescriptor(SrcType), CGF.EmitCheckTypeDescriptor(DstType), - llvm::ConstantInt::get(Builder.getInt8Ty(), Check.first)}; + llvm::ConstantInt::get(Builder.getInt8Ty(), Check.first), + llvm::ConstantInt::get(Builder.getInt32Ty(), 0)}; + CGF.EmitCheck(Check.second, SanitizerHandler::ImplicitConversion, StaticArgs, {Src, Dst}); } +static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType, + const char *Name, + CGBuilderTy &Builder) { + bool VSigned = VType->isSignedIntegerOrEnumerationType(); + llvm::Type *VTy = V->getType(); + if (!VSigned) { + // If the value is unsigned, then it is never negative. + return llvm::ConstantInt::getFalse(VTy->getContext()); + } + llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0); + return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero, + llvm::Twine(Name) + "." + V->getName() + + ".negativitycheck"); +} + // Should be called within CodeGenFunction::SanitizerScope RAII scope. // Returns 'i1 false' when the conversion Src -> Dst changed the sign. static std::pair<ScalarExprEmitter::ImplicitConversionCheckKind, @@ -1127,30 +1146,12 @@ EmitIntegerSignChangeCheckHelper(Value *Src, QualType SrcType, Value *Dst, assert(((SrcBits != DstBits) || (SrcSigned != DstSigned)) && "either the widths should be different, or the signednesses."); - // NOTE: zero value is considered to be non-negative. - auto EmitIsNegativeTest = [&Builder](Value *V, QualType VType, - const char *Name) -> Value * { - // Is this value a signed type? - bool VSigned = VType->isSignedIntegerOrEnumerationType(); - llvm::Type *VTy = V->getType(); - if (!VSigned) { - // If the value is unsigned, then it is never negative. - // FIXME: can we encounter non-scalar VTy here? - return llvm::ConstantInt::getFalse(VTy->getContext()); - } - // Get the zero of the same type with which we will be comparing. - llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0); - // %V.isnegative = icmp slt %V, 0 - // I.e is %V *strictly* less than zero, does it have negative value? - return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero, - llvm::Twine(Name) + "." + V->getName() + - ".negativitycheck"); - }; - // 1. Was the old Value negative? - llvm::Value *SrcIsNegative = EmitIsNegativeTest(Src, SrcType, "src"); + llvm::Value *SrcIsNegative = + EmitIsNegativeTestHelper(Src, SrcType, "src", Builder); // 2. Is the new Value negative? - llvm::Value *DstIsNegative = EmitIsNegativeTest(Dst, DstType, "dst"); + llvm::Value *DstIsNegative = + EmitIsNegativeTestHelper(Dst, DstType, "dst", Builder); // 3. Now, was the 'negativity status' preserved during the conversion? // NOTE: conversion from negative to zero is considered to change the sign. // (We want to get 'false' when the conversion changed the sign) @@ -1239,12 +1240,143 @@ void ScalarExprEmitter::EmitIntegerSignChangeCheck(Value *Src, QualType SrcType, llvm::Constant *StaticArgs[] = { CGF.EmitCheckSourceLocation(Loc), CGF.EmitCheckTypeDescriptor(SrcType), CGF.EmitCheckTypeDescriptor(DstType), - llvm::ConstantInt::get(Builder.getInt8Ty(), CheckKind)}; + llvm::ConstantInt::get(Builder.getInt8Ty(), CheckKind), + llvm::ConstantInt::get(Builder.getInt32Ty(), 0)}; // EmitCheck() will 'and' all the checks together. CGF.EmitCheck(Checks, SanitizerHandler::ImplicitConversion, StaticArgs, {Src, Dst}); } +// Should be called within CodeGenFunction::SanitizerScope RAII scope. +// Returns 'i1 false' when the truncation Src -> Dst was lossy. +static std::pair<ScalarExprEmitter::ImplicitConversionCheckKind, + std::pair<llvm::Value *, SanitizerMask>> +EmitBitfieldTruncationCheckHelper(Value *Src, QualType SrcType, Value *Dst, + QualType DstType, CGBuilderTy &Builder) { + bool SrcSigned = SrcType->isSignedIntegerOrEnumerationType(); + bool DstSigned = DstType->isSignedIntegerOrEnumerationType(); + + ScalarExprEmitter::ImplicitConversionCheckKind Kind; + if (!SrcSigned && !DstSigned) + Kind = ScalarExprEmitter::ICCK_UnsignedIntegerTruncation; + else + Kind = ScalarExprEmitter::ICCK_SignedIntegerTruncation; + + llvm::Value *Check = nullptr; + // 1. Extend the truncated value back to the same width as the Src. + Check = Builder.CreateIntCast(Dst, Src->getType(), DstSigned, "bf.anyext"); + // 2. Equality-compare with the original source value + Check = Builder.CreateICmpEQ(Check, Src, "bf.truncheck"); + // If the comparison result is 'i1 false', then the truncation was lossy. + + return std::make_pair( + Kind, std::make_pair(Check, SanitizerKind::ImplicitBitfieldConversion)); +} + +// Should be called within CodeGenFunction::SanitizerScope RAII scope. +// Returns 'i1 false' when the conversion Src -> Dst changed the sign. +static std::pair<ScalarExprEmitter::ImplicitConversionCheckKind, + std::pair<llvm::Value *, SanitizerMask>> +EmitBitfieldSignChangeCheckHelper(Value *Src, QualType SrcType, Value *Dst, + QualType DstType, CGBuilderTy &Builder) { + // 1. Was the old Value negative? + llvm::Value *SrcIsNegative = + EmitIsNegativeTestHelper(Src, SrcType, "bf.src", Builder); + // 2. Is the new Value negative? + llvm::Value *DstIsNegative = + EmitIsNegativeTestHelper(Dst, DstType, "bf.dst", Builder); + // 3. Now, was the 'negativity status' preserved during the conversion? + // NOTE: conversion from negative to zero is considered to change the sign. + // (We want to get 'false' when the conversion changed the sign) + // So we should just equality-compare the negativity statuses. + llvm::Value *Check = nullptr; + Check = + Builder.CreateICmpEQ(SrcIsNegative, DstIsNegative, "bf.signchangecheck"); + // If the comparison result is 'false', then the conversion changed the sign. + return std::make_pair( + ScalarExprEmitter::ICCK_IntegerSignChange, + std::make_pair(Check, SanitizerKind::ImplicitBitfieldConversion)); +} + +void CodeGenFunction::EmitBitfieldConversionCheck(Value *Src, QualType SrcType, + Value *Dst, QualType DstType, + const CGBitFieldInfo &Info, + SourceLocation Loc) { + + if (!SanOpts.has(SanitizerKind::ImplicitBitfieldConversion)) + return; + + // We only care about int->int conversions here. + // We ignore conversions to/from pointer and/or bool. + if (!PromotionIsPotentiallyEligibleForImplicitIntegerConversionCheck(SrcType, + DstType)) + return; + + if (DstType->isBooleanType() || SrcType->isBooleanType()) + return; + + // This should be truncation of integral types. + assert(isa<llvm::IntegerType>(Src->getType()) && + isa<llvm::IntegerType>(Dst->getType()) && "non-integer llvm type"); + + // TODO: Calculate src width to avoid emitting code + // for unecessary cases. + unsigned SrcBits = ConvertType(SrcType)->getScalarSizeInBits(); + unsigned DstBits = Info.Size; + + bool SrcSigned = SrcType->isSignedIntegerOrEnumerationType(); + bool DstSigned = DstType->isSignedIntegerOrEnumerationType(); + + CodeGenFunction::SanitizerScope SanScope(this); + + std::pair<ScalarExprEmitter::ImplicitConversionCheckKind, + std::pair<llvm::Value *, SanitizerMask>> + Check; + + // Truncation + bool EmitTruncation = DstBits < SrcBits; + // If Dst is signed and Src unsigned, we want to be more specific + // about the CheckKind we emit, in this case we want to emit + // ICCK_SignedIntegerTruncationOrSignChange. + bool EmitTruncationFromUnsignedToSigned = + EmitTruncation && DstSigned && !SrcSigned; + // Sign change + bool SameTypeSameSize = SrcSigned == DstSigned && SrcBits == DstBits; + bool BothUnsigned = !SrcSigned && !DstSigned; + bool LargerSigned = (DstBits > SrcBits) && DstSigned; + // We can avoid emitting sign change checks in some obvious cases + // 1. If Src and Dst have the same signedness and size + // 2. If both are unsigned sign check is unecessary! + // 3. If Dst is signed and bigger than Src, either + // sign-extension or zero-extension will make sure + // the sign remains. + bool EmitSignChange = !SameTypeSameSize && !BothUnsigned && !LargerSigned; + + if (EmitTruncation) + Check = + EmitBitfieldTruncationCheckHelper(Src, SrcType, Dst, DstType, Builder); + else if (EmitSignChange) { + assert(((SrcBits != DstBits) || (SrcSigned != DstSigned)) && + "either the widths should be different, or the signednesses."); + Check = + EmitBitfieldSignChangeCheckHelper(Src, SrcType, Dst, DstType, Builder); + } else + return; + + ScalarExprEmitter::ImplicitConversionCheckKind CheckKind = Check.first; + if (EmitTruncationFromUnsignedToSigned) + CheckKind = ScalarExprEmitter::ICCK_SignedIntegerTruncationOrSignChange; + + llvm::Constant *StaticArgs[] = { + EmitCheckSourceLocation(Loc), EmitCheckTypeDescriptor(SrcType), + EmitCheckTypeDescriptor(DstType), + llvm::ConstantInt::get(Builder.getInt8Ty(), CheckKind), + llvm::ConstantInt::get(Builder.getInt32Ty(), Info.Size)}; + + EmitCheck(Check.second, SanitizerHandler::ImplicitConversion, StaticArgs, + {Src, Dst}); +} + Value *ScalarExprEmitter::EmitScalarCast(Value *Src, QualType SrcType, QualType DstType, llvm::Type *SrcTy, llvm::Type *DstTy, @@ -2620,6 +2752,8 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV, llvm::PHINode *atomicPHI = nullptr; llvm::Value *value; llvm::Value *input; + llvm::Value *Previous = nullptr; + QualType SrcType = E->getType(); int amount = (isInc ? 1 : -1); bool isSubtraction = !isInc; @@ -2708,7 +2842,8 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV, "base or promoted) will be signed, or the bitwidths will match."); } if (CGF.SanOpts.hasOneOf( - SanitizerKind::ImplicitIntegerArithmeticValueChange) && + SanitizerKind::ImplicitIntegerArithmeticValueChange | + SanitizerKind::ImplicitBitfieldConversion) && canPerformLossyDemotionCh... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/87761 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits