Skip to content

Commit 8a57153

Browse files
riccibrunoriccibruno
committed
[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
1 parent b6eba31 commit 8a57153

File tree

3 files changed

+172
-46
lines changed

3 files changed

+172
-46
lines changed

‎clang/lib/Sema/SemaChecking.cpp

Lines changed: 96 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -12779,6 +12779,9 @@ class SequenceChecker : public ConstEvaluatedExprVisitor<SequenceChecker> {
1277912779
SmallVectorImpl<const Expr *> &WorkList)
1278012780
: Base(S.Context), SemaRef(S), Region(Tree.root()), WorkList(WorkList) {
1278112781
Visit(E);
12782+
// Silence a -Wunused-private-field since WorkList is now unused.
12783+
// TODO: Evaluate if it can be used, and if not remove it.
12784+
(void)this->WorkList;
1278212785
}
1278312786

1278412787
void VisitStmt(const Stmt *S) {
@@ -12908,64 +12911,126 @@ class SequenceChecker : public ConstEvaluatedExprVisitor<SequenceChecker> {
1290812911
notePostMod(O, UO, UK_ModAsSideEffect);
1290912912
}
1291012913

12911-
/// Don't visit the RHS of '&&' or '||' if it might not be evaluated.
1291212914
void VisitBinLOr(const BinaryOperator *BO) {
12913-
// The side-effects of the LHS of an '&&' are sequenced before the
12914-
// value computation of the RHS, and hence before the value computation
12915-
// of the '&&' itself, unless the LHS evaluates to zero. We treat them
12916-
// as if they were unconditionally sequenced.
12915+
// C++11 [expr.log.or]p2:
12916+
// If the second expression is evaluated, every value computation and
12917+
// side effect associated with the first expression is sequenced before
12918+
// every value computation and side effect associated with the
12919+
// second expression.
12920+
SequenceTree::Seq LHSRegion = Tree.allocate(Region);
12921+
SequenceTree::Seq RHSRegion = Tree.allocate(Region);
12922+
SequenceTree::Seq OldRegion = Region;
12923+
1291712924
EvaluationTracker Eval(*this);
1291812925
{
1291912926
SequencedSubexpression Sequenced(*this);
12927+
Region = LHSRegion;
1292012928
Visit(BO->getLHS());
1292112929
}
1292212930

12923-
bool Result;
12924-
if (Eval.evaluate(BO->getLHS(), Result)) {
12925-
if (!Result)
12926-
Visit(BO->getRHS());
12927-
} else {
12928-
// Check for unsequenced operations in the RHS, treating it as an
12929-
// entirely separate evaluation.
12930-
//
12931-
// FIXME: If there are operations in the RHS which are unsequenced
12932-
// with respect to operations outside the RHS, and those operations
12933-
// are unconditionally evaluated, diagnose them.
12934-
WorkList.push_back(BO->getRHS());
12931+
// C++11 [expr.log.or]p1:
12932+
// [...] the second operand is not evaluated if the first operand
12933+
// evaluates to true.
12934+
bool EvalResult = false;
12935+
bool EvalOK = Eval.evaluate(BO->getLHS(), EvalResult);
12936+
bool ShouldVisitRHS = !EvalOK || (EvalOK && !EvalResult);
12937+
if (ShouldVisitRHS) {
12938+
Region = RHSRegion;
12939+
Visit(BO->getRHS());
1293512940
}
12941+
12942+
Region = OldRegion;
12943+
Tree.merge(LHSRegion);
12944+
Tree.merge(RHSRegion);
1293612945
}
12946+
1293712947
void VisitBinLAnd(const BinaryOperator *BO) {
12948+
// C++11 [expr.log.and]p2:
12949+
// If the second expression is evaluated, every value computation and
12950+
// side effect associated with the first expression is sequenced before
12951+
// every value computation and side effect associated with the
12952+
// second expression.
12953+
SequenceTree::Seq LHSRegion = Tree.allocate(Region);
12954+
SequenceTree::Seq RHSRegion = Tree.allocate(Region);
12955+
SequenceTree::Seq OldRegion = Region;
12956+
1293812957
EvaluationTracker Eval(*this);
1293912958
{
1294012959
SequencedSubexpression Sequenced(*this);
12960+
Region = LHSRegion;
1294112961
Visit(BO->getLHS());
1294212962
}
1294312963

12944-
bool Result;
12945-
if (Eval.evaluate(BO->getLHS(), Result)) {
12946-
if (Result)
12947-
Visit(BO->getRHS());
12948-
} else {
12949-
WorkList.push_back(BO->getRHS());
12964+
// C++11 [expr.log.and]p1:
12965+
// [...] the second operand is not evaluated if the first operand is false.
12966+
bool EvalResult = false;
12967+
bool EvalOK = Eval.evaluate(BO->getLHS(), EvalResult);
12968+
bool ShouldVisitRHS = !EvalOK || (EvalOK && EvalResult);
12969+
if (ShouldVisitRHS) {
12970+
Region = RHSRegion;
12971+
Visit(BO->getRHS());
1295012972
}
12973+
12974+
Region = OldRegion;
12975+
Tree.merge(LHSRegion);
12976+
Tree.merge(RHSRegion);
1295112977
}
1295212978

12953-
// Only visit the condition, unless we can be sure which subexpression will
12954-
// be chosen.
1295512979
void VisitAbstractConditionalOperator(const AbstractConditionalOperator *CO) {
12980+
// C++11 [expr.cond]p1:
12981+
// [...] Every value computation and side effect associated with the first
12982+
// expression is sequenced before every value computation and side effect
12983+
// associated with the second or third expression.
12984+
SequenceTree::Seq ConditionRegion = Tree.allocate(Region);
12985+
12986+
// No sequencing is specified between the true and false expression.
12987+
// However since exactly one of both is going to be evaluated we can
12988+
// consider them to be sequenced. This is needed to avoid warning on
12989+
// something like "x ? y+= 1 : y += 2;" in the case where we will visit
12990+
// both the true and false expressions because we can't evaluate x.
12991+
// This will still allow us to detect an expression like (pre C++17)
12992+
// "(x ? y += 1 : y += 2) = y".
12993+
//
12994+
// We don't wrap the visitation of the true and false expression with
12995+
// SequencedSubexpression because we don't want to downgrade modifications
12996+
// as side effect in the true and false expressions after the visition
12997+
// is done. (for example in the expression "(x ? y++ : y++) + y" we should
12998+
// not warn between the two "y++", but we should warn between the "y++"
12999+
// and the "y".
13000+
SequenceTree::Seq TrueRegion = Tree.allocate(Region);
13001+
SequenceTree::Seq FalseRegion = Tree.allocate(Region);
13002+
SequenceTree::Seq OldRegion = Region;
13003+
1295613004
EvaluationTracker Eval(*this);
1295713005
{
1295813006
SequencedSubexpression Sequenced(*this);
13007+
Region = ConditionRegion;
1295913008
Visit(CO->getCond());
1296013009
}
1296113010

12962-
bool Result;
12963-
if (Eval.evaluate(CO->getCond(), Result))
12964-
Visit(Result ? CO->getTrueExpr() : CO->getFalseExpr());
12965-
else {
12966-
WorkList.push_back(CO->getTrueExpr());
12967-
WorkList.push_back(CO->getFalseExpr());
13011+
// C++11 [expr.cond]p1:
13012+
// [...] The first expression is contextually converted to bool (Clause 4).
13013+
// It is evaluated and if it is true, the result of the conditional
13014+
// expression is the value of the second expression, otherwise that of the
13015+
// third expression. Only one of the second and third expressions is
13016+
// evaluated. [...]
13017+
bool EvalResult = false;
13018+
bool EvalOK = Eval.evaluate(CO->getCond(), EvalResult);
13019+
bool ShouldVisitTrueExpr = !EvalOK || (EvalOK && EvalResult);
13020+
bool ShouldVisitFalseExpr = !EvalOK || (EvalOK && !EvalResult);
13021+
if (ShouldVisitTrueExpr) {
13022+
Region = TrueRegion;
13023+
Visit(CO->getTrueExpr());
13024+
}
13025+
if (ShouldVisitFalseExpr) {
13026+
Region = FalseRegion;
13027+
Visit(CO->getFalseExpr());
1296813028
}
13029+
13030+
Region = OldRegion;
13031+
Tree.merge(ConditionRegion);
13032+
Tree.merge(TrueRegion);
13033+
Tree.merge(FalseRegion);
1296913034
}
1297013035

1297113036
void VisitCallExpr(const CallExpr *CE) {

‎clang/test/Sema/warn-unsequenced.c

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -40,18 +40,18 @@ void test() {
4040
A agg1 = { a++, a++ }; // expected-warning {{multiple unsequenced modifications}}
4141
A agg2 = { a++ + a, a++ }; // expected-warning {{unsequenced modification and access}}
4242

43-
(xs[2] && (a = 0)) + a; // ok
43+
(xs[2] && (a = 0)) + a; // expected-warning {{unsequenced modification and access to 'a'}}
4444
(0 && (a = 0)) + a; // ok
4545
(1 && (a = 0)) + a; // expected-warning {{unsequenced modification and access}}
4646

47-
(xs[3] || (a = 0)) + a; // ok
47+
(xs[3] || (a = 0)) + a; // expected-warning {{unsequenced modification and access to 'a'}}
4848
(0 || (a = 0)) + a; // expected-warning {{unsequenced modification and access}}
4949
(1 || (a = 0)) + a; // ok
5050

51-
(xs[4] ? a : ++a) + a; // ok
51+
(xs[4] ? a : ++a) + a; // expected-warning {{unsequenced modification and access to 'a'}}
5252
(0 ? a : ++a) + a; // expected-warning {{unsequenced modification and access}}
5353
(1 ? a : ++a) + a; // ok
54-
(xs[5] ? ++a : ++a) + a; // FIXME: warn here
54+
(xs[5] ? ++a : ++a) + a; // expected-warning {{unsequenced modification and access to 'a'}}
5555

5656
(++a, xs[6] ? ++a : 0) + a; // expected-warning {{unsequenced modification and access}}
5757

@@ -73,10 +73,11 @@ void test() {
7373
// unconditional.
7474
a = a++ && f(a, a);
7575

76-
// This has undefined behavior if a != 0. FIXME: We should diagnose this.
77-
(a && a++) + a;
76+
// This has undefined behavior if a != 0.
77+
(a && a++) + a; // expected-warning {{unsequenced modification and access to 'a'}}
7878

79-
(xs[7] && ++a) * (!xs[7] && ++a); // ok
79+
// FIXME: Find a way to avoid warning here.
80+
(xs[7] && ++a) * (!xs[7] && ++a); // expected-warning {{multiple unsequenced modifications to 'a'}}
8081

8182
xs[0] = (a = 1, a); // ok
8283

‎clang/test/SemaCXX/warn-unsequenced.cpp

Lines changed: 68 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
// RUN: -Wunsequenced -Wno-c++17-extensions -Wno-c++14-extensions %s
55

66
int f(int, int = 0);
7+
int g1();
8+
int g2(int);
79

810
struct A {
911
int x, y;
@@ -79,24 +81,28 @@ void test() {
7981
A { ++a, a++ }.x + A { ++a, a++ }.y; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
8082
// cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
8183

82-
(xs[2] && (a = 0)) + a; // ok
84+
(xs[2] && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
85+
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
8386
(0 && (a = 0)) + a; // ok
8487
(1 && (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
8588
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
8689

87-
(xs[3] || (a = 0)) + a; // ok
90+
(xs[3] || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
91+
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
8892
(0 || (a = 0)) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
8993
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
9094
(1 || (a = 0)) + a; // ok
9195

92-
(xs[4] ? a : ++a) + a; // ok
96+
(xs[4] ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
97+
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
9398
(0 ? a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
9499
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
95100
(1 ? a : ++a) + a; // ok
96101
(0 ? a : a++) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
97102
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
98103
(1 ? a : a++) + a; // ok
99-
(xs[5] ? ++a : ++a) + a; // FIXME: warn here
104+
(xs[5] ? ++a : ++a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
105+
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
100106

101107
(++a, xs[6] ? ++a : 0) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
102108
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
@@ -122,10 +128,13 @@ void test() {
122128
// unconditional.
123129
a = a++ && f(a, a);
124130

125-
// This has undefined behavior if a != 0. FIXME: We should diagnose this.
126-
(a && a++) + a;
131+
// This has undefined behavior if a != 0.
132+
(a && a++) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
133+
// cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
127134

128-
(xs[7] && ++a) * (!xs[7] && ++a); // ok
135+
// FIXME: Don't warn here.
136+
(xs[7] && ++a) * (!xs[7] && ++a); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
137+
// cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
129138

130139
xs[0] = (a = 1, a); // ok
131140
(a -= 128) &= 128; // ok
@@ -135,13 +144,64 @@ void test() {
135144
// cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
136145
xs[8] ? 0 : ++a + a++; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
137146
// cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
138-
xs[8] ? ++a : a++; // ok
147+
xs[8] ? ++a : a++; // no-warning
148+
xs[8] ? a+=1 : a+= 2; // no-warning
149+
(xs[8] ? a+=1 : a+= 2) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
150+
// TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
151+
(xs[8] ? a+=1 : a) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
152+
// TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
153+
(xs[8] ? a : a+= 2) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
154+
// TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
155+
a = (xs[8] ? a+=1 : a+= 2); // no-warning
156+
a += (xs[8] ? a+=1 : a+= 2); // cxx11-warning {{unsequenced modification and access to 'a'}}
157+
// TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
158+
159+
(false ? a+=1 : a) = a; // no-warning
160+
(true ? a+=1 : a) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
161+
// TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
162+
(false ? a : a+=2) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
163+
// TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
164+
(true ? a : a+=2) = a; // no-warning
139165

140166
xs[8] && (++a + a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
141167
// cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
142168
xs[8] || (++a + a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
143169
// cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
144170

171+
((a++, false) || (a++, false)); // no-warning PR39779
172+
((a++, true) && (a++, true)); // no-warning PR39779
173+
174+
int i,j;
175+
(i = g1(), false) || (j = g2(i)); // no-warning PR22197
176+
(i = g1(), true) && (j = g2(i)); // no-warning PR22197
177+
178+
(a++, false) || (a++, false) || (a++, false) || (a++, false); // no-warning
179+
(a++, true) || (a++, true) || (a++, true) || (a++, true); // no-warning
180+
a = ((a++, false) || (a++, false) || (a++, false) || (a++, false)); // no-warning
181+
a = ((a++, true) && (a++, true) && (a++, true) && (a++, true)); // no-warning
182+
a = ((a++, false) || (a++, false) || (a++, false) || a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
183+
// TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
184+
a = ((a++, true) && (a++, true) && (a++, true) && a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
185+
// TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
186+
a = ((a++, false) || (a++, false) || (a++, false) || (a + a, false)); // no-warning
187+
a = ((a++, true) && (a++, true) && (a++, true) && (a + a, true)); // no-warning
188+
189+
a = (false && a++); // no-warning
190+
a = (true && a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
191+
// TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
192+
a = (true && ++a); // no-warning
193+
a = (true || a++); // no-warning
194+
a = (false || a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
195+
// TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
196+
a = (false || ++a); // no-warning
197+
198+
(a++) | (a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
199+
// cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
200+
(a++) & (a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
201+
// cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
202+
(a++) ^ (a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
203+
// cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
204+
145205
(__builtin_classify_type(++a) ? 1 : 0) + ++a; // ok
146206
(__builtin_constant_p(++a) ? 1 : 0) + ++a; // ok
147207
(__builtin_object_size(&(++a, a), 0) ? 1 : 0) + ++a; // ok

0 commit comments

Comments
 (0)