Fix "[clang][UBSan] Add implicit conversion check for bitfields" #87761
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
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Axel Lundberg (Zonotora) ChangesFix since #75481 got reverted.
- llvm::ConstantInt::get(Builder.getInt8Ty(), Check.first)};
+ llvm::ConstantInt::get(Builder.getInt8Ty(), Check.first),
+ llvm::ConstantInt::get(Builder.getInt32Ty(), 0)};
- 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.
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:
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]
|
Zonotora
force-pushed
the
ubsan-bitfield-conversion-explicit-pass
branch
2 times, most recently
from
a2a1ba3 to
6a4280f
Compare
|
This is fixed version of #75481 ? |
Zonotora
force-pushed
the
ubsan-bitfield-conversion-explicit-pass
branch
from
6a4280f to
bb2d48e
Compare
Zonotora
force-pushed
the
ubsan-bitfield-conversion-explicit-pass
branch
from
bb2d48e to
60e163b
Compare
|
@vitalybuka yes, seems like all testcases failed due to not explicitly setting BitfieldBits to 0 except one (https://lab.llvm.org/buildbot/#/builders/37/builds/33262), |
|
@vitalybuka all good? |
vitalybuka
reviewed
Apr 8, 2024
vitalybuka
approved these changes
Apr 8, 2024
|
I'll land after presubmit checks |
|
@vitalybuka Well, this is awkward, fails due to: https://lab.llvm.org/buildbot/#/builders/127/builds/64260 https://lab.llvm.org/buildbot/#/builders/72/builds/3597 Some other testcases failing as well. Seems to be flaky, no? Will you patch this? I think you are in a better position to prevent the testcases from failing! |
|
ok |
vitalybuka
added a commit
that referenced
this pull request
Apr 8, 2024
vitalybuka
added a commit
that referenced
this pull request
Apr 8, 2024
haonanya
reviewed
Apr 9, 2024
| // CHECK-BITFIELD-CONVERSION-NEXT: [[BFRESULTASHR:%.*]] = ashr i8 [[BFRESULTSHL]], 5 | ||
| // CHECK-BITFIELD-CONVERSION-NEXT: [[BFRESULTCAST:%.*]] = sext i8 [[BFRESULTASHR]] to i32 | ||
| // CHECK-BITFIELD-CONVERSION: call void @__ubsan_handle_implicit_conversion | ||
| // CHECK-BITFIELD-CONVERSION-NEXT: br label %[[CONT:.*]], !nosanitize !6 |
There was a problem hiding this comment.
@Zonotora Is the check "!nosanitize !6" necessary? Are we sure it's always !6? Same comments for ubsan-bitfield-conversion.c. Thanks very much.
There was a problem hiding this comment.
No, feel free to update
There was a problem hiding this comment.
@Zonotora
Q1: Is checking !nosanitize necessary?
Q2: Is checking !6 necessary?
Regarding the metadata, it's not always !6 (at least for us).
So, if we could simply remove at least this metadata check !6 (or even both, if possible), it would probably solve the check-clang failures we are encountering.
Also, when looking at the output, the referred metadata is empty, in my case it was !4, with !4 = !{}.
mhalk
added a commit
to mhalk/llvm-project
that referenced
this pull request
Apr 9, 2024
…rsion` Follow-up to: llvm#87761 As discussed after landing the original PR: Since fails could happen w.r.t. checking `!6`, these checks will be removed.
chapuni
added a commit
that referenced
this pull request
Apr 10, 2024
It has been incompatible since #87761
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.
Fix since #75481 got reverted.
Value **Previouswas erroneouslyValue *PreviousinCodeGenFunction::EmitWithOriginalRHSBitfieldAssignment, fixed now.CK_LValueToRValue when going from, e.g., char to char, and CK_IntegralCast otherwise.
Value *Previous = nullptr;is initialized (see 1189e87)ubsan/TestCases/ImplicitConversion/bitfield-conversion.c