[clang] b3469ce - [clang][Analysis] Handle && and || against variable and its negation as tautology
Takuya Shimizu via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 17 01:56:14 PDT 2023
Author: Takuya Shimizu
Date: 2023-08-17T17:55:48+09:00
New Revision: b3469ce6f80bce2b0a86026fd6867c4b04853466
URL: https://github.com/llvm/llvm-project/commit/b3469ce6f80bce2b0a86026fd6867c4b04853466
DIFF: https://github.com/llvm/llvm-project/commit/b3469ce6f80bce2b0a86026fd6867c4b04853466.diff
LOG: [clang][Analysis] Handle && and || against variable and its negation as tautology
This patch introduces a new warning flag -Wtautological-negation-compare grouped in -Wtautological-compare that warns on the use of && or || operators against a variable and its negation.
e.g. x || !x and !x && x
This also makes the -Winfinite-recursion diagnose more cases.
Fixes https://github.com/llvm/llvm-project/issues/56035
Differential Revision: https://reviews.llvm.org/D152093
Added:
clang/test/SemaCXX/tautological-negation-compare.cpp
Modified:
clang/docs/ReleaseNotes.rst
clang/include/clang/Analysis/CFG.h
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Analysis/CFG.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/Analysis/temp-obj-dtors-cfg-output.cpp
clang/test/Misc/warning-wall.c
clang/test/SemaCXX/warn-infinite-recursion.cpp
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 61272838a68b8c..02b897bcd0a68e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -132,6 +132,10 @@ Improvements to Clang's diagnostics
of a base class is not called in the constructor of its derived class.
- Clang no longer emits ``-Wmissing-variable-declarations`` for variables declared
with the ``register`` storage class.
+- Clang's ``-Wtautological-negation-compare`` flag now diagnoses logical
+ tautologies like ``x && !x`` and ``!x || x`` in expressions. This also
+ makes ``-Winfinite-recursion`` diagnose more cases.
+ (`#56035: <https://github.com/llvm/llvm-project/issues/56035>`_).
Bug Fixes in This Version
-------------------------
diff --git a/clang/include/clang/Analysis/CFG.h b/clang/include/clang/Analysis/CFG.h
index eacebe176dda4c..cf4fa2da2a358e 100644
--- a/clang/include/clang/Analysis/CFG.h
+++ b/clang/include/clang/Analysis/CFG.h
@@ -1162,6 +1162,7 @@ class CFGCallback {
CFGCallback() = default;
virtual ~CFGCallback() = default;
+ virtual void logicAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) {}
virtual void compareAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) {}
virtual void compareBitwiseEquality(const BinaryOperator *B,
bool isAlwaysTrue) {}
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 6f8386cd10922f..d1aa51393ef357 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -678,13 +678,15 @@ 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 TautologicalNegationCompare : DiagGroup<"tautological-negation-compare">;
def TautologicalCompare : DiagGroup<"tautological-compare",
[TautologicalConstantCompare,
TautologicalPointerCompare,
TautologicalOverlapCompare,
TautologicalBitwiseCompare,
TautologicalUndefinedCompare,
- TautologicalObjCBoolCompare]>;
+ TautologicalObjCBoolCompare,
+ TautologicalNegationCompare]>;
def HeaderHygiene : DiagGroup<"header-hygiene">;
def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">;
def CompareDistinctPointerType : DiagGroup<"compare-distinct-pointer-types">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 632be32cfddd5a..8c629aad89f48a 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9796,6 +9796,12 @@ def warn_comparison_bitwise_always : Warning<
def warn_comparison_bitwise_or : Warning<
"bitwise or with non-zero value always evaluates to true">,
InGroup<TautologicalBitwiseCompare>, DefaultIgnore;
+def warn_tautological_negation_and_compare: Warning<
+ "'&&' of a value and its negation always evaluates to false">,
+ InGroup<TautologicalNegationCompare>, DefaultIgnore;
+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">,
InGroup<TautologicalOverlapCompare>, DefaultIgnore;
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 0019f971aac4dc..b82f9010a83f77 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -1070,16 +1070,41 @@ class CFGBuilder {
}
}
- /// Find a pair of comparison expressions with or without parentheses
+ /// 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)
+ /// 2. Find a pair of comparison expressions with or without parentheses
/// with a shared variable and constants and a logical operator between them
/// that always evaluates to either true or false.
/// e.g. if (x != 3 || x != 4)
TryResult checkIncorrectLogicOperator(const BinaryOperator *B) {
assert(B->isLogicalOp());
- const BinaryOperator *LHS =
- dyn_cast<BinaryOperator>(B->getLHS()->IgnoreParens());
- const BinaryOperator *RHS =
- dyn_cast<BinaryOperator>(B->getRHS()->IgnoreParens());
+ const Expr *LHSExpr = B->getLHS()->IgnoreParens();
+ const Expr *RHSExpr = B->getRHS()->IgnoreParens();
+
+ auto CheckLogicalOpWithNegatedVariable = [this, B](const Expr *E1,
+ const Expr *E2) {
+ if (const auto *Negate = dyn_cast<UnaryOperator>(E1)) {
+ if (Negate->getOpcode() == UO_LNot &&
+ Expr::isSameComparisonOperand(Negate->getSubExpr(), E2)) {
+ bool AlwaysTrue = B->getOpcode() == BO_LOr;
+ if (BuildOpts.Observer)
+ BuildOpts.Observer->logicAlwaysTrue(B, AlwaysTrue);
+ return TryResult(AlwaysTrue);
+ }
+ }
+ return TryResult();
+ };
+
+ TryResult Result = CheckLogicalOpWithNegatedVariable(LHSExpr, RHSExpr);
+ if (Result.isKnown())
+ return Result;
+ Result = CheckLogicalOpWithNegatedVariable(RHSExpr, LHSExpr);
+ if (Result.isKnown())
+ return Result;
+
+ const auto *LHS = dyn_cast<BinaryOperator>(LHSExpr);
+ const auto *RHS = dyn_cast<BinaryOperator>(RHSExpr);
if (!LHS || !RHS)
return {};
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index d047df257b1042..07762997c9168c 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -158,6 +158,17 @@ class LogicalErrorHandler : public CFGCallback {
return false;
}
+ void logicAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) override {
+ if (HasMacroID(B))
+ return;
+
+ unsigned DiagID = isAlwaysTrue
+ ? diag::warn_tautological_negation_or_compare
+ : diag::warn_tautological_negation_and_compare;
+ SourceRange DiagRange = B->getSourceRange();
+ S.Diag(B->getExprLoc(), DiagID) << DiagRange;
+ }
+
void compareAlwaysTrue(const BinaryOperator *B, bool isAlwaysTrue) override {
if (HasMacroID(B))
return;
@@ -188,7 +199,8 @@ class LogicalErrorHandler : public CFGCallback {
static bool hasActiveDiagnostics(DiagnosticsEngine &Diags,
SourceLocation Loc) {
return !Diags.isIgnored(diag::warn_tautological_overlap_comparison, Loc) ||
- !Diags.isIgnored(diag::warn_comparison_bitwise_or, Loc);
+ !Diags.isIgnored(diag::warn_comparison_bitwise_or, Loc) ||
+ !Diags.isIgnored(diag::warn_tautological_negation_and_compare, Loc);
}
};
} // anonymous namespace
diff --git a/clang/test/Analysis/temp-obj-dtors-cfg-output.cpp b/clang/test/Analysis/temp-obj-dtors-cfg-output.cpp
index 3730c03e3e3360..b496d261e92f4a 100644
--- a/clang/test/Analysis/temp-obj-dtors-cfg-output.cpp
+++ b/clang/test/Analysis/temp-obj-dtors-cfg-output.cpp
@@ -1393,13 +1393,13 @@ const C &bar3(bool coin) {
// CHECK: Succs (2): B2 B1
// CHECK: [B4 (NORETURN)]
// CHECK: 1: ~NoReturn() (Temporary object destructor)
-// CHECK: Preds (1): B5
+// CHECK: Preds (1): B5(Unreachable)
// CHECK: Succs (1): B0
// CHECK: [B5]
// CHECK: 1: [B8.3] || [B7.2] || [B6.7]
// CHECK: T: (Temp Dtor) [B6.4]
// CHECK: Preds (3): B6 B7 B8
-// CHECK: Succs (2): B4 B3
+// CHECK: Succs (2): B4(Unreachable) B3
// CHECK: [B6]
// CHECK: 1: check
// CHECK: 2: [B6.1] (ImplicitCastExpr, FunctionToPointerDecay, _Bool (*)(const NoReturn &))
diff --git a/clang/test/Misc/warning-wall.c b/clang/test/Misc/warning-wall.c
index 1c74759ce61610..a0bd571cab0513 100644
--- a/clang/test/Misc/warning-wall.c
+++ b/clang/test/Misc/warning-wall.c
@@ -55,6 +55,7 @@ CHECK-NEXT: -Wtautological-overlap-compare
CHECK-NEXT: -Wtautological-bitwise-compare
CHECK-NEXT: -Wtautological-undefined-compare
CHECK-NEXT: -Wtautological-objc-bool-compare
+CHECK-NEXT: -Wtautological-negation-compare
CHECK-NEXT: -Wtrigraphs
CHECK-NEXT: -Wuninitialized
CHECK-NEXT: -Wsometimes-uninitialized
diff --git a/clang/test/SemaCXX/tautological-negation-compare.cpp b/clang/test/SemaCXX/tautological-negation-compare.cpp
new file mode 100644
index 00000000000000..0f4d9f9842c252
--- /dev/null
+++ b/clang/test/SemaCXX/tautological-negation-compare.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-negation-compare -Wno-constant-logical-operand %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-compare -Wno-constant-logical-operand %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused -Wno-loop-analysis -Wno-constant-logical-operand %s
+
+#define COPY(x) x
+
+void test_int(int x) {
+ if (x || !x) {} // expected-warning {{'||' of a value and its negation always evaluates to true}}
+ if (!x || x) {} // expected-warning {{'||' of a value and its negation always evaluates to true}}
+ if (x && !x) {} // expected-warning {{'&&' of a value and its negation always evaluates to false}}
+ if (!x && x) {} // expected-warning {{'&&' of a value and its negation always evaluates to false}}
+
+ // parentheses are ignored
+ if (x || (!x)) {} // expected-warning {{'||' of a value and its negation always evaluates to true}}
+ if (!(x) || x) {} // expected-warning {{'||' of a value and its negation always evaluates to true}}
+
+ // don't warn on macros
+ if (COPY(x) || !x) {}
+ if (!x || COPY(x)) {}
+ if (x && COPY(!x)) {}
+ if (COPY(!x && x)) {}
+
+ // dont' warn on literals
+ if (1 || !1) {}
+ if (!42 && 42) {}
+
+
+ // don't warn on overloads
+ struct Foo{
+ int val;
+ Foo operator!() const { return Foo{!val}; }
+ bool operator||(const Foo other) const { return val || other.val; }
+ bool operator&&(const Foo other) const { return val && other.val; }
+ };
+
+ Foo f{3};
+ if (f || !f) {}
+ if (!f || f) {}
+ if (f.val || !f.val) {} // expected-warning {{'||' of a value and its negation always evaluates to true}}
+ if (!f.val && f.val) {} // expected-warning {{'&&' of a value and its negation always evaluates to false}}
+}
diff --git a/clang/test/SemaCXX/warn-infinite-recursion.cpp b/clang/test/SemaCXX/warn-infinite-recursion.cpp
index 8e07d5c876612a..d0f3fe7b164e19 100644
--- a/clang/test/SemaCXX/warn-infinite-recursion.cpp
+++ b/clang/test/SemaCXX/warn-infinite-recursion.cpp
@@ -203,3 +203,13 @@ int unevaluated_recursive_function() {
(void)typeid(unevaluated_recursive_function());
return 0;
}
+
+void func1(int i) { // expected-warning {{call itself}}
+ if (i || !i)
+ func1(i);
+}
+void func2(int i) { // expected-warning {{call itself}}
+ if (!i && i) {}
+ else
+ func2(i);
+}
More information about the cfe-commits
mailing list