From e92c0a12959f88b63b2a6346be0fe1a98d6aa8c5 Mon Sep 17 00:00:00 2001 From: Richard Trieu Date: Sat, 21 Sep 2019 02:37:10 +0000 Subject: Improve -Wtautological-overlap-compare Allow this warning to detect a larger number of constant values, including negative numbers, and handle non-int types better. Differential Revision: https://reviews.llvm.org/D66044 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@372448 91177308-0d34-0410-b5e6-96231b3b80d8 --- docs/ReleaseNotes.rst | 3 ++- lib/Analysis/CFG.cpp | 44 ++++++++++++++++++++++++++++++++++----- lib/Analysis/ReachableCode.cpp | 2 +- test/Analysis/cfg.cpp | 21 +++++++++++++++++++ test/Sema/warn-overlap.c | 17 +++++++++++++++ test/Sema/warn-unreachable.c | 2 +- test/SemaCXX/warn-unreachable.cpp | 5 +++-- 7 files changed, 84 insertions(+), 10 deletions(-) diff --git a/docs/ReleaseNotes.rst b/docs/ReleaseNotes.rst index 9716360a78..e04e568ddb 100644 --- a/docs/ReleaseNotes.rst +++ b/docs/ReleaseNotes.rst @@ -51,7 +51,8 @@ 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 ------------------------------------------------- diff --git a/lib/Analysis/CFG.cpp b/lib/Analysis/CFG.cpp index 1a45c9f532..86816997f0 100644 --- a/lib/Analysis/CFG.cpp +++ b/lib/Analysis/CFG.cpp @@ -70,11 +70,35 @@ static SourceLocation GetEndLoc(Decl *D) { 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(E)) { + if (CE->getCastKind() != CK_IntegralCast) + return false; + E = CE->getSubExpr(); + } + + // Allow negative numbers. + if (const auto *UO = dyn_cast(E)) { + if (UO->getOpcode() != UO_Minus) + return false; + E = UO->getSubExpr(); + } + + return isa(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(E)) + if (IsIntegerLiteralConstantExpr(E)) return E; if (auto *DR = dyn_cast(E->IgnoreParenImpCasts())) return isa(DR->getDecl()) ? DR : nullptr; @@ -121,11 +145,11 @@ tryNormalizeBinaryOperator(const BinaryOperator *B) { 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(E1) != isa(E2)) + if (isa(E1) != isa(E2)) return false; // Integer literal comparisons, regardless of literal type, are acceptable. - if (isa(E1)) + if (!isa(E1)) return true; // IntegerLiterals are handled above and only EnumConstantDecls are expected @@ -1081,6 +1105,10 @@ private: // * 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); @@ -1096,10 +1124,16 @@ private: 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); } diff --git a/lib/Analysis/ReachableCode.cpp b/lib/Analysis/ReachableCode.cpp index 2fea88ea2e..1dab8e309f 100644 --- a/lib/Analysis/ReachableCode.cpp +++ b/lib/Analysis/ReachableCode.cpp @@ -247,7 +247,7 @@ static bool isConfigurationValue(const Stmt *S, } case Stmt::UnaryOperatorClass: { const UnaryOperator *UO = cast(S); - if (UO->getOpcode() != UO_LNot) + if (UO->getOpcode() != UO_LNot && UO->getOpcode() != UO_Minus) return false; bool SilenceableCondValNotSet = SilenceableCondVal && SilenceableCondVal->getBegin().isInvalid(); diff --git a/test/Analysis/cfg.cpp b/test/Analysis/cfg.cpp index 9b0203e99e..a7d707ee20 100644 --- a/test/Analysis/cfg.cpp +++ b/test/Analysis/cfg.cpp @@ -547,6 +547,27 @@ int foo() { } } // 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() // CHECK: [B2 (ENTRY)] // CHECK-NEXT: Succs (1): B1 diff --git a/test/Sema/warn-overlap.c b/test/Sema/warn-overlap.c index 6299c511fe..d72e60755d 100644 --- a/test/Sema/warn-overlap.c +++ b/test/Sema/warn-overlap.c @@ -141,3 +141,20 @@ int returns(int x) { 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. +} diff --git a/test/Sema/warn-unreachable.c b/test/Sema/warn-unreachable.c index aec3b07021..cfac36a131 100644 --- a/test/Sema/warn-unreachable.c +++ b/test/Sema/warn-unreachable.c @@ -433,7 +433,7 @@ void wrapOneInFixit(struct StructWithPointer *s) { } void unaryOpNoFixit() { - if (- 1) + if (~ 1) return; // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]] unaryOpNoFixit(); // expected-warning {{code will never be executed}} } diff --git a/test/SemaCXX/warn-unreachable.cpp b/test/SemaCXX/warn-unreachable.cpp index a14e422686..c664c19128 100644 --- a/test/SemaCXX/warn-unreachable.cpp +++ b/test/SemaCXX/warn-unreachable.cpp @@ -393,6 +393,9 @@ void tautological_compare(bool x, int y) { 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(); @@ -408,6 +411,4 @@ void tautological_compare(bool x, int y) { else calledFun(); - if (y == -1 && y != -1) - calledFun(); } -- cgit v1.2.1