Revert "Revert "[clang][UBSan] Add implicit conversion check for bitfields"" #87529
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ields" (…" This reverts commit 5822ca5.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Vitaly Buka (vitalybuka) ChangesReverts llvm/llvm-project#87518 Revert is not needed as regression was fixed with 1189e87. Patch is 43.31 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/87529.diff 11 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8fc925350849cd..e4c0e49ed6fc1c 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 8f58c92bd2a163..531d56e313826c 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 c2137e3f61f645..b228ffd07ee745 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 54432353e7420d..0c7f48fe00603d 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 397b4977acc3e9..a4ab8a11b867fa 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.
@@ -1103,6 +1105,21 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value *Src, QualType SrcType,
{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 +1144,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)
@@ -1245,6 +1244,136 @@ void ScalarExprEmitter::EmitIntegerSignChangeCheck(Value *Src, QualType SrcType,
{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 +2749,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 +2839,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) &&
canPerformLossyDemotionCheck) {
// While `x += 1` (for `x` with width less than int) is modeled as
// promotion+arithmetics+demotion, and we can catch lossy demotion with
@@ -2719,13 +2851,26 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV,
// the increment/decrement in the wider type, and finally
// perform the demotion. This will catch lossy demotions.
+ // We have a special case for bitfields defined using all the bits of the
+ // type. In this case we need to do the same trick as for the integer
+ // sanitizer checks, i.e., promotion -> increment/decrement -> demotion.
+
value = EmitScalarConversion(value, type, promotedType, E->getExprLoc());
Value *amt = llvm::ConstantInt::get(value->getType(), amount, true);
value = Builder.CreateAdd(value, amt, isInc ? "inc" : "dec");
// Do pass non-default ScalarConversionOpts so tha...
[truncated]
|
vitalybuka
added a commit
that referenced
this pull request
Apr 3, 2024
…for bitfields""" (#87562) Reverts #87529 Reverts #87518 https://lab.llvm.org/buildbot/#/builders/37/builds/33262 is still broken
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts #87518
Revert is not needed as the regression was fixed with 1189e87.
I assumed the crash and warning are different issues, but according to
https://lab.llvm.org/buildbot/#/builders/240/builds/26629
fixing warning resolves the crash.