Extend -Wtautological-overlap-compare to accept negative values and integer conversions

[rtrieu]

Summary

-Wtautological-overlap-compare checks compound conditionals which compare the same value multiple times against a constant. This patch allows the constant to be a negative value or implicitly cast from another integer type. There's also a minor change to reachable code analysis so that the negative value can act like other values for purposes of silencing the warning.

[xbolva00]

Good improvement, thanks

[jfb]

test/Sema/warn-unreachable.c

437 ↗

(On Diff #214481)

Why this change?

[rtrieu]

test/Sema/warn-unreachable.c

437 ↗

(On Diff #214481)

​  if (y == -1 && y != -1)  // condition from warn-unreachable.cpp
  if (- 1)  // condition here

The test case below in warn-unreachable.cpp all have silence notes on them. For consistency, I wanted the new case involving (y == -1 && y != -1) to also have a silence case. The change to the reachability analysis to do that would also generate a silence note for (- 1). Since (- 1) now generates a silence, I picked a different unary operator that would not.

[NoQ]

Looks great, thanks! I'd appreciate direct CFG tests for the part that changes the CFG (cf. test/Analysis/cfg.cpp), but i don't insist. Let's make sure @jfb is happy and commit :)

lib/Analysis/CFG.cpp

1104 ↗

(On Diff #214481)

Typo: "subexpressions".

[jfb]

I haven't reviewed in depth, but generally this looks good.

[rtrieu]

In D66044#1626008, @NoQ wrote:

Looks great, thanks! I'd appreciate direct CFG tests for the part that changes the CFG (cf. test/Analysis/cfg.cpp), but i don't insist. Let's make sure @jfb is happy and commit :)

I added a test case to show the new unreachable node in the CFG.

[NoQ]

*appreciates CFG tests*

test/Analysis/cfg.cpp

504–506 ↗

(On Diff #215021)

Looks like runaway formatting.

cfe/trunk/docs/ReleaseNotes.rst

 Major New Features
 ------------------
 
 - ...
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-- ...
+- -Wtautological-overlap-compare will warn on negative numbers and non-int
+  types.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 - For X86 target, -march=skylake-avx512, -march=icelake-client,
   -march=icelake-server, -march=cascadelake, -march=cooperlake will default to
   not using 512-bit zmm registers in vectorized code unless 512-bit intrinsics
   are used in the source code. 512-bit operations are known to cause the CPUs

cfe/trunk/lib/Analysis/CFG.cpp

 
 static SourceLocation GetEndLoc(Decl *D) {
   if (VarDecl *VD = dyn_cast<VarDecl>(D))
     if (Expr *Ex = VD->getInit())
       return Ex->getSourceRange().getEnd();
   return D->getLocation();
 }
 
+/// Returns true on constant values based around a single IntegerLiteral.
+/// Allow for use of parentheses, integer casts, and negative signs.
+static bool IsIntegerLiteralConstantExpr(const Expr *E) {
+  // Allow parentheses
+  E = E->IgnoreParens();
+
+  // Allow conversions to different integer kind.
+  if (const auto *CE = dyn_cast<CastExpr>(E)) {
+    if (CE->getCastKind() != CK_IntegralCast)
+      return false;
+    E = CE->getSubExpr();
+  }
+
+  // Allow negative numbers.
+  if (const auto *UO = dyn_cast<UnaryOperator>(E)) {
+    if (UO->getOpcode() != UO_Minus)
+      return false;
+    E = UO->getSubExpr();
+  }
+
+  return isa<IntegerLiteral>(E);
+}
+
 /// Helper for tryNormalizeBinaryOperator. Attempts to extract an IntegerLiteral
-/// or EnumConstantDecl from the given Expr. If it fails, returns nullptr.
+/// constant expression or EnumConstantDecl from the given Expr. If it fails,
+/// returns nullptr.
 static const Expr *tryTransformToIntOrEnumConstant(const Expr *E) {
   E = E->IgnoreParens();
-  if (isa<IntegerLiteral>(E))
+  if (IsIntegerLiteralConstantExpr(E))
     return E;
   if (auto *DR = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts()))
     return isa<EnumConstantDecl>(DR->getDecl()) ? DR : nullptr;
   return nullptr;
 }
 
 /// Tries to interpret a binary operator into `Decl Op Expr` form, if Expr is
 /// an integer literal or an enum constant.

 /// `Foo` and `Bar` are either of the same enumeration type, or both integer
 /// literals.
 ///
 /// It's an error to pass this arguments that are not either IntegerLiterals
 /// or DeclRefExprs (that have decls of type EnumConstantDecl)
 static bool areExprTypesCompatible(const Expr *E1, const Expr *E2) {
   // User intent isn't clear if they're mixing int literals with enum
   // constants.
-  if (isa<IntegerLiteral>(E1) != isa<IntegerLiteral>(E2))
+  if (isa<DeclRefExpr>(E1) != isa<DeclRefExpr>(E2))
     return false;
 
   // Integer literal comparisons, regardless of literal type, are acceptable.
-  if (isa<IntegerLiteral>(E1))
+  if (!isa<DeclRefExpr>(E1))
     return true;
 
   // IntegerLiterals are handled above and only EnumConstantDecls are expected
   // beyond this point
   assert(isa<DeclRefExpr>(E1) && isa<DeclRefExpr>(E2));
   auto *Decl1 = cast<DeclRefExpr>(E1)->getDecl();
   auto *Decl2 = cast<DeclRefExpr>(E2)->getDecl();
 
TryResult checkIncorrectLogicOperator(const BinaryOperator *B) {
 
     // Check whether expression is always true/false by evaluating the following
     // * variable x is less than the smallest literal.
     // * variable x is equal to the smallest literal.
     // * Variable x is between smallest and largest literal.
     // * Variable x is equal to the largest literal.
     // * Variable x is greater than largest literal.
     bool AlwaysTrue = true, AlwaysFalse = true;
+    // Track value of both subexpressions.  If either side is always
+    // true/false, another warning should have already been emitted.
+    bool LHSAlwaysTrue = true, LHSAlwaysFalse = true;
+    bool RHSAlwaysTrue = true, RHSAlwaysFalse = true;
     for (const llvm::APSInt &Value : Values) {
       TryResult Res1, Res2;
       Res1 = analyzeLogicOperatorCondition(BO1, Value, L1);
       Res2 = analyzeLogicOperatorCondition(BO2, Value, L2);
 
       if (!Res1.isKnown() || !Res2.isKnown())
         return {};
 
       if (B->getOpcode() == BO_LAnd) {
         AlwaysTrue &= (Res1.isTrue() && Res2.isTrue());
         AlwaysFalse &= !(Res1.isTrue() && Res2.isTrue());
       } else {
         AlwaysTrue &= (Res1.isTrue() || Res2.isTrue());
         AlwaysFalse &= !(Res1.isTrue() || Res2.isTrue());
       }
+
+      LHSAlwaysTrue &= Res1.isTrue();
+      LHSAlwaysFalse &= Res1.isFalse();
+      RHSAlwaysTrue &= Res2.isTrue();
+      RHSAlwaysFalse &= Res2.isFalse();
     }
 
     if (AlwaysTrue || AlwaysFalse) {
-      if (BuildOpts.Observer)
+      if (!LHSAlwaysTrue && !LHSAlwaysFalse && !RHSAlwaysTrue &&
+          !RHSAlwaysFalse && BuildOpts.Observer)
         BuildOpts.Observer->compareAlwaysTrue(B, AlwaysTrue);
       return TryResult(AlwaysTrue);
     }
     return {};
   }
 
   /// Try and evaluate an expression to an integer constant.
   bool tryEvaluate(Expr *S, Expr::EvalResult &outResult) {

cfe/trunk/lib/Analysis/ReachableCode.cpp

case Stmt::BinaryOperatorClass: {
       IncludeIntegers &= (B->isLogicalOp() || B->isComparisonOp());
       return isConfigurationValue(B->getLHS(), PP, SilenceableCondVal,
                                   IncludeIntegers) ||
              isConfigurationValue(B->getRHS(), PP, SilenceableCondVal,
                                   IncludeIntegers);
     }
     case Stmt::UnaryOperatorClass: {
       const UnaryOperator *UO = cast<UnaryOperator>(S);
-      if (UO->getOpcode() != UO_LNot)
+      if (UO->getOpcode() != UO_LNot && UO->getOpcode() != UO_Minus)
         return false;
       bool SilenceableCondValNotSet =
           SilenceableCondVal && SilenceableCondVal->getBegin().isInvalid();
       bool IsSubExprConfigValue =
           isConfigurationValue(UO->getSubExpr(), PP, SilenceableCondVal,
                                IncludeIntegers, WrappedInParens);
       // Update the silenceable condition value source range only if the range
       // was set directly by the child expression.

cfe/trunk/test/Analysis/cfg.cpp

return ({
         ;
       0;
     });
   else
     return 0;
 }
 } // namespace statement_expression_in_return
 
+// CHECK-LABEL: int overlap_compare(int x)
+// CHECK: [B2]
+// CHECK-NEXT:   1: 1
+// CHECK-NEXT:   2: return [B2.1];
+// CHECK-NEXT:   Preds (1): B3(Unreachable)
+// CHECK-NEXT:   Succs (1): B0
+// CHECK: [B3]
+// CHECK-NEXT:   1: x
+// CHECK-NEXT:   2: [B3.1] (ImplicitCastExpr, LValueToRValue, int)
+// CHECK-NEXT:   3: 5
+// CHECK-NEXT:   4: [B3.2] > [B3.3]
+// CHECK-NEXT:   T: if [B4.5] && [B3.4]
+// CHECK-NEXT:   Preds (1): B4
+// CHECK-NEXT:   Succs (2): B2(Unreachable) B1
+int overlap_compare(int x) {
+  if (x == -1 && x > 5)
+    return 1;
+
+  return 2;
+}
+
 // CHECK-LABEL: template<> int *PR18472<int>()
 // CHECK: [B2 (ENTRY)]
 // CHECK-NEXT:   Succs (1): B1
 // CHECK: [B1]
 // CHECK-NEXT:   1: 0
 // CHECK-NEXT:   2: [B1.1] (ImplicitCastExpr, NullToPointer, PR18472_t)
 // CHECK-NEXT:   3: (PR18472_t)[B1.2] (CStyleCastExpr, NoOp, PR18472_t)
 // CHECK-NEXT:   4: CFGNewAllocator(int *)

cfe/trunk/test/Sema/warn-overlap.c

int returns(int x) {
   return x != 1 || x != 3;
   // expected-warning@-1{{overlapping comparisons always evaluate to true}}
   return x == 1 && x == 2;
   // expected-warning@-1{{overlapping comparisons always evaluate to false}}
 
   return x < 1 || x != 0;
   // expected-warning@-1{{overlapping comparisons always evaluate to true}}
 }
+
+int integer_conversion(unsigned x, int y) {
+  return x > 4 || x < 10;
+  // expected-warning@-1{{overlapping comparisons always evaluate to true}}
+  return y > 4u || y < 10u;
+  // expected-warning@-1{{overlapping comparisons always evaluate to true}}
+}
+
+int negative_compare(int x) {
+  return x > -1 || x < 1;
+  // expected-warning@-1{{overlapping comparisons always evaluate to true}}
+}
+
+int no_warning(unsigned x) {
+  return x >= 0 || x == 1;
+  // no warning since "x >= 0" is caught by a different tautological warning.
+}

cfe/trunk/test/Sema/warn-unreachable.c

 void wrapOneInFixit(struct StructWithPointer *s) {
   if (!s->p || 1) // expected-note {{silence by adding parentheses to mark code as explicitly dead}}
     return; // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:16}:"/* DISABLES CODE */ ("
             // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:")"
   wrapOneInFixit(s); // expected-warning {{code will never be executed}}
 }
 
 void unaryOpNoFixit() {
-  if (- 1)
+  if (~ 1)
     return; // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
   unaryOpNoFixit(); // expected-warning {{code will never be executed}}
 }
 
 void unaryOpStrictFixit(struct StructWithPointer *s) {
   if (!(s->p && 0)) // expected-note {{silence by adding parentheses to mark code as explicitly dead}}
     return; // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:17-[[@LINE-1]]:17}:"/* DISABLES CODE */ ("
             // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:")"

cfe/trunk/test/SemaCXX/warn-unreachable.cpp

void tautological_compare(bool x, int y) {
   if (y > 5 && y < 4)   // expected-note {{silence}}
     calledFun();        // expected-warning {{will never be executed}}
 
   if (y < 10 || y > 5)  // expected-note {{silence}}
     calledFun();
   else
     calledFun();        // expected-warning {{will never be executed}}
 
+  if (y == -1 && y != -1)  // expected-note {{silence}}
+    calledFun();        // expected-warning {{will never be executed}}
+
   // TODO: Extend warning to the following code:
   if (x < -1)
     calledFun();
   if (x == -1)
     calledFun();
 
   if (x != -1)
     calledFun();
   else
     calledFun();
   if (-1 > x)
     calledFun();
   else
     calledFun();
 
-  if (y == -1 && y != -1)
-    calledFun();
 }