r312750 - [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 8 04:48:37 PDT 2017
Nice fix! 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.
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?
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170908/55351edd/attachment-0001.html>
More information about the cfe-commits
mailing list