r312750 - [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.
Roman Lebedev via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 8 09:10:54 PDT 2017
On Fri, Sep 8, 2017 at 3:26 PM, Roman Lebedev <lebedev.ri at gmail.com> wrote:
> On Fri, Sep 8, 2017 at 2:48 PM, Sam McCall <sammccall at 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.
> Roman.
Roman.
>> On Fri, Sep 8, 2017 at 12:14 AM, Roman Lebedev via cfe-commits
>> <cfe-commits at 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 at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>>
More information about the cfe-commits
mailing list