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" <[email protected]> wrote:
> On Thu, Mar 28, 2013 at 4:53 PM, Timur Iskhodzhanov > <[email protected]>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 <[email protected]>: >> > 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 >> > [email protected] >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
