Reverted as r178320.
2013/3/28 Sam Panzer <[email protected]>: > 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
