<p>Probably revert the change for now, once we note the false positives. I'm working on a fix.</p>
<div class="gmail_quote">On Mar 28, 2013 5:04 PM, "Richard Smith" <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Thu, Mar 28, 2013 at 4:53 PM, Timur Iskhodzhanov <span dir="ltr"><<a href="mailto:timurrrr@google.com" target="_blank">timurrrr@google.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Hi Sam,<br>
<br>
It looks like this change has uncovered some problems in Clang.<br>
<br>
I see a lot of compile-time errors on Linux like the following when<br>
building Clang with Clang.<br>
In file included from lib/Target/PowerPC/PPCJITInfo.cpp:17:<br>
In file included from lib/Target/PowerPC/PPCTargetMachine.h:18:<br>
In file included from /lib/Target/PowerPC/PPCISelLowering.h:22:<br>
/include/llvm/Target/TargetLowering.h:1012:59: error: implicit<br>
conversion from 'int' to 'uint8_t' (aka 'unsigned char') changes value<br>
from -241 to 15 [-Werror,-Wconstant-conversion]<br>
    IndexedModeActions[(unsigned)VT.SimpleTy][IdxMode] &= ~0xf0;<br></blockquote><div><br></div><div>Hm, we should probably allow bitwise &, |, and ^ if the constant operand's ignored bits are either all-0 or all-1.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
which look related to your recent commits.<br>
<br>
Can you please take a look?<br>
It makes some buildbots red...<br>
<br>
<br>
2013/3/28 Sam Panzer <<a href="mailto:espanz@gmail.com" target="_blank">espanz@gmail.com</a>>:<br>
<div><div>> Author: panzer<br>
> Date: Thu Mar 28 14:07:11 2013<br>
> New Revision: 178273<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=178273&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=178273&view=rev</a><br>
> Log:<br>
> Implemented a warning when an input several bitwise operations are<br>
> likely be implicitly truncated:<br>
><br>
>   * All forms of Bitwise-and, bitwise-or, and integer multiplication.<br>
>   * The assignment form of integer addition, subtraction, and exclusive-or<br>
>   * The RHS of the comma operator<br>
>   * The LHS of left shifts.<br>
><br>
> Modified:<br>
>     cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
>     cfe/trunk/lib/Sema/SemaChecking.cpp<br>
>     cfe/trunk/test/Sema/constant-conversion.c<br>
><br>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=178273&r1=178272&r2=178273&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=178273&r1=178272&r2=178273&view=diff</a><br>


> ==============================================================================<br>
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Mar 28 14:07:11 2013<br>
> @@ -2084,6 +2084,11 @@ def warn_impcast_integer_64_32 : Warning<br>
>  def warn_impcast_integer_precision_constant : Warning<<br>
>    "implicit conversion from %2 to %3 changes value from %0 to %1">,<br>
>    InGroup<ConstantConversion>;<br>
> +def warn_impcast_integer_precision_binop_constant : Warning<<br>
> +  "implicit conversion of binary operation from %2 to %3 may change its value; "<br>
> +  "value of operand would be changed from %0 to %1 if converted before "<br>
> +  "operation">,<br>
> +  InGroup<ConstantConversion>;<br>
>  def warn_impcast_bitfield_precision_constant : Warning<<br>
>    "implicit truncation from %2 to bitfield changes value from %0 to %1">,<br>
>    InGroup<ConstantConversion>;<br>
><br>
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=178273&r1=178272&r2=178273&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=178273&r1=178272&r2=178273&view=diff</a><br>


> ==============================================================================<br>
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)<br>
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Mar 28 14:07:11 2013<br>
> @@ -4686,7 +4686,8 @@ static void AnalyzeAssignment(Sema &S, B<br>
>    // We want to recurse on the RHS as normal unless we're assigning to<br>
>    // a bitfield.<br>
>    if (FieldDecl *Bitfield = E->getLHS()->getBitField()) {<br>
> -    if (AnalyzeBitFieldAssignment(S, Bitfield, E->getRHS(),<br>
> +    if (E->getOpcode() != BO_AndAssign &&<br>
> +        AnalyzeBitFieldAssignment(S, Bitfield, E->getRHS(),<br>
>                                    E->getOperatorLoc())) {<br>
>        // Recurse, ignoring any implicit conversions on the RHS.<br>
>        return AnalyzeImplicitConversions(S, E->getRHS()->IgnoreParenImpCasts(),<br>
> @@ -4795,8 +4796,82 @@ void CheckImplicitArgumentConversions(Se<br>
>    }<br>
>  }<br>
><br>
> +// Try to evaluate E as an integer. If EvaluateAsInt succeeds, Value is set to<br>
> +// the resulting value, ResultExpr is set to E.<br>
> +// Otherwise, the output parameters are not modified.<br>
> +static bool EvalAsInt(ASTContext &Ctx, llvm::APSInt &Value, Expr *E,<br>
> +                      Expr **ResultExpr) {<br>
> +  if (E->EvaluateAsInt(Value, Ctx, Expr::SE_AllowSideEffects)) {<br>
> +    *ResultExpr = E;<br>
> +    return true;<br>
> +  }<br>
> +  return false;<br>
> +}<br>
> +<br>
> +// Returns true when Value's value would change when narrowed to TargetRange.<br>
> +static bool TruncationChangesValue(const llvm::APSInt &Value,<br>
> +                                   const IntRange &TargetRange,<br>
> +                                   bool IsBitwiseAnd) {<br>
> +  // Checks if there are any active non-sign bits past the width of TargetRange.<br>
> +  if (IsBitwiseAnd)<br>
> +    return Value.getBitWidth() - Value.getNumSignBits() > TargetRange.Width;<br>
> +<br>
> +  llvm::APSInt UnsignedValue(Value);<br>
> +  unsigned BitWidth = TargetRange.NonNegative ?<br>
> +      UnsignedValue.getActiveBits() : Value.getMinSignedBits();<br>
> +  return BitWidth > TargetRange.Width;<br>
> +}<br>
> +<br>
> +// Determine if E is an expression containing or likely to contain an implicit<br>
> +// narrowing bug involving a constant.<br>
> +// If so, Value is set to the value of that constant. NarrowedExpr is set to the<br>
> +// problematic sub-expression if it is a strict subset of E.<br>
> +static bool OperandMightImplicitlyNarrow(ASTContext &Ctx, llvm::APSInt &Value,<br>
> +                                         Expr *E, IntRange TargetRange,<br>
> +                                         Expr **NarrowedExpr) {<br>
> +  BinaryOperator *BO = dyn_cast<BinaryOperator>(E);<br>
> +  if (!BO)<br>
> +    return false;<br>
> +<br>
> +  switch (BO->getOpcode()) {<br>
> +    case BO_And:<br>
> +    case BO_AndAssign:<br>
> +      return (EvalAsInt(Ctx, Value, BO->getLHS(), NarrowedExpr)<br>
> +               && TruncationChangesValue(Value, TargetRange, true)) ||<br>
> +              (EvalAsInt(Ctx, Value, BO->getRHS(), NarrowedExpr)<br>
> +               && TruncationChangesValue(Value, TargetRange, true));<br>
> +<br>
> +    case BO_Or:<br>
> +    case BO_OrAssign:<br>
> +    // FIXME: Include BO_Add, BO_Sub, and BO_Xor when we avoid false positives.<br>
> +    case BO_AddAssign:<br>
> +    case BO_SubAssign:<br>
> +    case BO_Mul:<br>
> +    case BO_MulAssign:<br>
> +    case BO_XorAssign:<br>
> +      return (EvalAsInt(Ctx, Value, BO->getLHS(), NarrowedExpr)<br>
> +               && TruncationChangesValue(Value, TargetRange, false)) ||<br>
> +              (EvalAsInt(Ctx, Value, BO->getRHS(), NarrowedExpr)<br>
> +               && TruncationChangesValue(Value, TargetRange, false));<br>
> +<br>
> +    // We can ignore the left side of the comma operator, since the value is<br>
> +    // explicitly ignored anyway.<br>
> +    case BO_Comma:<br>
> +      return EvalAsInt(Ctx, Value, BO->getRHS(), NarrowedExpr) &&<br>
> +             TruncationChangesValue(Value, TargetRange, false);<br>
> +<br>
> +    case BO_Shl:<br>
> +      return EvalAsInt(Ctx, Value, BO->getLHS(), NarrowedExpr) &&<br>
> +             TruncationChangesValue(Value, TargetRange, false);<br>
> +<br>
> +    default:<br>
> +      break;<br>
> +  }<br>
> +  return false;<br>
> +}<br>
> +<br>
>  void CheckImplicitConversion(Sema &S, Expr *E, QualType T,<br>
> -                             SourceLocation CC, bool *ICContext = 0) {<br>
> +                             SourceLocation CC, bool *ICContext = NULL) {<br>
>    if (E->isTypeDependent() || E->isValueDependent()) return;<br>
><br>
>    const Type *Source = S.Context.getCanonicalType(E->getType()).getTypePtr();<br>
> @@ -4977,17 +5052,30 @@ void CheckImplicitConversion(Sema &S, Ex<br>
>    IntRange SourceRange = GetExprRange(S.Context, E);<br>
>    IntRange TargetRange = IntRange::forTargetOfCanonicalType(S.Context, Target);<br>
><br>
> -  if (SourceRange.Width > TargetRange.Width) {<br>
> -    // If the source is a constant, use a default-on diagnostic.<br>
> -    // TODO: this should happen for bitfield stores, too.<br>
> -    llvm::APSInt Value(32);<br>
> -    if (E->isIntegerConstantExpr(Value, S.Context)) {<br>
> -      if (S.SourceMgr.isInSystemMacro(CC))<br>
> -        return;<br>
> +  llvm::APSInt Value(32);<br>
> +  Expr *NarrowedExpr = NULL;<br>
> +  // Use a default-on diagnostic if the source is involved in a<br>
> +  // narrowing-prone binary operation with a constant.<br>
> +  if (OperandMightImplicitlyNarrow(S.Context, Value, E, TargetRange,<br>
> +                                   &NarrowedExpr)) {<br>
> +    std::string PrettySourceValue = Value.toString(10);<br>
> +    std::string PrettyTargetValue = PrettyPrintInRange(Value, TargetRange);<br>
> +<br>
> +    S.DiagRuntimeBehavior(E->getExprLoc(), NarrowedExpr,<br>
> +      S.PDiag(diag::warn_impcast_integer_precision_binop_constant)<br>
> +          << PrettySourceValue << PrettyTargetValue<br>
> +          << E->getType() << T << NarrowedExpr->getSourceRange()<br>
> +          << clang::SourceRange(CC));<br>
> +    return;<br>
> +  } else if (SourceRange.Width > TargetRange.Width) {<br>
> +    // People want to build with -Wshorten-64-to-32 and not -Wconversion.<br>
> +    if (S.SourceMgr.isInSystemMacro(CC))<br>
> +      return;<br>
><br>
> +    // If the source is a constant, use a default-on diagnostic.<br>
> +    if (E->EvaluateAsInt(Value, S.Context, Expr::SE_AllowSideEffects)) {<br>
>        std::string PrettySourceValue = Value.toString(10);<br>
>        std::string PrettyTargetValue = PrettyPrintInRange(Value, TargetRange);<br>
> -<br>
>        S.DiagRuntimeBehavior(E->getExprLoc(), E,<br>
>          S.PDiag(diag::warn_impcast_integer_precision_constant)<br>
>              << PrettySourceValue << PrettyTargetValue<br>
> @@ -4996,10 +5084,6 @@ void CheckImplicitConversion(Sema &S, Ex<br>
>        return;<br>
>      }<br>
><br>
> -    // People want to build with -Wshorten-64-to-32 and not -Wconversion.<br>
> -    if (S.SourceMgr.isInSystemMacro(CC))<br>
> -      return;<br>
> -<br>
>      if (TargetRange.Width == 32 && S.Context.getIntWidth(E->getType()) == 64)<br>
>        return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_integer_64_32,<br>
>                               /* pruneControlFlow */ true);<br>
> @@ -5129,6 +5213,25 @@ void AnalyzeImplicitConversions(Sema &S,<br>
>    if (E->getType() != T)<br>
>      CheckImplicitConversion(S, E, T, CC);<br>
><br>
> +  // For CompoundAssignmentOperators, check the conversion from the computed<br>
> +  // LHS type to the type of the assignee.<br>
> +  if (CompoundAssignOperator *CAO = dyn_cast<CompoundAssignOperator>(E)) {<br>
> +    QualType LHST = CAO->getComputationLHSType();<br>
> +    // This CheckImplicitConversion would trigger a warning in addition to the<br>
> +    // more serious problem of using NULLs in arithmetic.<br>
> +    // Let the call to CheckImplicitConversion on the child expressions at the<br>
> +    // bottom of this function handle them by filtering those out here.<br>
> +    // Similarly, disable the extra check for shift assignments, since any<br>
> +    // narrowing would be less serious than a too-large shift count.<br>
> +    if (LHST != T &&<br>
> +        T->isIntegralType(S.Context) && LHST->isIntegralType(S.Context) &&<br>
> +        !CAO->isShiftAssignOp() &&<br>
> +        CAO->getRHS()->isNullPointerConstant(S.Context,<br>
> +                                             Expr::NPC_ValueDependentIsNotNull)<br>
> +        != Expr::NPCK_GNUNull)<br>
> +      CheckImplicitConversion(S, CAO->getRHS(), T, CC);<br>
> +  }<br>
> +<br>
>    // Now continue drilling into this expression.<br>
><br>
>    // Skip past explicit casts.<br>
> @@ -5142,8 +5245,8 @@ void AnalyzeImplicitConversions(Sema &S,<br>
>      if (BO->isComparisonOp())<br>
>        return AnalyzeComparison(S, BO);<br>
><br>
> -    // And with simple assignments.<br>
> -    if (BO->getOpcode() == BO_Assign)<br>
> +    // And with assignments.<br>
> +    if (BO->isAssignmentOp())<br>
>        return AnalyzeAssignment(S, BO);<br>
>    }<br>
><br>
><br>
> Modified: cfe/trunk/test/Sema/constant-conversion.c<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/constant-conversion.c?rev=178273&r1=178272&r2=178273&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/constant-conversion.c?rev=178273&r1=178272&r2=178273&view=diff</a><br>


> ==============================================================================<br>
> --- cfe/trunk/test/Sema/constant-conversion.c (original)<br>
> +++ cfe/trunk/test/Sema/constant-conversion.c Thu Mar 28 14:07:11 2013<br>
> @@ -66,13 +66,18 @@ void test7() {<br>
>         struct {<br>
>                 unsigned int twoBits1:2;<br>
>                 unsigned int twoBits2:2;<br>
> -               unsigned int reserved:28;<br>
> +               unsigned int twoBits3:2;<br>
> +               unsigned int reserved:26;<br>
>         } f;<br>
><br>
>         f.twoBits1 = ~1; // expected-warning {{implicit truncation from 'int' to bitfield changes value from -2 to 2}}<br>
>         f.twoBits2 = ~2; // expected-warning {{implicit truncation from 'int' to bitfield changes value from -3 to 1}}<br>
>         f.twoBits1 &= ~1; // no-warning<br>
>         f.twoBits2 &= ~2; // no-warning<br>
> +       f.twoBits3 |= 4; // expected-warning {{implicit truncation from 'int' to bitfield changes value from 4 to 0}}<br>
> +       f.twoBits3 += 4; // expected-warning {{implicit truncation from 'int' to bitfield changes value from 4 to 0}}<br>
> +       f.twoBits3 *= 4; // expected-warning {{implicit truncation from 'int' to bitfield changes value from 4 to 0}}<br>
> +       f.twoBits3 |= 1; // no-warning<br>
>  }<br>
><br>
>  void test8() {<br>
> @@ -80,3 +85,38 @@ void test8() {<br>
>    struct { enum E x : 1; } f;<br>
>    f.x = C; // expected-warning {{implicit truncation from 'int' to bitfield changes value from 2 to 0}}<br>
>  }<br>
> +<br>
> +int func(int);<br>
> +<br>
> +void test9() {<br>
> +  unsigned char x = 0;<br>
> +  unsigned char y = 0;<br>
> +  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}}<br>


> +  x = y | 0xff; // no-warning<br>
> +  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}}<br>


> +  x = y & 0xff; // no-warning<br>
> +  x = y & ~1; // no-warning<br>
> +  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}}<br>


> +  x = 0xff | y; // no-warning<br>
> +  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}}<br>


> +  x = (y | 0xff); // no-warning<br>
> +  x = 0xff + y; // no-warning<br>
> +  x += 0x1ff; // expected-warning {{implicit conversion from 'int' to 'unsigned char' changes value from 511 to 255}}<br>
> +  x = 0xff - y; // no-warning<br>
> +  x -= 0x1ff; // expected-warning {{implicit conversion from 'int' to 'unsigned char' changes value from 511 to 255}}<br>
> +  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}}<br>


> +  x = y * 0xff; // no-warning<br>
> +  x *= 0x1ff; // expected-warning {{implicit conversion from 'int' to 'unsigned char' changes value from 511 to 255}}<br>
> +  x = y ^ 0xff; // no-warning<br>
> +  x ^= 0x1ff; // expected-warning {{implicit conversion from 'int' to 'unsigned char' changes value from 511 to 255}}<br>
> +  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}}<br>


> +  x = (func(1), 0xff); // no-warning<br>
> +  x = 0xff << y; // no-warning<br>
> +  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}}<br>


> +<br>
> +<br>
> +  // These next two tests make sure that both LHS and RHS are checked for<br>
> +  // narrowing operations.<br>
> +  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}}<br>


> +  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}}<br>


> +}<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br>
</blockquote></div>