r178273 - Implemented a warning when an input several bitwise operations are

Richard Smith richard at metafoo.co.uk
Thu Mar 28 17:04:26 PDT 2013


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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130328/67d8c30a/attachment.html>


More information about the cfe-commits mailing list