llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Yutong Zhu (YutongZhuu) <details> <summary>Changes</summary> This PR attempts to improve the diagnostics flag ``-Wtautological-overlap-compare`` (#<!-- -->13473). I have added code to warn about float-point literals and character literals. I have also changed the warning message for the non-overlapping case to provide a more correct hint to the user. --- Patch is 22.65 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/133653.diff 5 Files Affected: - (modified) clang/docs/ReleaseNotes.rst (+3) - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+4-1) - (modified) clang/lib/Analysis/CFG.cpp (+143-70) - (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+6-3) - (modified) clang/test/Sema/warn-overlap.c (+87-21) ``````````diff diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 2a1c5ee2d788e..e6a31072567e7 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -256,6 +256,9 @@ Improvements to Clang's diagnostics - Improve the diagnostics for shadows template parameter to report correct location (#GH129060). +- Improved the ``-Wtautological-overlap-compare`` diagnostics to warn about overlapping and non-overlapping ranges involving character literals and floating-point literals. + The warning message for non-overlapping cases has also been improved (#GH13473). + Improvements to Clang's time-trace ---------------------------------- diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 86c9c955c1c78..493a66df982c4 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -10398,7 +10398,10 @@ def warn_tautological_negation_or_compare: Warning< "'||' of a value and its negation always evaluates to true">, InGroup<TautologicalNegationCompare>, DefaultIgnore; def warn_tautological_overlap_comparison : Warning< - "overlapping comparisons always evaluate to %select{false|true}0">, + "overlapping comparisons always evaluate to true">, + InGroup<TautologicalOverlapCompare>, DefaultIgnore; +def warn_tautological_non_overlap_comparison : Warning< + "non-overlapping comparisons always evaluate to false">, InGroup<TautologicalOverlapCompare>, DefaultIgnore; def warn_depr_array_comparison : Warning< "comparison between two arrays is deprecated; " diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index 9af1e915482da..c6b7c49d4cb4a 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -70,13 +70,11 @@ 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. -/// FIXME: it would be good to unify this function with -/// getIntegerLiteralSubexpressionValue at some point given the similarity -/// between the functions. +/// Returns true on constant values based around a single IntegerLiteral, +/// CharacterLiteral, or FloatingLiteral. Allow for use of parentheses, integer +/// casts, and negative signs. -static bool IsIntegerLiteralConstantExpr(const Expr *E) { +static bool IsLiteralConstantExpr(const Expr *E) { // Allow parentheses E = E->IgnoreParens(); @@ -93,16 +91,16 @@ static bool IsIntegerLiteralConstantExpr(const Expr *E) { return false; E = UO->getSubExpr(); } - - return isa<IntegerLiteral>(E); + return isa<IntegerLiteral>(E) || isa<CharacterLiteral>(E) || + isa<FloatingLiteral>(E); } /// Helper for tryNormalizeBinaryOperator. Attempts to extract an IntegerLiteral /// constant expression or EnumConstantDecl from the given Expr. If it fails, /// returns nullptr. -static const Expr *tryTransformToIntOrEnumConstant(const Expr *E) { +static const Expr *tryTransformToLiteralConstants(const Expr *E) { E = E->IgnoreParens(); - if (IsIntegerLiteralConstantExpr(E)) + if (IsLiteralConstantExpr(E)) return E; if (auto *DR = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts())) return isa<EnumConstantDecl>(DR->getDecl()) ? DR : nullptr; @@ -119,7 +117,7 @@ tryNormalizeBinaryOperator(const BinaryOperator *B) { BinaryOperatorKind Op = B->getOpcode(); const Expr *MaybeDecl = B->getLHS(); - const Expr *Constant = tryTransformToIntOrEnumConstant(B->getRHS()); + const Expr *Constant = tryTransformToLiteralConstants(B->getRHS()); // Expr looked like `0 == Foo` instead of `Foo == 0` if (Constant == nullptr) { // Flip the operator @@ -133,7 +131,7 @@ tryNormalizeBinaryOperator(const BinaryOperator *B) { Op = BO_GE; MaybeDecl = B->getRHS(); - Constant = tryTransformToIntOrEnumConstant(B->getLHS()); + Constant = tryTransformToLiteralConstants(B->getLHS()); } return std::make_tuple(MaybeDecl, Op, Constant); @@ -1104,6 +1102,27 @@ class CFGBuilder { } } + TryResult analyzeLogicOperatorCondition(BinaryOperatorKind Relation, + const llvm::APFloat &Value1, + const llvm::APFloat &Value2) { + switch (Relation) { + default: + return TryResult(); + case BO_EQ: + return TryResult(Value1.compare(Value2) == llvm::APFloat::cmpEqual); + case BO_NE: + return TryResult(Value1.compare(Value2) != llvm::APFloat::cmpEqual); + case BO_LT: + return TryResult(Value1.compare(Value2) == llvm::APFloat::cmpLessThan); + case BO_LE: + return TryResult(Value1.compare(Value2) != llvm::APFloat::cmpGreaterThan); + case BO_GT: + return TryResult(Value1.compare(Value2) == llvm::APFloat::cmpGreaterThan); + case BO_GE: + return TryResult(Value1.compare(Value2) != llvm::APFloat::cmpLessThan); + } + } + /// There are two checks handled by this function: /// 1. Find a law-of-excluded-middle or law-of-noncontradiction expression /// e.g. if (x || !x), if (x && !x) @@ -1171,81 +1190,135 @@ class CFGBuilder { return {}; Expr::EvalResult L1Result, L2Result; - if (!NumExpr1->EvaluateAsInt(L1Result, *Context) || - !NumExpr2->EvaluateAsInt(L2Result, *Context)) - return {}; - - llvm::APSInt L1 = L1Result.Val.getInt(); - llvm::APSInt L2 = L2Result.Val.getInt(); - - // Can't compare signed with unsigned or with different bit width. - if (L1.isSigned() != L2.isSigned() || L1.getBitWidth() != L2.getBitWidth()) + llvm::APFloat L1ResultFloat(llvm::APFloat::IEEEdouble()), + L2ResultFloat(llvm::APFloat::IEEEdouble()); + bool IsInt1 = NumExpr1->EvaluateAsInt(L1Result, *Context); + bool IsInt2 = NumExpr2->EvaluateAsInt(L2Result, *Context); + bool IsFloat1 = NumExpr1->EvaluateAsFloat(L1ResultFloat, *Context); + bool IsFloat2 = NumExpr2->EvaluateAsFloat(L2ResultFloat, *Context); + + if ((!IsInt1 || !IsInt2) && (!IsFloat1 || !IsFloat2)) return {}; - // Values that will be used to determine if result of logical - // operator is always true/false - const llvm::APSInt Values[] = { - // Value less than both Value1 and Value2 - llvm::APSInt::getMinValue(L1.getBitWidth(), L1.isUnsigned()), - // L1 - L1, - // Value between Value1 and Value2 - ((L1 < L2) ? L1 : L2) + llvm::APSInt(llvm::APInt(L1.getBitWidth(), 1), - L1.isUnsigned()), - // L2 - L2, - // Value greater than both Value1 and Value2 - llvm::APSInt::getMaxValue(L1.getBitWidth(), L1.isUnsigned()), - }; + // Handle integer comparison + if (IsInt1 && IsInt2) { + llvm::APSInt L1 = L1Result.Val.getInt(); + llvm::APSInt L2 = L2Result.Val.getInt(); - // 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()) + // Can't compare signed with unsigned or with different bit width. + if (L1.isSigned() != L2.isSigned() || + L1.getBitWidth() != L2.getBitWidth()) 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()); + // Values that will be used to determine if result of logical + // operator is always true/false + const llvm::APSInt Values[] = { + // Value less than both Value1 and Value2 + llvm::APSInt::getMinValue(L1.getBitWidth(), L1.isUnsigned()), + // L1 + L1, + // Value between Value1 and Value2 + ((L1 < L2) ? L1 : L2) + + llvm::APSInt(llvm::APInt(L1.getBitWidth(), 1), L1.isUnsigned()), + // L2 + L2, + // Value greater than both Value1 and Value2 + llvm::APSInt::getMaxValue(L1.getBitWidth(), L1.isUnsigned()), + }; + + // 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(); } - LHSAlwaysTrue &= Res1.isTrue(); - LHSAlwaysFalse &= Res1.isFalse(); - RHSAlwaysTrue &= Res2.isTrue(); - RHSAlwaysFalse &= Res2.isFalse(); + if (AlwaysTrue || AlwaysFalse) { + if (!LHSAlwaysTrue && !LHSAlwaysFalse && !RHSAlwaysTrue && + !RHSAlwaysFalse && BuildOpts.Observer) + BuildOpts.Observer->compareAlwaysTrue(B, AlwaysTrue); + return TryResult(AlwaysTrue); + } + return {}; } - if (AlwaysTrue || AlwaysFalse) { - if (!LHSAlwaysTrue && !LHSAlwaysFalse && !RHSAlwaysTrue && - !RHSAlwaysFalse && BuildOpts.Observer) - BuildOpts.Observer->compareAlwaysTrue(B, AlwaysTrue); - return TryResult(AlwaysTrue); + // Handle float comparison, the logic is similar to integer comparison + if (IsFloat1 && IsFloat2) { + llvm::APFloat MidValue = L1ResultFloat; + MidValue.add(L2ResultFloat, llvm::APFloat::rmNearestTiesToEven); + MidValue.divide(llvm::APFloat(2.0), llvm::APFloat::rmNearestTiesToEven); + + const llvm::APFloat Values[] = { + llvm::APFloat::getSmallest(L1ResultFloat.getSemantics(), true), + L1ResultFloat, + MidValue, + L2ResultFloat, + llvm::APFloat::getLargest(L1ResultFloat.getSemantics(), false), + }; + + bool AlwaysTrue = true, AlwaysFalse = true; + + for (const llvm::APFloat &Value : Values) { + TryResult Res1, Res2; + Res1 = analyzeLogicOperatorCondition(BO1, Value, L1ResultFloat); + Res2 = analyzeLogicOperatorCondition(BO2, Value, L2ResultFloat); + + 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()); + } + } + + if (AlwaysTrue || AlwaysFalse) { + if (BuildOpts.Observer) + BuildOpts.Observer->compareAlwaysTrue(B, AlwaysTrue); + return TryResult(AlwaysTrue); + } + return {}; } + return {}; } /// A bitwise-or with a non-zero constant always evaluates to true. TryResult checkIncorrectBitwiseOrOperator(const BinaryOperator *B) { const Expr *LHSConstant = - tryTransformToIntOrEnumConstant(B->getLHS()->IgnoreParenImpCasts()); + tryTransformToLiteralConstants(B->getLHS()->IgnoreParenImpCasts()); const Expr *RHSConstant = - tryTransformToIntOrEnumConstant(B->getRHS()->IgnoreParenImpCasts()); + tryTransformToLiteralConstants(B->getRHS()->IgnoreParenImpCasts()); if ((LHSConstant && RHSConstant) || (!LHSConstant && !RHSConstant)) return {}; diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 3d6da4f70f99e..487b44d6fb312 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -166,13 +166,16 @@ class LogicalErrorHandler : public CFGCallback { S.Diag(B->getExprLoc(), DiagID) << DiagRange; } - void compareAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) override { + void compareAlwaysTrue(const BinaryOperator *B, + bool isAlwaysTrueOrFalse) override { if (HasMacroID(B)) return; SourceRange DiagRange = B->getSourceRange(); - S.Diag(B->getExprLoc(), diag::warn_tautological_overlap_comparison) - << DiagRange << isAlwaysTrue; + S.Diag(B->getExprLoc(), + isAlwaysTrueOrFalse ? diag::warn_tautological_overlap_comparison + : diag::warn_tautological_non_overlap_comparison) + << DiagRange; } void compareBitwiseEquality(const BinaryOperator *B, diff --git a/clang/test/Sema/warn-overlap.c b/clang/test/Sema/warn-overlap.c index 2db07ebcd17b8..e43c63bdaff9b 100644 --- a/clang/test/Sema/warn-overlap.c +++ b/clang/test/Sema/warn-overlap.c @@ -37,37 +37,37 @@ void f(int x) { if (x >= 0 || x <= 0) { } // expected-warning {{overlapping comparisons always evaluate to true}} // > && < - if (x > 2 && x < 1) { } // expected-warning {{overlapping comparisons always evaluate to false}} - if (x > 2 && x < 2) { } // expected-warning {{overlapping comparisons always evaluate to false}} - if (x > 2 && x < 3) { } // expected-warning {{overlapping comparisons always evaluate to false}} - if (x > 0 && x < 1) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x > 2 && x < 1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} + if (x > 2 && x < 2) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} + if (x > 2 && x < 3) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} + if (x > 0 && x < 1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} - if (x > 2 && x <= 1) { } // expected-warning {{overlapping comparisons always evaluate to false}} - if (x > 2 && x <= 2) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x > 2 && x <= 1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} + if (x > 2 && x <= 2) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} if (x > 2 && x <= 3) { } - if (x >= 2 && x < 1) { } // expected-warning {{overlapping comparisons always evaluate to false}} - if (x >= 2 && x < 2) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x >= 2 && x < 1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} + if (x >= 2 && x < 2) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} if (x >= 2 && x < 3) { } - if (x >= 0 && x < 0) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x >= 0 && x < 0) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} - if (x >= 2 && x <= 1) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x >= 2 && x <= 1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} if (x >= 2 && x <= 2) { } if (x >= 2 && x <= 3) { } // !=, ==, .. if (x != 2 || x != 3) { } // expected-warning {{overlapping comparisons always evaluate to true}} if (x != 2 || x < 3) { } // expected-warning {{overlapping comparisons always evaluate to true}} - if (x == 2 && x == 3) { } // expected-warning {{overlapping comparisons always evaluate to false}} - if (x == 2 && x > 3) { } // expected-warning {{overlapping comparisons always evaluate to false}} - if (x == 3 && x < 0) { } // expected-warning {{overlapping comparisons always evaluate to false}} - if (3 == x && x < 0) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x == 2 && x == 3) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} + if (x == 2 && x > 3) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} + if (x == 3 && x < 0) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} + if (3 == x && x < 0) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} if (x == mydefine && x > 3) { } if (x == (mydefine + 1) && x > 3) { } if (x != CHOICE_0 || x != CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to true}} - if (x == CHOICE_0 && x == CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (x == CHOICE_0 && x == CHOICE_1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} // Don't warn if comparing x to different types if (x == CHOICE_0 && x == 1) { } @@ -80,7 +80,7 @@ void f(int x) { void enums(enum Choices c) { if (c != CHOICE_0 || c != CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to true}} - if (c == CHOICE_0 && c == CHOICE_1) { } // expected-warning {{overlapping comparisons always evaluate to false}} + if (c == CHOICE_0 && c == CHOICE_1) { } // expected-warning {{non-overlapping comparisons always evaluate to false}} // Don't warn if comparing x to different types if (c == CHOICE_0 && c == 1) { } @@ -117,12 +117,12 @@ void assignment(int x) { int a = x > 4 || x < 10; // expected-warning@-1{{overlapping comparisons always evaluate to true}} int b = x < 2 && x > 5; - // expected-warning@-1{{overlapping comparisons always evaluate to false}} + // expected-warning@-1{{non-overlapping comparisons always evaluate to false}} int c = x != 1 || x != 3; // expected-warning@-1{{overlapping comparisons always evaluate to true}} int d = x == 1 && x == 2; - // expected-warning@-1{{overlapping comparisons always evaluate to false}} + // expected-warning@-1{{non-overlapping comparisons always evaluate to false}} int e = x < 1 || x != 0; // expected-warning@-1{{overlapping comparisons always evaluate to true}} @@ -132,12 +132,12 @@ int returns(int x) { return x > 4 || x < 10; // expected-warning@-1{{overlapping comparisons always evaluate to true}} return x < 2 && x > 5; - // expected-warning@-1{{overlapping comparisons always evaluate to false}} + // expected-warning@-1{{non-overlapping comparisons always evaluate to false}} 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}} + // expected-warning@-1{{non-overlapping comparisons always evaluate to false}} return x < 1 || x != 0; // expected-warning@-1{{overlapping comparisons always evaluate to true}} @@ -169,7 +169,73 @@ int struct_test(struct A a) { return a.x > 5 && a.y... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/133653 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
