Add -Wtautological-value-range-compare warning.

[rsmith]

Summary

This warning diagnoses cases where an expression is compared to a
constant, and the comparison is tautological due to the form of the
expression (but not merely due to its type). This applies in cases such
as comparisons of bit-fields and the result of bit-masks.

The new warning is added to the Clang diagnostic group
-Wtautological-constant-in-range-compare but not to the
formerly-equivalent GCC-compatibility diagnostic group -Wtype-limits,
which retains its old meaning of diagnosing only tautological
comparisons to extremal values of a type (eg, int > INT_MAX).

[xbolva00]

Could this solve https://bugs.llvm.org/show_bug.cgi?id=40921 ?

[rsmith]

Unbreak test that was autobroken by lint.

[rsmith]

This comment was removed by rsmith.

[rsmith]

In D85256#2194892, @xbolva00 wrote:

Could this solve https://bugs.llvm.org/show_bug.cgi?id=40921 ?

Not directly; GetExprRange in SemaChecking apparently intentionally treats casts as producing an arbitrary value of the destination type:

// I think we only want to look through implicit casts here; if the
// user has an explicit widening cast, we should treat the value as
// being of the new, wider type.

I'm not entirely convinced that what the comment describes is the best approach. If we revisited that choice then, with this patch, we would indeed diagnose the PR40921 case. But I think that and this are separate changes and I'd prefer not to combine them.

[xbolva00]

But I think that and this are separate changes and I'd prefer not to combine them.

Yeah, right.

[rtrieu]

LGTM

[sberg]

I think this generates a false positive with test.cc

enum E { E1 = 1, E2 = 2 };
bool f(E e) { return ((e & E1) ? 1 : 0) + ((e & E2) ? 1 : 0) > 1; }

and clang++ -fsyntax-only -Wtautological-value-range-compare test.cc

test.cc:2:62: warning: result of comparison of 1-bit unsigned value > 1 is always false [-Wtautological-value-range-compare]
bool f(E e) { return ((e & E1) ? 1 : 0) + ((e & E2) ? 1 : 0) > 1; }
                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~

[rsmith]

In D85256#2209211, @sberg wrote:

I think this generates a false positive with test.cc

enum E { E1 = 1, E2 = 2 };
bool f(E e) { return ((e & E1) ? 1 : 0) + ((e & E2) ? 1 : 0) > 1; }

and clang++ -fsyntax-only -Wtautological-value-range-compare test.cc

test.cc:2:62: warning: result of comparison of 1-bit unsigned value > 1 is always false [-Wtautological-value-range-compare]
bool f(E e) { return ((e & E1) ? 1 : 0) + ((e & E2) ? 1 : 0) > 1; }
                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~

This appears to be a general problem: the GetExprRange mechanism in SemaChecking miscomputes the ranges for +, *, and - expressions, and we'll get them wrong for all warnings that use that mechanism :-(

[rsmith]

In D85256#2211129, @rsmith wrote:

This appears to be a general problem: the GetExprRange mechanism in SemaChecking miscomputes the ranges for +, *, and - expressions, and we'll get them wrong for all warnings that use that mechanism :-(

See https://reviews.llvm.org/D85778 for a fix. We were intentionally miscomputing the expression ranges in order to give conservative narrowing-conversion results for cases like char a = some_char + 1; (which is technically lossy because the RHS could be CHAR_MAX + 1, but is probably fine), and this caused us to similarly compute that (a & 1) + (b & 1) was in the range [0, 1], not [0, 2].

clang/include/clang/Basic/DiagnosticGroups.td

 def StringCompare : DiagGroup<"string-compare">;
 def StringPlusInt : DiagGroup<"string-plus-int">;
 def StringPlusChar : DiagGroup<"string-plus-char">;
 def StrncatSize : DiagGroup<"strncat-size">;
 def IntInBoolContext : DiagGroup<"int-in-bool-context">;
 def TautologicalTypeLimitCompare : DiagGroup<"tautological-type-limit-compare">;
 def TautologicalUnsignedZeroCompare : DiagGroup<"tautological-unsigned-zero-compare">;
 def TautologicalUnsignedEnumZeroCompare : DiagGroup<"tautological-unsigned-enum-zero-compare">;
-def TautologicalInRangeCompare : DiagGroup<"tautological-constant-in-range-compare",
-                                           [TautologicalTypeLimitCompare,
-                                            TautologicalUnsignedZeroCompare,
-                                            TautologicalUnsignedEnumZeroCompare]>;
-// For compatibility with GCC; -Wtype-limits = -Wtautological-constant-in-range-compare
-def TypeLimits : DiagGroup<"type-limits", [TautologicalInRangeCompare]>;
+// For compatibility with GCC. Tautological comparison warnings for constants
+// that are an extremal value of the type.
+def TypeLimits : DiagGroup<"type-limits", [TautologicalTypeLimitCompare,
+// Additional tautological comparison warnings based on the expression, not
+// only on its type.
+def TautologicalValueRangeCompare : DiagGroup<"tautological-value-range-compare">;
+def TautologicalInRangeCompare : DiagGroup<"tautological-constant-in-range-compare",
+                                           [TypeLimits, TautologicalValueRangeCompare]>;
 def TautologicalOutOfRangeCompare : DiagGroup<"tautological-constant-out-of-range-compare">;
 def TautologicalConstantCompare : DiagGroup<"tautological-constant-compare",
                                             [TautologicalOutOfRangeCompare]>;
 def TautologicalPointerCompare : DiagGroup<"tautological-pointer-compare">;
 def TautologicalOverlapCompare : DiagGroup<"tautological-overlap-compare">;
 def TautologicalBitwiseCompare : DiagGroup<"tautological-bitwise-compare">;
 def TautologicalUndefinedCompare : DiagGroup<"tautological-undefined-compare">;
 def TautologicalObjCBoolCompare : DiagGroup<"tautological-objc-bool-compare">;

 def Switch         : DiagGroup<"switch">;
 def ImplicitFallthroughPerFunction :
   DiagGroup<"implicit-fallthrough-per-function">;
 def ImplicitFallthrough  : DiagGroup<"implicit-fallthrough",
                                      [ImplicitFallthroughPerFunction]>;
 def InvalidPPToken : DiagGroup<"invalid-pp-token">;
 def Trigraphs      : DiagGroup<"trigraphs">;
 
-def : DiagGroup<"type-limits">;
 def UndefinedReinterpretCast : DiagGroup<"undefined-reinterpret-cast">;
 def ReinterpretBaseClass : DiagGroup<"reinterpret-base-class">;
 def Unicode  : DiagGroup<"unicode">;
 def UninitializedMaybe : DiagGroup<"conditional-uninitialized">;
 def UninitializedSometimes : DiagGroup<"sometimes-uninitialized">;
 def UninitializedStaticSelfInit : DiagGroup<"static-self-init">;
 def UninitializedConstReference : DiagGroup<"uninitialized-const-reference">;
 def Uninitialized  : DiagGroup<"uninitialized", [UninitializedSometimes,

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_tautological_constant_compare : Warning<
   "result of comparison %select{%3|%1}0 %2 "
   "%select{%1|%3}0 is always %4">,
   InGroup<TautologicalTypeLimitCompare>, DefaultIgnore;
 def warn_tautological_compare_objc_bool : Warning<
   "result of comparison of constant %0 with expression of type 'BOOL'"
   " is always %1, as the only well defined values for 'BOOL' are YES and NO">,
   InGroup<TautologicalObjCBoolCompare>;
+def subst_int_range : TextSubstitution<"%0-bit %select{signed|unsigned}1 value">;
+def warn_tautological_compare_value_range : Warning<
+  "result of comparison of "
+  "%select{%4|%sub{subst_int_range}1,2}0 %3 "
+  "%select{%sub{subst_int_range}1,2|%4}0 is always %5">,
+  InGroup<TautologicalValueRangeCompare>, DefaultIgnore;
 
 def warn_mixed_sign_comparison : Warning<
   "comparison of integers of different signs: %0 and %1">,
   InGroup<SignCompare>, DefaultIgnore;
 def warn_out_of_range_compare : Warning<
   "result of comparison of %select{constant %0|true|false}1 with "
   "%select{expression of type %2|boolean expression}3 is always %4">,
   InGroup<TautologicalOutOfRangeCompare>;

clang/lib/Sema/SemaChecking.cpp

static bool CheckTautologicalComparison(Sema &S, BinaryOperator *E,
   //    the enumeration. In such a case, we should warn about the cast
   //    to enumeration type, not about the comparison.
   //  - If the constant is the maximum / minimum in-range value. For an
   //    enumeratin type, such comparisons can be meaningful and useful.
   if (Constant->getType()->isEnumeralType() &&
       S.Context.hasSameUnqualifiedType(Constant->getType(), Other->getType()))
     return false;
 
-  // TODO: Investigate using GetExprRange() to get tighter bounds
-  // on the bit ranges.
+  IntRange OtherValueRange =
+      GetExprRange(S.Context, Other, S.isConstantEvaluated());
+
   QualType OtherT = Other->getType();
   if (const auto *AT = OtherT->getAs<AtomicType>())
     OtherT = AT->getValueType();
-  IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
+  IntRange OtherTypeRange = IntRange::forValueOfType(S.Context, OtherT);
 
   // Special case for ObjC BOOL on targets where its a typedef for a signed char
-  // (Namely, macOS).
+  // (Namely, macOS). FIXME: IntRange::forValueOfType should do this.
   bool IsObjCSignedCharBool = S.getLangOpts().ObjC &&
                               S.NSAPIObj->isObjCBOOLType(OtherT) &&
                               OtherT->isSpecificBuiltinType(BuiltinType::SChar);
 
   // Whether we're treating Other as being a bool because of the form of
   // expression despite it having another type (typically 'int' in C).
   bool OtherIsBooleanDespiteType =
       !OtherT->isBooleanType() && Other->isKnownToHaveBooleanValue();
   if (OtherIsBooleanDespiteType || IsObjCSignedCharBool)
-    OtherRange = IntRange::forBoolType();
+    OtherTypeRange = OtherValueRange = IntRange::forBoolType();
 
-  // Determine the promoted range of the other type and see if a comparison of
-  // the constant against that range is tautological.
-  PromotedRange OtherPromotedRange(OtherRange, Value.getBitWidth(),
+  // Check if all values in the range of possible values of this expression
+  // lead to the same comparison outcome.
+  PromotedRange OtherPromotedValueRange(OtherValueRange, Value.getBitWidth(),
                                         Value.isUnsigned());
-  auto Cmp = OtherPromotedRange.compare(Value);
+  auto Cmp = OtherPromotedValueRange.compare(Value);
   auto Result = PromotedRange::constantValue(E->getOpcode(), Cmp, RhsConstant);
   if (!Result)
     return false;
 
+  // Also consider the range determined by the type alone. This allows us to
+  // classify the warning under the proper diagnostic group.
+  bool TautologicalTypeCompare = false;
+  {
+    PromotedRange OtherPromotedTypeRange(OtherTypeRange, Value.getBitWidth(),
+                                         Value.isUnsigned());
+    auto TypeCmp = OtherPromotedTypeRange.compare(Value);
+    if (auto TypeResult = PromotedRange::constantValue(E->getOpcode(), TypeCmp,
+                                                       RhsConstant)) {
+      TautologicalTypeCompare = true;
+      Cmp = TypeCmp;
+      Result = TypeResult;
+    }
+  }
+
   // Suppress the diagnostic for an in-range comparison if the constant comes
   // from a macro or enumerator. We don't want to diagnose
   //
   //   some_long_value <= INT_MAX
   //
   // when sizeof(int) == sizeof(long).
   bool InRange = Cmp & PromotedRange::InRangeFlag;
   if (InRange && IsEnumConstOrFromMacro(S, Constant))
if (ED) {
     OS << '\'' << *ED << "' (" << Value << ")";
   } else if (auto *BL = dyn_cast<ObjCBoolLiteralExpr>(
                Constant->IgnoreParenImpCasts())) {
     OS << (BL->getValue() ? "YES" : "NO");
   } else {
     OS << Value;
   }
 
+  if (!TautologicalTypeCompare) {
+    S.Diag(E->getOperatorLoc(), diag::warn_tautological_compare_value_range)
+        << RhsConstant << OtherValueRange.Width << OtherValueRange.NonNegative
+        << E->getOpcodeStr() << OS.str() << *Result
+        << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
+    return true;
+  }
+
   if (IsObjCSignedCharBool) {
     S.DiagRuntimeBehavior(E->getOperatorLoc(), E,
                           S.PDiag(diag::warn_tautological_compare_objc_bool)
                               << OS.str() << *Result);
     return true;
   }
 
   // FIXME: We use a somewhat different formatting for the in-range cases and

clang/test/Sema/tautological-constant-compare.c

-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtautological-constant-in-range-compare -DTEST -verify %s
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtautological-constant-in-range-compare -DTEST -verify -x c++ %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtautological-constant-in-range-compare -DTEST=2 -verify %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtautological-constant-in-range-compare -DTEST=2 -verify -x c++ %s
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtautological-type-limit-compare -DTEST -verify %s
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtautological-type-limit-compare -DTEST -verify -x c++ %s
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtype-limits -DTEST -verify %s
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wtype-limits -DTEST -verify -x c++ %s
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -verify %s
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wextra -Wno-sign-compare -verify -x c++ %s
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify %s
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -Wall -verify -x c++ %s
if (maybe < e)
       return 0;
   if (maybe <= e)
       return 0;
   if (maybe > e)
       return 0;
   if (maybe >= e)
       return 0;
 
-  // For the time being, use the declared type of bit-fields rather than their
-  // length when determining whether a value is in-range.
-  // FIXME: Reconsider this.
+  // We only warn on out-of-range bitfields and expressions with limited range
+  // under -Wtantological-in-range-compare, not under -Wtype-limits, because
+  // the warning is not based on the type alone.
   struct A {
     int a : 3;
     unsigned b : 3;
     long c : 3;
     unsigned long d : 3;
   } a;
   if (a.a < 3) {}
-  if (a.a < 4) {}
+  if (a.a < 4) {
+  } // #bitfield1
   if (a.b < 7) {}
-  if (a.b < 8) {}
+  if (a.b < 8) {
+  } // #bitfield2
   if (a.c < 3) {}
-  if (a.c < 4) {}
+  if (a.c < 4) {
+  } // #bitfield3
   if (a.d < 7) {}
-  if (a.d < 8) {}
+  if (a.d < 8) {
+  } // #bitfield4
+#if TEST == 2
+  // expected-warning@#bitfield1 {{comparison of 3-bit signed value < 4 is always true}}
+  // expected-warning@#bitfield2 {{comparison of 3-bit unsigned value < 8 is always true}}
+  // expected-warning@#bitfield3 {{comparison of 3-bit signed value < 4 is always true}}
+  // expected-warning@#bitfield4 {{comparison of 3-bit unsigned value < 8 is always true}}
+#endif
+
+  if ((s & 0xff) < 0) {
+  } // #valuerange1
+  if ((s & 0xff) < 1) {
+  }
+  if ((s & -3) < -4) {
+  } // #valuerange2
+  if ((s & -3) < -3) {
+  }
+  if ((s & -3) < 4u) {
+  } // (true if s non-negative)
+  if ((s & -3) > 4u) {
+  } // (true if s negative)
+  if ((s & -3) == 4u) {
+  } // #valuerange3 (never true)
+  if ((s & -3) == 3u) {
+  }
+  if ((s & -3) == -5u) {
+  } // #valuerange4
+  if ((s & -3) == -4u) {
+  }
+#if TEST == 2
+  // expected-warning@#valuerange1 {{comparison of 8-bit unsigned value < 0 is always false}}
+  // expected-warning@#valuerange2 {{comparison of 3-bit signed value < -4 is always false}}
+  // expected-warning@#valuerange3 {{comparison of 3-bit signed value == 4 is always false}}
+  // expected-warning@#valuerange4 {{comparison of 3-bit signed value == 4294967291 is always false}}
+#endif
 
   return 1;
 }