On Fri, Sep 8, 2017 at 7:10 PM, Roman Lebedev <lebedev...@gmail.com> wrote: > On Fri, Sep 8, 2017 at 3:26 PM, Roman Lebedev <lebedev...@gmail.com> wrote: >> On Fri, Sep 8, 2017 at 2:48 PM, Sam McCall <sammcc...@google.com> wrote: >> Hi. >> >>> Nice fix! >> Thank you! >> >>> It catches a lot of new cases on our codebase, all technically >>> correct so far. >>> >>> A couple of issues though: >>> A) Rollout - until we've completely cleaned up, we need to disable >>> -Wtautological-compare entirely, which is a valuable check. I imagine anyone >>> else using -Werror is in the same boat. >>> What do you think about putting the new warnings behind a subcategory? (e.g. >>> -Wtautological-compare-unsigned-zero, implied by -Wtautological-compare) >>> It's an ugly artifact of the history here, but allows this fix to be rolled >>> out in a controlled way. >> https://reviews.llvm.org/D37620 > And landed. > >>> B) Enums (not new I guess). Typical case: if (enum < 0 || enum > MAX) >>> The warning strongly suggests that the enum < 0 check has no effect (for >>> enums with nonnegative ranges). >>> Clang doesn't seem to optimize such checks out though, and they seem likely >>> to catch bugs in some cases. Yes, only if there's UB elsewhere, but I assume >>> not optimizing out these checks indicates a deliberate decision to stay >>> somewhat compatible with a technically-incorrect mental model. >>> If this is the case, should we move these to a -Wtautological-compare-enum >>> subcategory? >> (Did not look at this yet) > https://reviews.llvm.org/D37629 i hope that is what you meant. And landed, too.
Next will try to work on the second part of the fix for https://bugs.llvm.org/show_bug.cgi?id=34147 >> Roman. > Roman. Roman. >>> On Fri, Sep 8, 2017 at 12:14 AM, Roman Lebedev via cfe-commits >>> <cfe-commits@lists.llvm.org> wrote: >>>> >>>> Author: lebedevri >>>> Date: Thu Sep 7 15:14:25 2017 >>>> New Revision: 312750 >>>> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=312750&view=rev >>>> Log: >>>> [Sema] -Wtautological-compare: handle comparison of unsigned with 0S. >>>> >>>> Summary: >>>> This is a first half(?) of a fix for the following bug: >>>> https://bugs.llvm.org/show_bug.cgi?id=34147 (gcc -Wtype-limits) >>>> >>>> GCC's -Wtype-limits does warn on comparison of unsigned value >>>> with signed zero (as in, with 0), but clang only warns if the >>>> zero is unsigned (i.e. 0U). >>>> >>>> Also, be careful not to double-warn, or falsely warn on >>>> comparison of signed/fp variable and signed 0. >>>> >>>> Yes, all these testcases are needed. >>>> >>>> Testing: $ ninja check-clang-sema check-clang-semacxx >>>> Also, no new warnings for clang stage-2 build. >>>> >>>> Reviewers: rjmccall, rsmith, aaron.ballman >>>> >>>> Reviewed By: rjmccall >>>> >>>> Subscribers: cfe-commits >>>> >>>> Tags: #clang >>>> >>>> Differential Revision: https://reviews.llvm.org/D37565 >>>> >>>> Modified: >>>> cfe/trunk/docs/ReleaseNotes.rst >>>> cfe/trunk/lib/Sema/SemaChecking.cpp >>>> cfe/trunk/test/Sema/compare.c >>>> cfe/trunk/test/Sema/outof-range-constant-compare.c >>>> cfe/trunk/test/SemaCXX/compare.cpp >>>> >>>> Modified: cfe/trunk/docs/ReleaseNotes.rst >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=312750&r1=312749&r2=312750&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/docs/ReleaseNotes.rst (original) >>>> +++ cfe/trunk/docs/ReleaseNotes.rst Thu Sep 7 15:14:25 2017 >>>> @@ -71,6 +71,13 @@ Improvements to Clang's diagnostics >>>> errors/warnings, as the system frameworks might add a method with the >>>> same >>>> selector which could make the message send to ``id`` ambiguous. >>>> >>>> +- ``-Wtautological-compare`` now warns when comparing an unsigned integer >>>> and 0 >>>> + regardless of whether the constant is signed or unsigned." >>>> + >>>> +- ``-Wtautological-compare`` now warns about comparing a signed integer >>>> and 0 >>>> + when the signed integer is coerced to an unsigned type for the >>>> comparison. >>>> + ``-Wsign-compare`` was adjusted not to warn in this case. >>>> + >>>> Non-comprehensive list of changes in this release >>>> ------------------------------------------------- >>>> >>>> >>>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=312750&r1=312749&r2=312750&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original) >>>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Sep 7 15:14:25 2017 >>>> @@ -8567,32 +8567,51 @@ bool HasEnumType(Expr *E) { >>>> return E->getType()->isEnumeralType(); >>>> } >>>> >>>> -void CheckTrivialUnsignedComparison(Sema &S, BinaryOperator *E) { >>>> +bool isNonBooleanUnsignedValue(Expr *E) { >>>> + // We are checking that the expression is not known to have boolean >>>> value, >>>> + // is an integer type; and is either unsigned after implicit casts, >>>> + // or was unsigned before implicit casts. >>>> + return !E->isKnownToHaveBooleanValue() && E->getType()->isIntegerType() >>>> && >>>> + (!E->getType()->isSignedIntegerType() || >>>> + !E->IgnoreParenImpCasts()->getType()->isSignedIntegerType()); >>>> +} >>>> + >>>> +bool CheckTautologicalComparisonWithZero(Sema &S, BinaryOperator *E) { >>>> // Disable warning in template instantiations. >>>> if (S.inTemplateInstantiation()) >>>> - return; >>>> + return false; >>>> + >>>> + // bool values are handled by DiagnoseOutOfRangeComparison(). >>>> >>>> BinaryOperatorKind op = E->getOpcode(); >>>> if (E->isValueDependent()) >>>> - return; >>>> + return false; >>>> >>>> - if (op == BO_LT && IsZero(S, E->getRHS())) { >>>> + Expr *LHS = E->getLHS(); >>>> + Expr *RHS = E->getRHS(); >>>> + >>>> + bool Match = true; >>>> + >>>> + if (op == BO_LT && isNonBooleanUnsignedValue(LHS) && IsZero(S, RHS)) { >>>> S.Diag(E->getOperatorLoc(), >>>> diag::warn_lunsigned_always_true_comparison) >>>> - << "< 0" << "false" << HasEnumType(E->getLHS()) >>>> - << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange(); >>>> - } else if (op == BO_GE && IsZero(S, E->getRHS())) { >>>> + << "< 0" << "false" << HasEnumType(LHS) >>>> + << LHS->getSourceRange() << RHS->getSourceRange(); >>>> + } else if (op == BO_GE && isNonBooleanUnsignedValue(LHS) && IsZero(S, >>>> RHS)) { >>>> S.Diag(E->getOperatorLoc(), >>>> diag::warn_lunsigned_always_true_comparison) >>>> - << ">= 0" << "true" << HasEnumType(E->getLHS()) >>>> - << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange(); >>>> - } else if (op == BO_GT && IsZero(S, E->getLHS())) { >>>> + << ">= 0" << "true" << HasEnumType(LHS) >>>> + << LHS->getSourceRange() << RHS->getSourceRange(); >>>> + } else if (op == BO_GT && isNonBooleanUnsignedValue(RHS) && IsZero(S, >>>> LHS)) { >>>> S.Diag(E->getOperatorLoc(), >>>> diag::warn_runsigned_always_true_comparison) >>>> - << "0 >" << "false" << HasEnumType(E->getRHS()) >>>> - << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange(); >>>> - } else if (op == BO_LE && IsZero(S, E->getLHS())) { >>>> + << "0 >" << "false" << HasEnumType(RHS) >>>> + << LHS->getSourceRange() << RHS->getSourceRange(); >>>> + } else if (op == BO_LE && isNonBooleanUnsignedValue(RHS) && IsZero(S, >>>> LHS)) { >>>> S.Diag(E->getOperatorLoc(), >>>> diag::warn_runsigned_always_true_comparison) >>>> - << "0 <=" << "true" << HasEnumType(E->getRHS()) >>>> - << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange(); >>>> - } >>>> + << "0 <=" << "true" << HasEnumType(RHS) >>>> + << LHS->getSourceRange() << RHS->getSourceRange(); >>>> + } else >>>> + Match = false; >>>> + >>>> + return Match; >>>> } >>>> >>>> void DiagnoseOutOfRangeComparison(Sema &S, BinaryOperator *E, Expr >>>> *Constant, >>>> @@ -8612,7 +8631,7 @@ void DiagnoseOutOfRangeComparison(Sema & >>>> >>>> bool OtherIsBooleanType = Other->isKnownToHaveBooleanValue(); >>>> >>>> - // 0 values are handled later by CheckTrivialUnsignedComparison(). >>>> + // 0 values are handled later by CheckTautologicalComparisonWithZero(). >>>> if ((Value == 0) && (!OtherIsBooleanType)) >>>> return; >>>> >>>> @@ -8849,16 +8868,22 @@ void AnalyzeComparison(Sema &S, BinaryOp >>>> (IsRHSIntegralLiteral && IsLHSIntegralLiteral); >>>> } else if (!T->hasUnsignedIntegerRepresentation()) >>>> IsComparisonConstant = E->isIntegerConstantExpr(S.Context); >>>> - >>>> + >>>> + // We don't care about value-dependent expressions or expressions >>>> + // whose result is a constant. >>>> + if (IsComparisonConstant) >>>> + return AnalyzeImpConvsInComparison(S, E); >>>> + >>>> + // If this is a tautological comparison, suppress -Wsign-compare. >>>> + if (CheckTautologicalComparisonWithZero(S, E)) >>>> + return AnalyzeImpConvsInComparison(S, E); >>>> + >>>> // We don't do anything special if this isn't an unsigned integral >>>> // comparison: we're only interested in integral comparisons, and >>>> // signed comparisons only happen in cases we don't care to warn about. >>>> - // >>>> - // We also don't care about value-dependent expressions or expressions >>>> - // whose result is a constant. >>>> - if (!T->hasUnsignedIntegerRepresentation() || IsComparisonConstant) >>>> + if (!T->hasUnsignedIntegerRepresentation()) >>>> return AnalyzeImpConvsInComparison(S, E); >>>> - >>>> + >>>> // Check to see if one of the (unmodified) operands is of different >>>> // signedness. >>>> Expr *signedOperand, *unsignedOperand; >>>> @@ -8871,7 +8896,6 @@ void AnalyzeComparison(Sema &S, BinaryOp >>>> signedOperand = RHS; >>>> unsignedOperand = LHS; >>>> } else { >>>> - CheckTrivialUnsignedComparison(S, E); >>>> return AnalyzeImpConvsInComparison(S, E); >>>> } >>>> >>>> @@ -8883,11 +8907,9 @@ void AnalyzeComparison(Sema &S, BinaryOp >>>> AnalyzeImplicitConversions(S, LHS, E->getOperatorLoc()); >>>> AnalyzeImplicitConversions(S, RHS, E->getOperatorLoc()); >>>> >>>> - // If the signed range is non-negative, -Wsign-compare won't fire, >>>> - // but we should still check for comparisons which are always true >>>> - // or false. >>>> + // If the signed range is non-negative, -Wsign-compare won't fire. >>>> if (signedRange.NonNegative) >>>> - return CheckTrivialUnsignedComparison(S, E); >>>> + return; >>>> >>>> // For (in)equality comparisons, if the unsigned operand is a >>>> // constant which cannot collide with a overflowed signed operand, >>>> >>>> Modified: cfe/trunk/test/Sema/compare.c >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/compare.c?rev=312750&r1=312749&r2=312750&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/test/Sema/compare.c (original) >>>> +++ cfe/trunk/test/Sema/compare.c Thu Sep 7 15:14:25 2017 >>>> @@ -77,7 +77,7 @@ int ints(long a, unsigned long b) { >>>> ((int) a == (unsigned int) B) + >>>> ((short) a == (unsigned short) B) + >>>> ((signed char) a == (unsigned char) B) + >>>> - (a < (unsigned long) B) + // expected-warning {{comparison of >>>> integers of different signs}} >>>> + (a < (unsigned long) B) + // expected-warning {{comparison of >>>> unsigned expression < 0 is always false}} >>>> (a < (unsigned int) B) + >>>> (a < (unsigned short) B) + >>>> (a < (unsigned char) B) + >>>> @@ -85,8 +85,8 @@ int ints(long a, unsigned long b) { >>>> ((int) a < B) + >>>> ((short) a < B) + >>>> ((signed char) a < B) + >>>> - ((long) a < (unsigned long) B) + // expected-warning >>>> {{comparison of integers of different signs}} >>>> - ((int) a < (unsigned int) B) + // expected-warning {{comparison >>>> of integers of different signs}} >>>> + ((long) a < (unsigned long) B) + // expected-warning >>>> {{comparison of unsigned expression < 0 is always false}} >>>> + ((int) a < (unsigned int) B) + // expected-warning {{comparison >>>> of unsigned expression < 0 is always false}} >>>> ((short) a < (unsigned short) B) + >>>> ((signed char) a < (unsigned char) B) + >>>> >>>> >>>> Modified: cfe/trunk/test/Sema/outof-range-constant-compare.c >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/outof-range-constant-compare.c?rev=312750&r1=312749&r2=312750&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/test/Sema/outof-range-constant-compare.c (original) >>>> +++ cfe/trunk/test/Sema/outof-range-constant-compare.c Thu Sep 7 15:14:25 >>>> 2017 >>>> @@ -6,6 +6,59 @@ int value(void); >>>> int main() >>>> { >>>> int a = value(); >>>> + >>>> + if (a == 0x0000000000000000L) >>>> + return 0; >>>> + if (a != 0x0000000000000000L) >>>> + return 0; >>>> + if (a < 0x0000000000000000L) >>>> + return 0; >>>> + if (a <= 0x0000000000000000L) >>>> + return 0; >>>> + if (a > 0x0000000000000000L) >>>> + return 0; >>>> + if (a >= 0x0000000000000000L) >>>> + return 0; >>>> + >>>> + if (0x0000000000000000L == a) >>>> + return 0; >>>> + if (0x0000000000000000L != a) >>>> + return 0; >>>> + if (0x0000000000000000L < a) >>>> + return 0; >>>> + if (0x0000000000000000L <= a) >>>> + return 0; >>>> + if (0x0000000000000000L > a) >>>> + return 0; >>>> + if (0x0000000000000000L >= a) >>>> + return 0; >>>> + >>>> + if (a == 0x0000000000000000UL) >>>> + return 0; >>>> + if (a != 0x0000000000000000UL) >>>> + return 0; >>>> + if (a < 0x0000000000000000UL) // expected-warning {{comparison of >>>> unsigned expression < 0 is always false}} >>>> + return 0; >>>> + if (a <= 0x0000000000000000UL) >>>> + return 0; >>>> + if (a > 0x0000000000000000UL) >>>> + return 0; >>>> + if (a >= 0x0000000000000000UL) // expected-warning {{comparison of >>>> unsigned expression >= 0 is always true}} >>>> + return 0; >>>> + >>>> + if (0x0000000000000000UL == a) >>>> + return 0; >>>> + if (0x0000000000000000UL != a) >>>> + return 0; >>>> + if (0x0000000000000000UL < a) >>>> + return 0; >>>> + if (0x0000000000000000UL <= a) // expected-warning {{comparison of 0 >>>> <= unsigned expression is always true}} >>>> + return 0; >>>> + if (0x0000000000000000UL > a) // expected-warning {{comparison of 0 > >>>> unsigned expression is always false}} >>>> + return 0; >>>> + if (0x0000000000000000UL >= a) >>>> + return 0; >>>> + >>>> if (a == 0x1234567812345678L) // expected-warning {{comparison of >>>> constant 1311768465173141112 with expression of type 'int' is always >>>> false}} >>>> return 0; >>>> if (a != 0x1234567812345678L) // expected-warning {{comparison of >>>> constant 1311768465173141112 with expression of type 'int' is always true}} >>>> @@ -74,6 +127,7 @@ int main() >>>> return 0; >>>> if (0x1234567812345678L >= s) // expected-warning {{comparison of >>>> constant 1311768465173141112 with expression of type 'short' is always >>>> true}} >>>> return 0; >>>> + >>>> long l = value(); >>>> if (l == 0x1234567812345678L) >>>> return 0; >>>> @@ -106,13 +160,13 @@ int main() >>>> return 0; >>>> if (un != 0x0000000000000000L) >>>> return 0; >>>> - if (un < 0x0000000000000000L) >>>> + if (un < 0x0000000000000000L) // expected-warning {{comparison of >>>> unsigned expression < 0 is always false}} >>>> return 0; >>>> if (un <= 0x0000000000000000L) >>>> return 0; >>>> if (un > 0x0000000000000000L) >>>> return 0; >>>> - if (un >= 0x0000000000000000L) >>>> + if (un >= 0x0000000000000000L) // expected-warning {{comparison of >>>> unsigned expression >= 0 is always true}} >>>> return 0; >>>> >>>> if (0x0000000000000000L == un) >>>> @@ -121,19 +175,92 @@ int main() >>>> return 0; >>>> if (0x0000000000000000L < un) >>>> return 0; >>>> - if (0x0000000000000000L <= un) >>>> + if (0x0000000000000000L <= un) // expected-warning {{comparison of 0 >>>> <= unsigned expression is always true}} >>>> return 0; >>>> - if (0x0000000000000000L > un) >>>> + if (0x0000000000000000L > un) // expected-warning {{comparison of 0 > >>>> unsigned expression is always false}} >>>> return 0; >>>> if (0x0000000000000000L >= un) >>>> return 0; >>>> + >>>> + if (un == 0x0000000000000000UL) >>>> + return 0; >>>> + if (un != 0x0000000000000000UL) >>>> + return 0; >>>> + if (un < 0x0000000000000000UL) // expected-warning {{comparison of >>>> unsigned expression < 0 is always false}} >>>> + return 0; >>>> + if (un <= 0x0000000000000000UL) >>>> + return 0; >>>> + if (un > 0x0000000000000000UL) >>>> + return 0; >>>> + if (un >= 0x0000000000000000UL) // expected-warning {{comparison of >>>> unsigned expression >= 0 is always true}} >>>> + return 0; >>>> + >>>> + if (0x0000000000000000UL == un) >>>> + return 0; >>>> + if (0x0000000000000000UL != un) >>>> + return 0; >>>> + if (0x0000000000000000UL < un) >>>> + return 0; >>>> + if (0x0000000000000000UL <= un) // expected-warning {{comparison of 0 >>>> <= unsigned expression is always true}} >>>> + return 0; >>>> + if (0x0000000000000000UL > un) // expected-warning {{comparison of 0 >>>> > unsigned expression is always false}} >>>> + return 0; >>>> + if (0x0000000000000000UL >= un) >>>> + return 0; >>>> + >>>> float fl = 0; >>>> - if (fl == 0x0000000000000000L) // no warning >>>> - return 0; >>>> + if (fl == 0x0000000000000000L) >>>> + return 0; >>>> + if (fl != 0x0000000000000000L) >>>> + return 0; >>>> + if (fl < 0x0000000000000000L) >>>> + return 0; >>>> + if (fl <= 0x0000000000000000L) >>>> + return 0; >>>> + if (fl > 0x0000000000000000L) >>>> + return 0; >>>> + if (fl >= 0x0000000000000000L) >>>> + return 0; >>>> >>>> - float dl = 0; >>>> - if (dl == 0x0000000000000000L) // no warning >>>> - return 0; >>>> + if (0x0000000000000000L == fl) >>>> + return 0; >>>> + if (0x0000000000000000L != fl) >>>> + return 0; >>>> + if (0x0000000000000000L < fl) >>>> + return 0; >>>> + if (0x0000000000000000L <= fl) >>>> + return 0; >>>> + if (0x0000000000000000L > fl) >>>> + return 0; >>>> + if (0x0000000000000000L >= fl) >>>> + return 0; >>>> + >>>> + double dl = 0; >>>> + if (dl == 0x0000000000000000L) >>>> + return 0; >>>> + if (dl != 0x0000000000000000L) >>>> + return 0; >>>> + if (dl < 0x0000000000000000L) >>>> + return 0; >>>> + if (dl <= 0x0000000000000000L) >>>> + return 0; >>>> + if (dl > 0x0000000000000000L) >>>> + return 0; >>>> + if (dl >= 0x0000000000000000L) >>>> + return 0; >>>> + >>>> + if (0x0000000000000000L == dl) >>>> + return 0; >>>> + if (0x0000000000000000L != dl) >>>> + return 0; >>>> + if (0x0000000000000000L < dl) >>>> + return 0; >>>> + if (0x0000000000000000L <= dl) >>>> + return 0; >>>> + if (0x0000000000000000L > dl) >>>> + return 0; >>>> + if (0x0000000000000000L >= dl) >>>> + return 0; >>>> >>>> enum E { >>>> yes, >>>> >>>> Modified: cfe/trunk/test/SemaCXX/compare.cpp >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/compare.cpp?rev=312750&r1=312749&r2=312750&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/test/SemaCXX/compare.cpp (original) >>>> +++ cfe/trunk/test/SemaCXX/compare.cpp Thu Sep 7 15:14:25 2017 >>>> @@ -73,7 +73,7 @@ int test0(long a, unsigned long b) { >>>> ((int) a == (unsigned int) B) + >>>> ((short) a == (unsigned short) B) + >>>> ((signed char) a == (unsigned char) B) + >>>> - (a < (unsigned long) B) + // expected-warning {{comparison of >>>> integers of different signs}} >>>> + (a < (unsigned long) B) + // expected-warning {{comparison of >>>> unsigned expression < 0 is always false}} >>>> (a < (unsigned int) B) + >>>> (a < (unsigned short) B) + >>>> (a < (unsigned char) B) + >>>> @@ -81,8 +81,8 @@ int test0(long a, unsigned long b) { >>>> ((int) a < B) + >>>> ((short) a < B) + >>>> ((signed char) a < B) + >>>> - ((long) a < (unsigned long) B) + // expected-warning >>>> {{comparison of integers of different signs}} >>>> - ((int) a < (unsigned int) B) + // expected-warning {{comparison >>>> of integers of different signs}} >>>> + ((long) a < (unsigned long) B) + // expected-warning >>>> {{comparison of unsigned expression < 0 is always false}} >>>> + ((int) a < (unsigned int) B) + // expected-warning {{comparison >>>> of unsigned expression < 0 is always false}} >>>> ((short) a < (unsigned short) B) + >>>> ((signed char) a < (unsigned char) B) + >>>> >>>> >>>> >>>> _______________________________________________ >>>> cfe-commits mailing list >>>> cfe-commits@lists.llvm.org >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> >>> _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits