r178273 - Implemented a warning when an input several bitwise operations are
Timur Iskhodzhanov
timurrrr at google.com
Thu Mar 28 17:23:13 PDT 2013
Reverted as r178320.
2013/3/28 Sam Panzer <espanz at gmail.com>:
> Probably revert the change for now, once we note the false positives. I'm
> working on a fix.
>
> On Mar 28, 2013 5:04 PM, "Richard Smith" <richard at metafoo.co.uk> wrote:
>>
>> On Thu, Mar 28, 2013 at 4:53 PM, Timur Iskhodzhanov <timurrrr at google.com>
>> wrote:
>>>
>>> Hi Sam,
>>>
>>> It looks like this change has uncovered some problems in Clang.
>>>
>>> I see a lot of compile-time errors on Linux like the following when
>>> building Clang with Clang.
>>> In file included from lib/Target/PowerPC/PPCJITInfo.cpp:17:
>>> In file included from lib/Target/PowerPC/PPCTargetMachine.h:18:
>>> In file included from /lib/Target/PowerPC/PPCISelLowering.h:22:
>>> /include/llvm/Target/TargetLowering.h:1012:59: error: implicit
>>> conversion from 'int' to 'uint8_t' (aka 'unsigned char') changes value
>>> from -241 to 15 [-Werror,-Wconstant-conversion]
>>> IndexedModeActions[(unsigned)VT.SimpleTy][IdxMode] &= ~0xf0;
>>
>>
>> Hm, we should probably allow bitwise &, |, and ^ if the constant operand's
>> ignored bits are either all-0 or all-1.
>>
>>>
>>> which look related to your recent commits.
>>>
>>> Can you please take a look?
>>> It makes some buildbots red...
>>>
>>>
>>> 2013/3/28 Sam Panzer <espanz at gmail.com>:
>>> > Author: panzer
>>> > Date: Thu Mar 28 14:07:11 2013
>>> > New Revision: 178273
>>> >
>>> > URL: http://llvm.org/viewvc/llvm-project?rev=178273&view=rev
>>> > Log:
>>> > Implemented a warning when an input several bitwise operations are
>>> > likely be implicitly truncated:
>>> >
>>> > * All forms of Bitwise-and, bitwise-or, and integer multiplication.
>>> > * The assignment form of integer addition, subtraction, and
>>> > exclusive-or
>>> > * The RHS of the comma operator
>>> > * The LHS of left shifts.
>>> >
>>> > Modified:
>>> > cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>> > cfe/trunk/lib/Sema/SemaChecking.cpp
>>> > cfe/trunk/test/Sema/constant-conversion.c
>>> >
>>> > Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>> > URL:
>>> > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=178273&r1=178272&r2=178273&view=diff
>>> >
>>> > ==============================================================================
>>> > --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>>> > +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Mar 28
>>> > 14:07:11 2013
>>> > @@ -2084,6 +2084,11 @@ def warn_impcast_integer_64_32 : Warning
>>> > def warn_impcast_integer_precision_constant : Warning<
>>> > "implicit conversion from %2 to %3 changes value from %0 to %1">,
>>> > InGroup<ConstantConversion>;
>>> > +def warn_impcast_integer_precision_binop_constant : Warning<
>>> > + "implicit conversion of binary operation from %2 to %3 may change
>>> > its value; "
>>> > + "value of operand would be changed from %0 to %1 if converted before
>>> > "
>>> > + "operation">,
>>> > + InGroup<ConstantConversion>;
>>> > def warn_impcast_bitfield_precision_constant : Warning<
>>> > "implicit truncation from %2 to bitfield changes value from %0 to
>>> > %1">,
>>> > InGroup<ConstantConversion>;
>>> >
>>> > Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>>> > URL:
>>> > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=178273&r1=178272&r2=178273&view=diff
>>> >
>>> > ==============================================================================
>>> > --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>>> > +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Mar 28 14:07:11 2013
>>> > @@ -4686,7 +4686,8 @@ static void AnalyzeAssignment(Sema &S, B
>>> > // We want to recurse on the RHS as normal unless we're assigning to
>>> > // a bitfield.
>>> > if (FieldDecl *Bitfield = E->getLHS()->getBitField()) {
>>> > - if (AnalyzeBitFieldAssignment(S, Bitfield, E->getRHS(),
>>> > + if (E->getOpcode() != BO_AndAssign &&
>>> > + AnalyzeBitFieldAssignment(S, Bitfield, E->getRHS(),
>>> > E->getOperatorLoc())) {
>>> > // Recurse, ignoring any implicit conversions on the RHS.
>>> > return AnalyzeImplicitConversions(S,
>>> > E->getRHS()->IgnoreParenImpCasts(),
>>> > @@ -4795,8 +4796,82 @@ void CheckImplicitArgumentConversions(Se
>>> > }
>>> > }
>>> >
>>> > +// Try to evaluate E as an integer. If EvaluateAsInt succeeds, Value
>>> > is set to
>>> > +// the resulting value, ResultExpr is set to E.
>>> > +// Otherwise, the output parameters are not modified.
>>> > +static bool EvalAsInt(ASTContext &Ctx, llvm::APSInt &Value, Expr *E,
>>> > + Expr **ResultExpr) {
>>> > + if (E->EvaluateAsInt(Value, Ctx, Expr::SE_AllowSideEffects)) {
>>> > + *ResultExpr = E;
>>> > + return true;
>>> > + }
>>> > + return false;
>>> > +}
>>> > +
>>> > +// Returns true when Value's value would change when narrowed to
>>> > TargetRange.
>>> > +static bool TruncationChangesValue(const llvm::APSInt &Value,
>>> > + const IntRange &TargetRange,
>>> > + bool IsBitwiseAnd) {
>>> > + // Checks if there are any active non-sign bits past the width of
>>> > TargetRange.
>>> > + if (IsBitwiseAnd)
>>> > + return Value.getBitWidth() - Value.getNumSignBits() >
>>> > TargetRange.Width;
>>> > +
>>> > + llvm::APSInt UnsignedValue(Value);
>>> > + unsigned BitWidth = TargetRange.NonNegative ?
>>> > + UnsignedValue.getActiveBits() : Value.getMinSignedBits();
>>> > + return BitWidth > TargetRange.Width;
>>> > +}
>>> > +
>>> > +// Determine if E is an expression containing or likely to contain an
>>> > implicit
>>> > +// narrowing bug involving a constant.
>>> > +// If so, Value is set to the value of that constant. NarrowedExpr is
>>> > set to the
>>> > +// problematic sub-expression if it is a strict subset of E.
>>> > +static bool OperandMightImplicitlyNarrow(ASTContext &Ctx, llvm::APSInt
>>> > &Value,
>>> > + Expr *E, IntRange
>>> > TargetRange,
>>> > + Expr **NarrowedExpr) {
>>> > + BinaryOperator *BO = dyn_cast<BinaryOperator>(E);
>>> > + if (!BO)
>>> > + return false;
>>> > +
>>> > + switch (BO->getOpcode()) {
>>> > + case BO_And:
>>> > + case BO_AndAssign:
>>> > + return (EvalAsInt(Ctx, Value, BO->getLHS(), NarrowedExpr)
>>> > + && TruncationChangesValue(Value, TargetRange, true)) ||
>>> > + (EvalAsInt(Ctx, Value, BO->getRHS(), NarrowedExpr)
>>> > + && TruncationChangesValue(Value, TargetRange, true));
>>> > +
>>> > + case BO_Or:
>>> > + case BO_OrAssign:
>>> > + // FIXME: Include BO_Add, BO_Sub, and BO_Xor when we avoid false
>>> > positives.
>>> > + case BO_AddAssign:
>>> > + case BO_SubAssign:
>>> > + case BO_Mul:
>>> > + case BO_MulAssign:
>>> > + case BO_XorAssign:
>>> > + return (EvalAsInt(Ctx, Value, BO->getLHS(), NarrowedExpr)
>>> > + && TruncationChangesValue(Value, TargetRange, false))
>>> > ||
>>> > + (EvalAsInt(Ctx, Value, BO->getRHS(), NarrowedExpr)
>>> > + && TruncationChangesValue(Value, TargetRange, false));
>>> > +
>>> > + // We can ignore the left side of the comma operator, since the
>>> > value is
>>> > + // explicitly ignored anyway.
>>> > + case BO_Comma:
>>> > + return EvalAsInt(Ctx, Value, BO->getRHS(), NarrowedExpr) &&
>>> > + TruncationChangesValue(Value, TargetRange, false);
>>> > +
>>> > + case BO_Shl:
>>> > + return EvalAsInt(Ctx, Value, BO->getLHS(), NarrowedExpr) &&
>>> > + TruncationChangesValue(Value, TargetRange, false);
>>> > +
>>> > + default:
>>> > + break;
>>> > + }
>>> > + return false;
>>> > +}
>>> > +
>>> > void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
>>> > - SourceLocation CC, bool *ICContext = 0) {
>>> > + SourceLocation CC, bool *ICContext =
>>> > NULL) {
>>> > if (E->isTypeDependent() || E->isValueDependent()) return;
>>> >
>>> > const Type *Source =
>>> > S.Context.getCanonicalType(E->getType()).getTypePtr();
>>> > @@ -4977,17 +5052,30 @@ void CheckImplicitConversion(Sema &S, Ex
>>> > IntRange SourceRange = GetExprRange(S.Context, E);
>>> > IntRange TargetRange = IntRange::forTargetOfCanonicalType(S.Context,
>>> > Target);
>>> >
>>> > - if (SourceRange.Width > TargetRange.Width) {
>>> > - // If the source is a constant, use a default-on diagnostic.
>>> > - // TODO: this should happen for bitfield stores, too.
>>> > - llvm::APSInt Value(32);
>>> > - if (E->isIntegerConstantExpr(Value, S.Context)) {
>>> > - if (S.SourceMgr.isInSystemMacro(CC))
>>> > - return;
>>> > + llvm::APSInt Value(32);
>>> > + Expr *NarrowedExpr = NULL;
>>> > + // Use a default-on diagnostic if the source is involved in a
>>> > + // narrowing-prone binary operation with a constant.
>>> > + if (OperandMightImplicitlyNarrow(S.Context, Value, E, TargetRange,
>>> > + &NarrowedExpr)) {
>>> > + std::string PrettySourceValue = Value.toString(10);
>>> > + std::string PrettyTargetValue = PrettyPrintInRange(Value,
>>> > TargetRange);
>>> > +
>>> > + S.DiagRuntimeBehavior(E->getExprLoc(), NarrowedExpr,
>>> > + S.PDiag(diag::warn_impcast_integer_precision_binop_constant)
>>> > + << PrettySourceValue << PrettyTargetValue
>>> > + << E->getType() << T << NarrowedExpr->getSourceRange()
>>> > + << clang::SourceRange(CC));
>>> > + return;
>>> > + } else if (SourceRange.Width > TargetRange.Width) {
>>> > + // People want to build with -Wshorten-64-to-32 and not
>>> > -Wconversion.
>>> > + if (S.SourceMgr.isInSystemMacro(CC))
>>> > + return;
>>> >
>>> > + // If the source is a constant, use a default-on diagnostic.
>>> > + if (E->EvaluateAsInt(Value, S.Context, Expr::SE_AllowSideEffects))
>>> > {
>>> > std::string PrettySourceValue = Value.toString(10);
>>> > std::string PrettyTargetValue = PrettyPrintInRange(Value,
>>> > TargetRange);
>>> > -
>>> > S.DiagRuntimeBehavior(E->getExprLoc(), E,
>>> > S.PDiag(diag::warn_impcast_integer_precision_constant)
>>> > << PrettySourceValue << PrettyTargetValue
>>> > @@ -4996,10 +5084,6 @@ void CheckImplicitConversion(Sema &S, Ex
>>> > return;
>>> > }
>>> >
>>> > - // People want to build with -Wshorten-64-to-32 and not
>>> > -Wconversion.
>>> > - if (S.SourceMgr.isInSystemMacro(CC))
>>> > - return;
>>> > -
>>> > if (TargetRange.Width == 32 && S.Context.getIntWidth(E->getType())
>>> > == 64)
>>> > return DiagnoseImpCast(S, E, T, CC,
>>> > diag::warn_impcast_integer_64_32,
>>> > /* pruneControlFlow */ true);
>>> > @@ -5129,6 +5213,25 @@ void AnalyzeImplicitConversions(Sema &S,
>>> > if (E->getType() != T)
>>> > CheckImplicitConversion(S, E, T, CC);
>>> >
>>> > + // For CompoundAssignmentOperators, check the conversion from the
>>> > computed
>>> > + // LHS type to the type of the assignee.
>>> > + if (CompoundAssignOperator *CAO =
>>> > dyn_cast<CompoundAssignOperator>(E)) {
>>> > + QualType LHST = CAO->getComputationLHSType();
>>> > + // This CheckImplicitConversion would trigger a warning in
>>> > addition to the
>>> > + // more serious problem of using NULLs in arithmetic.
>>> > + // Let the call to CheckImplicitConversion on the child
>>> > expressions at the
>>> > + // bottom of this function handle them by filtering those out
>>> > here.
>>> > + // Similarly, disable the extra check for shift assignments, since
>>> > any
>>> > + // narrowing would be less serious than a too-large shift count.
>>> > + if (LHST != T &&
>>> > + T->isIntegralType(S.Context) &&
>>> > LHST->isIntegralType(S.Context) &&
>>> > + !CAO->isShiftAssignOp() &&
>>> > + CAO->getRHS()->isNullPointerConstant(S.Context,
>>> > +
>>> > Expr::NPC_ValueDependentIsNotNull)
>>> > + != Expr::NPCK_GNUNull)
>>> > + CheckImplicitConversion(S, CAO->getRHS(), T, CC);
>>> > + }
>>> > +
>>> > // Now continue drilling into this expression.
>>> >
>>> > // Skip past explicit casts.
>>> > @@ -5142,8 +5245,8 @@ void AnalyzeImplicitConversions(Sema &S,
>>> > if (BO->isComparisonOp())
>>> > return AnalyzeComparison(S, BO);
>>> >
>>> > - // And with simple assignments.
>>> > - if (BO->getOpcode() == BO_Assign)
>>> > + // And with assignments.
>>> > + if (BO->isAssignmentOp())
>>> > return AnalyzeAssignment(S, BO);
>>> > }
>>> >
>>> >
>>> > Modified: cfe/trunk/test/Sema/constant-conversion.c
>>> > URL:
>>> > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/constant-conversion.c?rev=178273&r1=178272&r2=178273&view=diff
>>> >
>>> > ==============================================================================
>>> > --- cfe/trunk/test/Sema/constant-conversion.c (original)
>>> > +++ cfe/trunk/test/Sema/constant-conversion.c Thu Mar 28 14:07:11 2013
>>> > @@ -66,13 +66,18 @@ void test7() {
>>> > struct {
>>> > unsigned int twoBits1:2;
>>> > unsigned int twoBits2:2;
>>> > - unsigned int reserved:28;
>>> > + unsigned int twoBits3:2;
>>> > + unsigned int reserved:26;
>>> > } f;
>>> >
>>> > f.twoBits1 = ~1; // expected-warning {{implicit truncation from
>>> > 'int' to bitfield changes value from -2 to 2}}
>>> > f.twoBits2 = ~2; // expected-warning {{implicit truncation from
>>> > 'int' to bitfield changes value from -3 to 1}}
>>> > f.twoBits1 &= ~1; // no-warning
>>> > f.twoBits2 &= ~2; // no-warning
>>> > + f.twoBits3 |= 4; // expected-warning {{implicit truncation from
>>> > 'int' to bitfield changes value from 4 to 0}}
>>> > + f.twoBits3 += 4; // expected-warning {{implicit truncation from
>>> > 'int' to bitfield changes value from 4 to 0}}
>>> > + f.twoBits3 *= 4; // expected-warning {{implicit truncation from
>>> > 'int' to bitfield changes value from 4 to 0}}
>>> > + f.twoBits3 |= 1; // no-warning
>>> > }
>>> >
>>> > void test8() {
>>> > @@ -80,3 +85,38 @@ void test8() {
>>> > struct { enum E x : 1; } f;
>>> > f.x = C; // expected-warning {{implicit truncation from 'int' to
>>> > bitfield changes value from 2 to 0}}
>>> > }
>>> > +
>>> > +int func(int);
>>> > +
>>> > +void test9() {
>>> > + unsigned char x = 0;
>>> > + unsigned char y = 0;
>>> > + x = y | 0x1ff; // expected-warning {{implicit conversion of binary
>>> > operation from 'int' to 'unsigned char' may change its value; value of
>>> > operand would be changed from 511 to 255 if converted before operation}}
>>> > + x = y | 0xff; // no-warning
>>> > + x = y & 0xdff; // expected-warning {{implicit conversion of binary
>>> > operation from 'int' to 'unsigned char' may change its value; value of
>>> > operand would be changed from 3583 to 255 if converted before operation}}
>>> > + x = y & 0xff; // no-warning
>>> > + x = y & ~1; // no-warning
>>> > + x = 0x1ff | y; // expected-warning {{implicit conversion of binary
>>> > operation from 'int' to 'unsigned char' may change its value; value of
>>> > operand would be changed from 511 to 255 if converted before operation}}
>>> > + x = 0xff | y; // no-warning
>>> > + x = (y | 0x1ff); // expected-warning {{implicit conversion of binary
>>> > operation from 'int' to 'unsigned char' may change its value; value of
>>> > operand would be changed from 511 to 255 if converted before operation}}
>>> > + x = (y | 0xff); // no-warning
>>> > + x = 0xff + y; // no-warning
>>> > + x += 0x1ff; // expected-warning {{implicit conversion from 'int' to
>>> > 'unsigned char' changes value from 511 to 255}}
>>> > + x = 0xff - y; // no-warning
>>> > + x -= 0x1ff; // expected-warning {{implicit conversion from 'int' to
>>> > 'unsigned char' changes value from 511 to 255}}
>>> > + x = y * 0x1ff; // expected-warning {{implicit conversion of binary
>>> > operation from 'int' to 'unsigned char' may change its value; value of
>>> > operand would be changed from 511 to 255 if converted before operation}}
>>> > + x = y * 0xff; // no-warning
>>> > + x *= 0x1ff; // expected-warning {{implicit conversion from 'int' to
>>> > 'unsigned char' changes value from 511 to 255}}
>>> > + x = y ^ 0xff; // no-warning
>>> > + x ^= 0x1ff; // expected-warning {{implicit conversion from 'int' to
>>> > 'unsigned char' changes value from 511 to 255}}
>>> > + x = (func(1), 0x1ff); // expected-warning {{implicit conversion of
>>> > binary operation from 'int' to 'unsigned char' may change its value; value
>>> > of operand would be changed from 511 to 255 if converted before operation}}
>>> > + x = (func(1), 0xff); // no-warning
>>> > + x = 0xff << y; // no-warning
>>> > + x = 0x1ff << y; // expected-warning {{implicit conversion of binary
>>> > operation from 'int' to 'unsigned char' may change its value; value of
>>> > operand would be changed from 511 to 255 if converted before operation}}
>>> > +
>>> > +
>>> > + // These next two tests make sure that both LHS and RHS are checked
>>> > for
>>> > + // narrowing operations.
>>> > + x = 0x1ff | 0xff; // expected-warning {{implicit conversion of
>>> > binary operation from 'int' to 'unsigned char' may change its value; value
>>> > of operand would be changed from 511 to 255 if converted before operation}}
>>> > + x = 0xff | 0x1ff; // expected-warning {{implicit conversion of
>>> > binary operation from 'int' to 'unsigned char' may change its value; value
>>> > of operand would be changed from 511 to 255 if converted before operation}}
>>> > +}
>>> >
>>> >
>>> > _______________________________________________
>>> > cfe-commits mailing list
>>> > cfe-commits at cs.uiuc.edu
>>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
More information about the cfe-commits
mailing list