riccibruno created this revision.
riccibruno added reviewers: aaron.ballman, rsmith.
riccibruno added a project: clang.
Herald added a subscriber: cfe-commits.
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`.
Repository:
rC Clang
https://reviews.llvm.org/D57747
Files:
lib/Sema/SemaChecking.cpp
test/Sema/warn-unsequenced.c
test/SemaCXX/warn-unsequenced.cpp
Index: test/SemaCXX/warn-unsequenced.cpp
===================================================================
--- test/SemaCXX/warn-unsequenced.cpp
+++ test/SemaCXX/warn-unsequenced.cpp
@@ -1,6 +1,8 @@
// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wno-unused -Wno-uninitialized -Wunsequenced %s
int f(int, int = 0);
+int g1();
+int g2(int);
struct A {
int x, y;
@@ -57,20 +59,20 @@
a = A { ++a, a++ }.x; // expected-warning {{unsequenced modifications to 'a'}}
A { ++a, a++ }.x + A { ++a, a++ }.y; // expected-warning {{unsequenced modifications to 'a'}}
- (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 to 'a'}}
- (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 to 'a'}}
(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 to 'a'}}
(1 ? a : ++a) + a; // ok
(0 ? a : a++) + a; // expected-warning {{unsequenced modification and access to 'a'}}
(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 to 'a'}}
@@ -93,10 +95,10 @@
// 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
+ (xs[7] && ++a) * (!xs[7] && ++a); // expected-warning {{multiple unsequenced modifications to 'a'}}
xs[0] = (a = 1, a); // ok
(a -= 128) &= 128; // ok
@@ -104,11 +106,49 @@
xs[8] ? ++a + a++ : 0; // expected-warning {{multiple unsequenced modifications to 'a'}}
xs[8] ? 0 : ++a + a++; // expected-warning {{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; // expected-warning {{unsequenced modification and access to 'a'}}
+ (xs[8] ? a+=1 : a) = a; // expected-warning {{unsequenced modification and access to 'a'}}
+ (xs[8] ? a : a+= 2) = a; // expected-warning {{unsequenced modification and access to 'a'}}
+ a = (xs[8] ? a+=1 : a+= 2); // no-warning
+ a += (xs[8] ? a+=1 : a+= 2); // expected-warning {{unsequenced modification and access to 'a'}}
+
+ (false ? a+=1 : a) = a; // no-warning
+ (true ? a+=1 : a) = a; // expected-warning {{unsequenced modification and access to 'a'}}
+ (false ? a : a+=2) = a; // expected-warning {{unsequenced modification and access to 'a'}}
+ (true ? a : a+=2) = a; // no-warning
xs[8] && (++a + a++); // expected-warning {{multiple unsequenced modifications to 'a'}}
xs[8] || (++a + a++); // expected-warning {{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++); // expected-warning {{multiple unsequenced modifications to 'a'}}
+ a = ((a++, true) && (a++, true) && (a++, true) && a++); // expected-warning {{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++); // expected-warning {{multiple unsequenced modifications to 'a'}}
+ a = (true && ++a); // no-warning
+ a = (true || a++); // no-warning
+ a = (false || a++); // expected-warning {{multiple unsequenced modifications to 'a'}}
+ a = (false || ++a); // no-warning
+
+ (a++) | (a++); // expected-warning {{multiple unsequenced modifications to 'a'}}
+ (a++) & (a++); // expected-warning {{multiple unsequenced modifications to 'a'}}
+ (a++) ^ (a++); // expected-warning {{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
Index: test/Sema/warn-unsequenced.c
===================================================================
--- test/Sema/warn-unsequenced.c
+++ test/Sema/warn-unsequenced.c
@@ -40,18 +40,18 @@
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,10 @@
// 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
+ (xs[7] && ++a) * (!xs[7] && ++a); // expected-warning {{multiple unsequenced modifications to 'a'}}
xs[0] = (a = 1, a); // ok
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -12208,64 +12208,124 @@
notePostMod(MemoryLoc, UO, UK_ModAsSideEffect);
}
- /// Don't visit the RHS of '&&' or '||' if it might not be evaluated.
void VisitBinLOr(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(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(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(CallExpr *CE) {
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
