[Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

[lebedev.ri]

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.

[lebedev.ri]

Added ReleaseNotes.rst note

[rjmccall]

lib/Sema/SemaChecking.cpp

8599

Please extract a function which computes this for an Expr* and then call it as part of the conditions below, e.g.:

if (op == BO_LT && isNonBooleanUnsignedValue(E->getLHS()) && IsZero(S, E->getRHS()))

8876

Part of the purpose of checking for signed comparisons up here is to avoid unnecessarily computing ranges for the operands when we aren't going to use them. That's still something we want to avoid. I think you just need to call CheckTrivialUnsignedComparison in this fallback case, at least when the comparison is not constant.

You should also rename CheckTrivialUnsignedComparison to something like CheckTautologicalComparison.

[lebedev.ri]

Address review notes.

lib/Sema/SemaChecking.cpp

8599

Yes, that is indeed a better way :)

8876

Ok, i must admit that i understand the idea to have minimal overhead. But i don't really follow the code in here :)
This seems to work, and check-clang-sema+check-clang-semacxx+stage2 still pass.
Is this obviously-wrong / some testcase is missing?

[rjmccall]

lib/Sema/SemaChecking.cpp

8589

Do you still need these? I'm always antsy about code that ignores implicit casts, especially before evaluation.

8876

There's a lot of code here doing very similar checks, and we just want to ensure that we aren't doing too much redundant work. I think the way you've structured it now is fine.

[lebedev.ri]

lib/Sema/SemaChecking.cpp

8589

Yes, absolutely. I did check without them, and they are needed. Else:

$ ninja check-clang-sema check-clang-semacxx
[27/29] Running lit suite /build/clang/test/Sema
lit.py: /build/clang/test/lit.cfg:200: note: using clang: '/build/llvm-build-Clang-release/./bin/clang'
FAIL: Clang :: Sema/compare.c (195 of 548)
******************** TEST 'Clang :: Sema/compare.c' FAILED ********************
Script:
--
/build/llvm-build-Clang-release/./bin/clang -cc1 -internal-isystem /build/llvm-build-Clang-release/lib/clang/6.0.0/include -nostdsysteminc -triple x86_64-apple-darwin -fsyntax-only -pedantic -verify -Wsign-compare /build/clang/test/Sema/compare.c -Wno-unreachable-code
--
Exit Code: 1

Command Output (stderr):
--
error: 'warning' diagnostics seen but not expected: 
  File /build/clang/test/Sema/compare.c Line 80: comparison of unsigned expression < 0 is always false
  File /build/clang/test/Sema/compare.c Line 88: comparison of unsigned expression < 0 is always false
  File /build/clang/test/Sema/compare.c Line 89: comparison of unsigned expression < 0 is always false
3 errors generated.

--

********************
FAIL: Clang :: Sema/outof-range-constant-compare.c (357 of 548)
******************** TEST 'Clang :: Sema/outof-range-constant-compare.c' FAILED ********************
Script:
--
/build/llvm-build-Clang-release/./bin/clang -cc1 -internal-isystem /build/llvm-build-Clang-release/lib/clang/6.0.0/include -nostdsysteminc -triple x86_64-apple-darwin -fsyntax-only -Wtautological-constant-out-of-range-compare -verify /build/clang/test/Sema/outof-range-constant-compare.c
--
Exit Code: 1

Command Output (stderr):
--
error: 'warning' diagnostics expected but not seen: 
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 163: comparison of unsigned expression < 0 is always false
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 169: comparison of unsigned expression >= 0 is always true
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 178: comparison of 0 <= unsigned expression is always true
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 180: comparison of 0 > unsigned expression is always false
error: 'warning' diagnostics seen but not expected: 
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 40: comparison of unsigned expression < 0 is always false
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 46: comparison of unsigned expression >= 0 is always true
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 55: comparison of 0 <= unsigned expression is always true
  File /build/clang/test/Sema/outof-range-constant-compare.c Line 57: comparison of 0 > unsigned expression is always false
8 errors generated.

--

********************
Testing Time: 2.05s
********************
Failing Tests (2):
    Clang :: Sema/compare.c
    Clang :: Sema/outof-range-constant-compare.c

  Expected Passes    : 546
  Unexpected Failures: 2
FAILED: tools/clang/test/CMakeFiles/check-clang-sema 
cd /build/llvm-build-Clang-release/tools/clang/test && /usr/bin/python2.7 /build/llvm/utils/lit/lit.py -sv --param clang_site_config=/build/llvm-build-Clang-release/tools/clang/test/lit.site.cfg /build/clang/test/Sema
ninja: build stopped: subcommand failed.

8876

OK :)

[rjmccall]

lib/Sema/SemaChecking.cpp

8589

What happens if you just move them into isNonBooleanUnsignedValue?

test/Sema/outof-range-constant-compare.c

41

Hmm. I think this should probably still warn under -Wtautological-compare, shouldn't it? The fact that it also warns under -Wsign-compare is something we should try to suppress, but it's definitely still a tautological comparison because of the promotion to an unsigned type.

[lebedev.ri]

test/Sema/outof-range-constant-compare.c

41

Hmm, actually, i guess so?

int value(void);
int main()
{
    int a = value();
    if (a <= 0x0000000000000000UL)
        return 0;	
}

|-BinaryOperator <line:5:9, col:14> '_Bool' '<='
| |-ImplicitCastExpr <col:9> 'unsigned long' <IntegralCast>
| | `-ImplicitCastExpr <col:9> 'int' <LValueToRValue>
| |   `-DeclRefExpr <col:9> 'int' lvalue Var 0x7028450 'a' 'int'
| `-IntegerLiteral <col:14> 'unsigned long' 0

Here, clang and gcc both only warn about different sign comparison.

[lebedev.ri]

Address review notes.

[rjmccall]

Great, thanks. Just a few tweaks.

docs/ReleaseNotes.rst

76

"now warns when comparing an unsigned integer and 0 regardless of whether the constant is signed or unsigned."

79

Probably better to be clear here: "now warns about comparing a signed integer and 0 when the signed integer is coerced to an unsigned type for the comparison."

83

I'm not sure this level of detail is necessary in release notes.

lib/Sema/SemaChecking.cpp

8877

Space after 'if', and I would suggest for the comment:

"If this is a tautological comparison, suppress -Wsign-compare."

[lebedev.ri]

Address review notes.

[rjmccall]

LGTM.

docs/ReleaseNotes.rst

79

That's great, thanks.

docs/ReleaseNotes.rst

- ``-Wpragma-pack-suspicious-include`` (disabled by default) warns on an
     directive prior to the ``#include``.
 
 - ``-Wobjc-messaging-id`` is a new, non-default warning that warns about
   message sends to unqualified ``id`` in Objective-C. This warning is useful
   for projects that would like to avoid any potential future compiler
   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`` warning on comparison of unsigned integer and
+  ``0`` constant was adjusted to warn regardless of whether the constant is
+  signed or unsigned.
+
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 - Bitrig OS was merged back into OpenBSD, so Bitrig support has been
   removed from Clang/LLVM.
 
 New Compiler Flags
 ------------------
 
 - --autocomplete was implemented to obtain a list of flags and its arguments. This is used for shell autocompletion.
 
 Deprecated Compiler Flags
 -------------------------
 

lib/Sema/SemaChecking.cpp

if (ICE->getCastKind() != CK_IntegralCast &&
         ICE->getCastKind() != CK_NoOp)
       break;
     E = ICE->getSubExpr();
   }
 
   return E->getType()->isEnumeralType();
 }
 
-void CheckTrivialUnsignedComparison(Sema &S, BinaryOperator *E) {
+bool isNonBooleanUnsignedValue(Expr *E) {
+  QualType Type = E->getType();
+
+  return !E->isKnownToHaveBooleanValue() && Type->isIntegerType() &&
+         !Type->isSignedIntegerType();
+}
+
+void CheckTautologicalComparisonWithZero(Sema &S, BinaryOperator *E) {
   // Disable warning in template instantiations.
   if (S.inTemplateInstantiation())
     return;
 
+  // bool values are handled by DiagnoseOutOfRangeComparison().
+
   BinaryOperatorKind op = E->getOpcode();
   if (E->isValueDependent())
     return;
 
-  if (op == BO_LT && IsZero(S, E->getRHS())) {
+  Expr *LHS = E->getLHS()->IgnoreParenImpCasts();
+  Expr *RHS = E->getRHS()->IgnoreParenImpCasts();
+
+  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();
   }
 }
 
 void DiagnoseOutOfRangeComparison(Sema &S, BinaryOperator *E, Expr *Constant,
                                   Expr *Other, const llvm::APSInt &Value,
                                   bool RhsConstant) {
   // Disable warning in template instantiations.
   if (S.inTemplateInstantiation())
     return;
 
   // TODO: Investigate using GetExprRange() to get tighter bounds
   // on the bit ranges.
   QualType OtherT = Other->getType();
   if (const auto *AT = OtherT->getAs<AtomicType>())
     OtherT = AT->getValueType();
   IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
   unsigned OtherWidth = OtherRange.Width;
 
   bool OtherIsBooleanType = Other->isKnownToHaveBooleanValue();
 
-  // 0 values are handled later by CheckTrivialUnsignedComparison().
+  // 0 values are handled later by CheckTautologicalComparisonWithZero().
   if ((Value == 0) && (!OtherIsBooleanType))
     return;
 
   BinaryOperatorKind op = E->getOpcode();
   bool IsTrue = true;
 
   // Used for diagnostic printout.
   enum {
if (IsRHSIntegralLiteral && !IsLHSIntegralLiteral)
         DiagnoseOutOfRangeComparison(S, E, RHS, LHS, RHSValue, true);
     else if (!IsRHSIntegralLiteral && IsLHSIntegralLiteral)
       DiagnoseOutOfRangeComparison(S, E, LHS, RHS, LHSValue, false);
     else
       IsComparisonConstant = 
         (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);
+  
+  CheckTautologicalComparisonWithZero(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;
   if (LHS->getType()->hasSignedIntegerRepresentation()) {
     assert(!RHS->getType()->hasSignedIntegerRepresentation() &&
            "unsigned comparison between two signed integer expressions?");
     signedOperand = LHS;
     unsignedOperand = RHS;
   } else if (RHS->getType()->hasSignedIntegerRepresentation()) {
     signedOperand = RHS;
     unsignedOperand = LHS;
   } else {
-    CheckTrivialUnsignedComparison(S, E);
     return AnalyzeImpConvsInComparison(S, E);
   }
 
   // Otherwise, calculate the effective range of the signed operand.
   IntRange signedRange = GetExprRange(S.Context, signedOperand);
 
   // Go ahead and analyze implicit conversions in the operands.  Note
   // that we skip the implicit conversions on both sides.
   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,
   // then reinterpreting the signed operand as unsigned will not
   // change the result of the comparison.
   if (E->isEqualityOp()) {
     unsigned comparisonWidth = S.Context.getIntWidth(T);
     IntRange unsignedRange = GetExprRange(S.Context, unsignedOperand);

test/Sema/outof-range-constant-compare.c

 // RUN: %clang_cc1 -triple x86_64-apple-darwin -fsyntax-only -Wtautological-constant-out-of-range-compare -verify %s 
 // rdar://12202422
 
 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)
+        return 0;
+    if (a <= 0x0000000000000000UL)
+        return 0;
+    if (a > 0x0000000000000000UL)
+        return 0;
+    if (a >= 0x0000000000000000UL)
+        return 0;
+
+    if (0x0000000000000000UL == a)
+        return 0;
+    if (0x0000000000000000UL != a)
+        return 0;
+    if (0x0000000000000000UL < a)
+        return 0;
+    if (0x0000000000000000UL <= a)
+        return 0;
+    if (0x0000000000000000UL > a)
+        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}}
         return 0;
     if (a < 0x1234567812345678L)  // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'int' is always true}}
         return 0;
     if (a <= 0x1234567812345678L)  // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'int' is always true}}
         return 0;
int main()
     if (0x1234567812345678L < s) // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'short' is always false}}
         return 0;
     if (0x1234567812345678L <= s) // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'short' is always false}}
         return 0;
     if (0x1234567812345678L > s) // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'short' is always true}}
         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;
     if (l != 0x1234567812345678L)
         return 0;
     if (l < 0x1234567812345678L)
         return 0;
     if (l <= 0x1234567812345678L)
int main()
     if (0x1234567812345678L >= l)
         return 0;
 
     unsigned un = 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 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)
         return 0;
     if (0x0000000000000000L != un)
         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
+    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;
+
+    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;
 
-    float dl = 0;
-    if (dl == 0x0000000000000000L) // no warning
+    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,
     no, 
     maybe
     };
     enum E e;
 
     if (e == 0x1234567812345678L) // expected-warning {{comparison of constant 1311768465173141112 with expression of type 'enum E' is always false}}
       return 0;
 
     return 1;
 }