[clang] 8a57153 - [Sema] SequenceChecker: Fix handling of operator ||, && and ?:
Bruno Ricci via cfe-commits
cfe-commits at lists.llvm.org
Sun Dec 22 04:36:43 PST 2019
Author: Bruno Ricci
Date: 2019-12-22T12:27:31Z
New Revision: 8a571538dff6dbc1b7f4fed6aed8be7ec1a00a8d
URL: https://github.com/llvm/llvm-project/commit/8a571538dff6dbc1b7f4fed6aed8be7ec1a00a8d
DIFF: https://github.com/llvm/llvm-project/commit/8a571538dff6dbc1b7f4fed6aed8be7ec1a00a8d.diff
LOG: [Sema] SequenceChecker: Fix handling of operator ||, && and ?:
The current handling of the operators ||, && and ?: has a number of false
positive and false negative. The issues for operator || and && are:
1. We need to add sequencing regions for the LHS and RHS as is done for the
comma operator. Not doing so causes false positives in expressions like
`((a++, false) || (a++, false))` (from PR39779, see PR22197 for another
example).
2. In the current implementation when the evaluation of the LHS fails, the RHS
is added to a worklist to be processed later. This results in false negatives
in expressions like `(a && a++) + a`.
Fix these issues by introducing sequencing regions for the LHS and RHS, and by
not deferring the visitation of the RHS.
The issues with the ternary operator ?: are similar, with the added twist that
we should not warn on expressions like `(x ? y += 1 : y += 2)` since exactly
one of the 2nd and 3rd expression is going to be evaluated, but we should still
warn on expressions like `(x ? y += 1 : y += 2) = y`.
Differential Revision: https://reviews.llvm.org/D57747
Reviewed By: rsmith
Added:
Modified:
clang/lib/Sema/SemaChecking.cpp
clang/test/Sema/warn-unsequenced.c
clang/test/SemaCXX/warn-unsequenced.cpp
Removed:
################################################################################
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 445a10b5eb8f..2cbe3632be3a 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -12779,6 +12779,9 @@ class SequenceChecker : public ConstEvaluatedExprVisitor<SequenceChecker> {
SmallVectorImpl<const Expr *> &WorkList)
: Base(S.Context), SemaRef(S), Region(Tree.root()), WorkList(WorkList) {
Visit(E);
+ // Silence a -Wunused-private-field since WorkList is now unused.
+ // TODO: Evaluate if it can be used, and if not remove it.
+ (void)this->WorkList;
}
void VisitStmt(const Stmt *S) {
@@ -12908,64 +12911,126 @@ class SequenceChecker : public ConstEvaluatedExprVisitor<SequenceChecker> {
notePostMod(O, UO, UK_ModAsSideEffect);
}
- /// Don't visit the RHS of '&&' or '||' if it might not be evaluated.
void VisitBinLOr(const BinaryOperator *BO) {
- // The side-effects of the LHS of an '&&' are sequenced before the
- // value computation of the RHS, and hence before the value computation
- // of the '&&' itself, unless the LHS evaluates to zero. We treat them
- // as if they were unconditionally sequenced.
+ // C++11 [expr.log.or]p2:
+ // If the second expression is evaluated, every value computation and
+ // side effect associated with the first expression is sequenced before
+ // every value computation and side effect associated with the
+ // second expression.
+ SequenceTree::Seq LHSRegion = Tree.allocate(Region);
+ SequenceTree::Seq RHSRegion = Tree.allocate(Region);
+ SequenceTree::Seq OldRegion = Region;
+
EvaluationTracker Eval(*this);
{
SequencedSubexpression Sequenced(*this);
+ Region = LHSRegion;
Visit(BO->getLHS());
}
- bool Result;
- if (Eval.evaluate(BO->getLHS(), Result)) {
- if (!Result)
- Visit(BO->getRHS());
- } else {
- // Check for unsequenced operations in the RHS, treating it as an
- // entirely separate evaluation.
- //
- // FIXME: If there are operations in the RHS which are unsequenced
- // with respect to operations outside the RHS, and those operations
- // are unconditionally evaluated, diagnose them.
- WorkList.push_back(BO->getRHS());
+ // C++11 [expr.log.or]p1:
+ // [...] the second operand is not evaluated if the first operand
+ // evaluates to true.
+ bool EvalResult = false;
+ bool EvalOK = Eval.evaluate(BO->getLHS(), EvalResult);
+ bool ShouldVisitRHS = !EvalOK || (EvalOK && !EvalResult);
+ if (ShouldVisitRHS) {
+ Region = RHSRegion;
+ Visit(BO->getRHS());
}
+
+ Region = OldRegion;
+ Tree.merge(LHSRegion);
+ Tree.merge(RHSRegion);
}
+
void VisitBinLAnd(const BinaryOperator *BO) {
+ // C++11 [expr.log.and]p2:
+ // If the second expression is evaluated, every value computation and
+ // side effect associated with the first expression is sequenced before
+ // every value computation and side effect associated with the
+ // second expression.
+ SequenceTree::Seq LHSRegion = Tree.allocate(Region);
+ SequenceTree::Seq RHSRegion = Tree.allocate(Region);
+ SequenceTree::Seq OldRegion = Region;
+
EvaluationTracker Eval(*this);
{
SequencedSubexpression Sequenced(*this);
+ Region = LHSRegion;
Visit(BO->getLHS());
}
- bool Result;
- if (Eval.evaluate(BO->getLHS(), Result)) {
- if (Result)
- Visit(BO->getRHS());
- } else {
- WorkList.push_back(BO->getRHS());
+ // C++11 [expr.log.and]p1:
+ // [...] the second operand is not evaluated if the first operand is false.
+ bool EvalResult = false;
+ bool EvalOK = Eval.evaluate(BO->getLHS(), EvalResult);
+ bool ShouldVisitRHS = !EvalOK || (EvalOK && EvalResult);
+ if (ShouldVisitRHS) {
+ Region = RHSRegion;
+ Visit(BO->getRHS());
}
+
+ Region = OldRegion;
+ Tree.merge(LHSRegion);
+ Tree.merge(RHSRegion);
}
- // Only visit the condition, unless we can be sure which subexpression will
- // be chosen.
void VisitAbstractConditionalOperator(const AbstractConditionalOperator *CO) {
+ // C++11 [expr.cond]p1:
+ // [...] Every value computation and side effect associated with the first
+ // expression is sequenced before every value computation and side effect
+ // associated with the second or third expression.
+ SequenceTree::Seq ConditionRegion = Tree.allocate(Region);
+
+ // No sequencing is specified between the true and false expression.
+ // However since exactly one of both is going to be evaluated we can
+ // consider them to be sequenced. This is needed to avoid warning on
+ // something like "x ? y+= 1 : y += 2;" in the case where we will visit
+ // both the true and false expressions because we can't evaluate x.
+ // This will still allow us to detect an expression like (pre C++17)
+ // "(x ? y += 1 : y += 2) = y".
+ //
+ // We don't wrap the visitation of the true and false expression with
+ // SequencedSubexpression because we don't want to downgrade modifications
+ // as side effect in the true and false expressions after the visition
+ // is done. (for example in the expression "(x ? y++ : y++) + y" we should
+ // not warn between the two "y++", but we should warn between the "y++"
+ // and the "y".
+ SequenceTree::Seq TrueRegion = Tree.allocate(Region);
+ SequenceTree::Seq FalseRegion = Tree.allocate(Region);
+ SequenceTree::Seq OldRegion = Region;
+
EvaluationTracker Eval(*this);
{
SequencedSubexpression Sequenced(*this);
+ Region = ConditionRegion;
Visit(CO->getCond());
}
- bool Result;
- if (Eval.evaluate(CO->getCond(), Result))
- Visit(Result ? CO->getTrueExpr() : CO->getFalseExpr());
- else {
- WorkList.push_back(CO->getTrueExpr());
- WorkList.push_back(CO->getFalseExpr());
+ // C++11 [expr.cond]p1:
+ // [...] The first expression is contextually converted to bool (Clause 4).
+ // It is evaluated and if it is true, the result of the conditional
+ // expression is the value of the second expression, otherwise that of the
+ // third expression. Only one of the second and third expressions is
+ // evaluated. [...]
+ bool EvalResult = false;
+ bool EvalOK = Eval.evaluate(CO->getCond(), EvalResult);
+ bool ShouldVisitTrueExpr = !EvalOK || (EvalOK && EvalResult);
+ bool ShouldVisitFalseExpr = !EvalOK || (EvalOK && !EvalResult);
+ if (ShouldVisitTrueExpr) {
+ Region = TrueRegion;
+ Visit(CO->getTrueExpr());
+ }
+ if (ShouldVisitFalseExpr) {
+ Region = FalseRegion;
+ Visit(CO->getFalseExpr());
}
+
+ Region = OldRegion;
+ Tree.merge(ConditionRegion);
+ Tree.merge(TrueRegion);
+ Tree.merge(FalseRegion);
}
void VisitCallExpr(const CallExpr *CE) {
diff --git a/clang/test/Sema/warn-unsequenced.c b/clang/test/Sema/warn-unsequenced.c
index 9654cda92408..fd0227b479a8 100644
--- a/clang/test/Sema/warn-unsequenced.c
+++ b/clang/test/Sema/warn-unsequenced.c
@@ -40,18 +40,18 @@ void test() {
A agg1 = { a++, a++ }; // expected-warning {{multiple unsequenced modifications}}
A agg2 = { a++ + a, a++ }; // expected-warning {{unsequenced modification and access}}
- (xs[2] && (a = 0)) + a; // ok
+ (xs[2] && (a = 0)) + a; // expected-warning {{unsequenced modification and access to 'a'}}
(0 && (a = 0)) + a; // ok
(1 && (a = 0)) + a; // expected-warning {{unsequenced modification and access}}
- (xs[3] || (a = 0)) + a; // ok
+ (xs[3] || (a = 0)) + a; // expected-warning {{unsequenced modification and access to 'a'}}
(0 || (a = 0)) + a; // expected-warning {{unsequenced modification and access}}
(1 || (a = 0)) + a; // ok
- (xs[4] ? a : ++a) + a; // ok
+ (xs[4] ? a : ++a) + a; // expected-warning {{unsequenced modification and access to 'a'}}
(0 ? a : ++a) + a; // expected-warning {{unsequenced modification and access}}
(1 ? a : ++a) + a; // ok
- (xs[5] ? ++a : ++a) + a; // FIXME: warn here
+ (xs[5] ? ++a : ++a) + a; // expected-warning {{unsequenced modification and access to 'a'}}
(++a, xs[6] ? ++a : 0) + a; // expected-warning {{unsequenced modification and access}}
@@ -73,10 +73,11 @@ void test() {
// unconditional.
a = a++ && f(a, a);
- // This has undefined behavior if a != 0. FIXME: We should diagnose this.
- (a && a++) + a;
+ // This has undefined behavior if a != 0.
+ (a && a++) + a; // expected-warning {{unsequenced modification and access to 'a'}}
- (xs[7] && ++a) * (!xs[7] && ++a); // ok
+ // FIXME: Find a way to avoid warning here.
+ (xs[7] && ++a) * (!xs[7] && ++a); // expected-warning {{multiple unsequenced modifications to 'a'}}
xs[0] = (a = 1, a); // ok
diff --git a/clang/test/SemaCXX/warn-unsequenced.cpp b/clang/test/SemaCXX/warn-unsequenced.cpp
index bb8fd8be3d4f..43b36009fe30 100644
--- a/clang/test/SemaCXX/warn-unsequenced.cpp
+++ b/clang/test/SemaCXX/warn-unsequenced.cpp
@@ -4,6 +4,8 @@
// RUN: -Wunsequenced -Wno-c++17-extensions -Wno-c++14-extensions %s
int f(int, int = 0);
+int g1();
+int g2(int);
struct A {
int x, y;
@@ -79,24 +81,28 @@ void test() {
A { ++a, a++ }.x + A { ++a, a++ }.y; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
// cxx17-warning at -1 {{multiple unsequenced modifications to 'a'}}
- (xs[2] && (a = 0)) + a; // ok
+ (xs[2] && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
(0 && (a = 0)) + a; // ok
(1 && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
// cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
- (xs[3] || (a = 0)) + a; // ok
+ (xs[3] || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
(0 || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
// cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
(1 || (a = 0)) + a; // ok
- (xs[4] ? a : ++a) + a; // ok
+ (xs[4] ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
(0 ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
// cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
(1 ? a : ++a) + a; // ok
(0 ? a : a++) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
// cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
(1 ? a : a++) + a; // ok
- (xs[5] ? ++a : ++a) + a; // FIXME: warn here
+ (xs[5] ? ++a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
(++a, xs[6] ? ++a : 0) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
// cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
@@ -122,10 +128,13 @@ void test() {
// unconditional.
a = a++ && f(a, a);
- // This has undefined behavior if a != 0. FIXME: We should diagnose this.
- (a && a++) + a;
+ // This has undefined behavior if a != 0.
+ (a && a++) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
- (xs[7] && ++a) * (!xs[7] && ++a); // ok
+ // FIXME: Don't warn here.
+ (xs[7] && ++a) * (!xs[7] && ++a); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
+ // cxx17-warning at -1 {{multiple unsequenced modifications to 'a'}}
xs[0] = (a = 1, a); // ok
(a -= 128) &= 128; // ok
@@ -135,13 +144,64 @@ void test() {
// cxx17-warning at -1 {{multiple unsequenced modifications to 'a'}}
xs[8] ? 0 : ++a + a++; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
// cxx17-warning at -1 {{multiple unsequenced modifications to 'a'}}
- xs[8] ? ++a : a++; // ok
+ xs[8] ? ++a : a++; // no-warning
+ xs[8] ? a+=1 : a+= 2; // no-warning
+ (xs[8] ? a+=1 : a+= 2) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // TODO cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
+ (xs[8] ? a+=1 : a) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // TODO cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
+ (xs[8] ? a : a+= 2) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // TODO cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
+ a = (xs[8] ? a+=1 : a+= 2); // no-warning
+ a += (xs[8] ? a+=1 : a+= 2); // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // TODO cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
+
+ (false ? a+=1 : a) = a; // no-warning
+ (true ? a+=1 : a) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // TODO cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
+ (false ? a : a+=2) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
+ // TODO cxx17-warning at -1 {{unsequenced modification and access to 'a'}}
+ (true ? a : a+=2) = a; // no-warning
xs[8] && (++a + a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
// cxx17-warning at -1 {{multiple unsequenced modifications to 'a'}}
xs[8] || (++a + a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
// cxx17-warning at -1 {{multiple unsequenced modifications to 'a'}}
+ ((a++, false) || (a++, false)); // no-warning PR39779
+ ((a++, true) && (a++, true)); // no-warning PR39779
+
+ int i,j;
+ (i = g1(), false) || (j = g2(i)); // no-warning PR22197
+ (i = g1(), true) && (j = g2(i)); // no-warning PR22197
+
+ (a++, false) || (a++, false) || (a++, false) || (a++, false); // no-warning
+ (a++, true) || (a++, true) || (a++, true) || (a++, true); // no-warning
+ a = ((a++, false) || (a++, false) || (a++, false) || (a++, false)); // no-warning
+ a = ((a++, true) && (a++, true) && (a++, true) && (a++, true)); // no-warning
+ a = ((a++, false) || (a++, false) || (a++, false) || a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
+ // TODO cxx17-warning at -1 {{multiple unsequenced modifications to 'a'}}
+ a = ((a++, true) && (a++, true) && (a++, true) && a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
+ // TODO cxx17-warning at -1 {{multiple unsequenced modifications to 'a'}}
+ a = ((a++, false) || (a++, false) || (a++, false) || (a + a, false)); // no-warning
+ a = ((a++, true) && (a++, true) && (a++, true) && (a + a, true)); // no-warning
+
+ a = (false && a++); // no-warning
+ a = (true && a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
+ // TODO cxx17-warning at -1 {{multiple unsequenced modifications to 'a'}}
+ a = (true && ++a); // no-warning
+ a = (true || a++); // no-warning
+ a = (false || a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
+ // TODO cxx17-warning at -1 {{multiple unsequenced modifications to 'a'}}
+ a = (false || ++a); // no-warning
+
+ (a++) | (a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
+ // cxx17-warning at -1 {{multiple unsequenced modifications to 'a'}}
+ (a++) & (a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
+ // cxx17-warning at -1 {{multiple unsequenced modifications to 'a'}}
+ (a++) ^ (a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
+ // cxx17-warning at -1 {{multiple unsequenced modifications to 'a'}}
+
(__builtin_classify_type(++a) ? 1 : 0) + ++a; // ok
(__builtin_constant_p(++a) ? 1 : 0) + ++a; // ok
(__builtin_object_size(&(++a, a), 0) ? 1 : 0) + ++a; // ok
More information about the cfe-commits
mailing list