[cfe-commits] [PATCH] Improve -Wtautological-constant-out-of-range-compare

[Richard Trieu]

I uploaded a new patch.

On Mon, Nov 12, 2012 at 11:22 AM, Eli Friedman <eli.friedman at gmail.com>wrote:

> Comments below.
>
> On Mon, Nov 12, 2012 at 9:46 AM, Richard Trieu <rtrieu at google.com> wrote:
> > Make -Wtautological-constant-out-of-range-compare checking take into
> account types and conversion between types.  The old version merely checked
> the bit widths, which allowed failed to catch a few cases, while warning on
> other safe comparisons.
> >
> > http://llvm-reviews.chandlerc.com/D113
> >
> > Files:
> >   test/Analysis/additive-folding.cpp
> >   test/SemaCXX/compare.cpp
> >   test/SemaCXX/warn-enum-compare.cpp
> >   lib/Sema/SemaChecking.cpp
>
> Index: test/SemaCXX/warn-enum-compare.cpp
> ===================================================================
> --- test/SemaCXX/warn-enum-compare.cpp
> +++ test/SemaCXX/warn-enum-compare.cpp
> @@ -39,8 +39,8 @@
>    while (b == c);
>    while (B1 == name1::B2);
>    while (B2 == name2::B1);
> -  while (x == AnonAA); // expected-warning {{comparison of constant
> 42 with expression of type 'Foo' is always false}}
> -  while (AnonBB == y); // expected-warning {{comparison of constant
> 45 with expression of type 'Bar' is always false}}
> +  while (x == AnonAA);
> +  while (AnonBB == y);
>    while (AnonAA == AnonAB);
>    while (AnonAB == AnonBA);
>    while (AnonBB == AnonAA);
>
> Why are you changing this warning?
>
See discussion below at IntRange.

>
>    while (AnonBB == AnonAA);
> Index: lib/Sema/SemaChecking.cpp
> ===================================================================
> --- lib/Sema/SemaChecking.cpp
> +++ lib/Sema/SemaChecking.cpp
> @@ -4328,38 +4328,91 @@
>                                           Expr *Constant, Expr *Other,
>                                           llvm::APSInt Value,
>                                           bool RhsConstant) {
> +  // 0 values are handled later by CheckTrivialUnsignedComparison().
> +  if (Value == 0)
> +    return;
> +
>    BinaryOperatorKind op = E->getOpcode();
>    QualType OtherT = Other->getType();
>    QualType ConstantT = Constant->getType();
> +  QualType CommonT = E->getLHS()->getType();
>    if (S.Context.hasSameUnqualifiedType(OtherT, ConstantT))
>      return;
>    assert((OtherT->isIntegerType() && ConstantT->isIntegerType())
>           && "comparison with non-integer type");
>    // FIXME. handle cases for signedness to catch (signed char)N == 200
>
> Does this patch fix this FIXME?
>
Yes.  FIXME removed, added test case.

>
> -  IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
> -  IntRange LitRange = GetValueRange(S.Context, Value,
> Value.getBitWidth());
> -  if (OtherRange.Width >= LitRange.Width)
> +
> +  bool ConstantSigned = ConstantT->isSignedIntegerType();
> +  bool OtherSigned = OtherT->isSignedIntegerType();
> +  bool CommonSigned = CommonT->isSignedIntegerType();
>
> Why are you getting rid of the IntRange usage?  Granted, we didn't
> really have any good testcases, but it does add power to the analysis
> in some cases.
>
 For my use, IntRange didn't add any value.  Everything I needed, I could
just get from the APSInt.  The only difference was what width I used for
Other.  IntRange does truncate the enum width so that different values will
be considered out of range when comparing against enums.  I factored out
the width into a variable.  The current code will remove the existing
warnings in warn-enum-compare.cpp.  The code in the comments would have
preserved them.

>
> +  bool EqualityOnly = false;
> +
> +  if (CommonSigned) {
> +    // The common type is signed, therefore no signed to unsigned
> conversion.
> +    if (OtherSigned) {
> +      // Check that the constant is representable in type OtherT.
> +      if (S.Context.getIntWidth(OtherT) >= Value.getMinSignedBits())
>
> Calling getMinSignedBits() on an unsigned constant will do the wrong
> thing in cases like 0xFFFFFFFFU.  (I'm not sure it's actually possible
> to write a testcase where this matters, but it would make more sense
> anyway.)
>
Added checks for signedness before using functions that depend on it.

>
> +        return;
> +    } else { // !OtherSigned
> +      // Check that the constant is representable in type OtherT.
> +      // Negative values are out of range.
> +      if (Value.isNonNegative() &&
> +          S.Context.getIntWidth(OtherT) >= Value.getActiveBits())
>
> Calling isNonNegative() on an unsigned constant can also do the wrong
> thing.
>
Same here.

>
> .+        return;
> +    }
> +  } else {  // !CommonSigned
> +    if (!OtherSigned && !ConstantSigned) {
> +      // Both types are unsigned, check the constant is representable in
> +      // type OtherT.
> +      if (S.Context.getIntWidth(OtherT) >= Value.getActiveBits())
> +        return;
> +    } else if (!OtherSigned && ConstantSigned) {
> +      if (S.Context.getIntWidth(OtherT) >= Value.getActiveBits())
> +        return;
>
> Calling getActiveBits() on a signed value is suspicious; the return
> will vary with the width of the constant for the same value.
>
And here.

>
> +    } else if (OtherSigned && !ConstantSigned) {
> +      // Check to see if the constant is representable in OtherT.
> +      if (S.Context.getIntWidth(OtherT) > Value.getActiveBits())
> +        return;
> +      // Check to see if the constant is equivalent to a negative value
> +      // cast to CommonT.
> +      if (S.Context.getIntWidth(ConstantT) ==
> S.Context.getIntWidth(CommonT) &&
> +          Value.isNegative() &&
>
> Is this going to handle cases like (int)x == 0xFFFFFFFFFFFFFFFFULL
> correctly?
>
 Yes, it doesn't warn on that case.

>
> -Eli
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121112/ca39c2f1/attachment.html>