Skip to content

Commit 73c72f2

Browse files
nectonecto
authored
[analyzer] Keep alive short-circuiting condition subexpressions in a conditional (#100745)
Fix the false negative caused by state merging in the evaluation of a short-circuiting expression inside the condition of a ternary operator. The fixed symptom is that CSA always evaluates `(x || x) ? n : m` to `m`. This change forces the analyzer to consider all logical expressions prone to short-circuiting alive until the entire conditional expression is evaluated. Here is why. By default, LiveVariables analysis marks only direct subexpressions as live relative to any expression. So for `a ? b : c` it will consider `a`, `b`, and `c` alive when evaluating the ternary operator expression. To explore both possibilities opened by a ternary operator, it is important to keep something different about the exploded nodes created after the evaluation of its branches. These two nodes come to the same location, so they must have different states. Otherwise, they will be considered identical and can engender only one outcome. `ExprEngine::visitGuardedExpr` chooses the first predecessor exploded node to carry the value of the conditional expression. It works well in the case of a simple condition, because when `a ? b : c` is evaluated, `a` is kept alive, so the two branches differ in the value of `a`. However, before this patch is applied, this strategy breaks for `(x || x) ? n : m`. `x` is not a direct child of the ternary expression. Due to short-circuiting, once `x` is assumed to be `true`, evaluation jumps directly to `n` and then to the result of the entire ternary expression. Given that the result of the entire condition `(x || x)` is not constructed, and `x` is not kept alive, the difference between the path coming through `n` and through `m` disappears. As a result, exploded nodes coming from the "true expression" and the "false expression" engender identical successors and merge the execution paths.
1 parent 034d014 commit 73c72f2

File tree

3 files changed

+185
-1
lines changed

3 files changed

+185
-1
lines changed

‎clang/lib/Analysis/LiveVariables.cpp

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -214,6 +214,22 @@ static void AddLiveExpr(llvm::ImmutableSet<const Expr *> &Set,
214214
Set = F.add(Set, LookThroughExpr(E));
215215
}
216216

217+
/// Add as a live expression all individual conditions in a logical expression.
218+
/// For example, for the expression:
219+
/// "(a < b) || (c && d && ((e || f) != (g && h)))"
220+
/// the following expressions will be added as live:
221+
/// "a < b", "c", "d", "((e || f) != (g && h))"
222+
static void AddAllConditionalTerms(llvm::ImmutableSet<const Expr *> &Set,
223+
llvm::ImmutableSet<const Expr *>::Factory &F,
224+
const Expr *Cond) {
225+
AddLiveExpr(Set, F, Cond);
226+
if (auto const *BO = dyn_cast<BinaryOperator>(Cond->IgnoreParens());
227+
BO && BO->isLogicalOp()) {
228+
AddAllConditionalTerms(Set, F, BO->getLHS());
229+
AddAllConditionalTerms(Set, F, BO->getRHS());
230+
}
231+
}
232+
217233
void TransferFunctions::Visit(Stmt *S) {
218234
if (observer)
219235
observer->observeStmt(S, currentBlock, val);
@@ -313,7 +329,27 @@ void TransferFunctions::Visit(Stmt *S) {
313329
AddLiveExpr(val.liveExprs, LV.ESetFact, cast<ForStmt>(S)->getCond());
314330
return;
315331
}
316-
332+
case Stmt::ConditionalOperatorClass: {
333+
// Keep not only direct children alive, but also all the short-circuited
334+
// parts of the condition. Short-circuiting evaluation may cause the
335+
// conditional operator evaluation to skip the evaluation of the entire
336+
// condtion expression, so the value of the entire condition expression is
337+
// never computed.
338+
//
339+
// This makes a difference when we compare exploded nodes coming from true
340+
// and false expressions with no side effects: the only difference in the
341+
// state is the value of (part of) the condition.
342+
//
343+
// BinaryConditionalOperatorClass ('x ?: y') is not affected because it
344+
// explicitly calculates the value of the entire condition expression (to
345+
// possibly use as a value for the "true expr") even if it is
346+
// short-circuited.
347+
auto const *CO = cast<ConditionalOperator>(S);
348+
AddAllConditionalTerms(val.liveExprs, LV.ESetFact, CO->getCond());
349+
AddLiveExpr(val.liveExprs, LV.ESetFact, CO->getTrueExpr());
350+
AddLiveExpr(val.liveExprs, LV.ESetFact, CO->getFalseExpr());
351+
return;
352+
}
317353
}
318354

319355
// HACK + FIXME: What is this? One could only guess that this is an attempt to

‎clang/test/Analysis/live-stmts.cpp

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,3 +193,112 @@ void test_lambda_refcapture() {
193193
// CHECK-NEXT: [ B2 (live expressions at block exit) ]
194194
// CHECK-EMPTY:
195195
// CHECK-EMPTY:
196+
197+
int logicalOpInTernary(bool b) {
198+
return (b || b) ? 0 : 1;
199+
}
200+
201+
// [B6 (ENTRY)]
202+
// |
203+
// V
204+
// [B5 (b || ...)]
205+
// | \
206+
// | |
207+
// V V
208+
// [B4 (b||b)] ? [B2 (0)] : [B3 (1)]
209+
// \ /
210+
// ---|----
211+
// V
212+
// [B1] --> [B0 (EXIT)]
213+
// return
214+
215+
// CHECK: [ B0 (live expressions at block exit) ]
216+
// CHECK-EMPTY:
217+
// CHECK-EMPTY:
218+
// CHECK: [ B1 (live expressions at block exit) ]
219+
// CHECK-EMPTY:
220+
// CHECK-EMPTY:
221+
// CHECK: [ B2 (live expressions at block exit) ]
222+
// CHECK-EMPTY:
223+
// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
224+
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
225+
// CHECK-EMPTY:
226+
// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
227+
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
228+
// CHECK-EMPTY:
229+
// CHECK: BinaryOperator {{.*}} '_Bool' '||'
230+
// CHECK: |-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
231+
// CHECK: | `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
232+
// CHECK: `-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
233+
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
234+
// CHECK-EMPTY:
235+
// CHECK: IntegerLiteral {{.*}} 'int' 0
236+
// CHECK-EMPTY:
237+
// CHECK: IntegerLiteral {{.*}} 'int' 1
238+
// CHECK-EMPTY:
239+
// CHECK-EMPTY:
240+
// CHECK: [ B3 (live expressions at block exit) ]
241+
// CHECK-EMPTY:
242+
// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
243+
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
244+
// CHECK-EMPTY:
245+
// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
246+
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
247+
// CHECK-EMPTY:
248+
// CHECK: BinaryOperator {{.*}} '_Bool' '||'
249+
// CHECK: |-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
250+
// CHECK: | `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
251+
// CHECK: `-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
252+
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
253+
// CHECK-EMPTY:
254+
// CHECK: IntegerLiteral {{.*}} 'int' 0
255+
// CHECK-EMPTY:
256+
// CHECK: IntegerLiteral {{.*}} 'int' 1
257+
// CHECK-EMPTY:
258+
// CHECK-EMPTY:
259+
// CHECK: [ B4 (live expressions at block exit) ]
260+
// CHECK-EMPTY:
261+
// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
262+
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
263+
// CHECK-EMPTY:
264+
// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
265+
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
266+
// CHECK-EMPTY:
267+
// CHECK: BinaryOperator {{.*}} '_Bool' '||'
268+
// CHECK: |-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
269+
// CHECK: | `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
270+
// CHECK: `-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
271+
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
272+
// CHECK-EMPTY:
273+
// CHECK: IntegerLiteral {{.*}} 'int' 0
274+
// CHECK-EMPTY:
275+
// CHECK: IntegerLiteral {{.*}} 'int' 1
276+
// CHECK-EMPTY:
277+
// CHECK-EMPTY:
278+
// CHECK: [ B5 (live expressions at block exit) ]
279+
// CHECK-EMPTY:
280+
// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
281+
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
282+
// CHECK-EMPTY:
283+
// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
284+
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
285+
// CHECK-EMPTY:
286+
// CHECK: BinaryOperator {{.*}} '_Bool' '||'
287+
// CHECK: |-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
288+
// CHECK: | `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
289+
// CHECK: `-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
290+
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
291+
// CHECK-EMPTY:
292+
// CHECK: IntegerLiteral {{.*}} 'int' 0
293+
// CHECK-EMPTY:
294+
// CHECK: IntegerLiteral {{.*}} 'int' 1
295+
// CHECK-EMPTY:
296+
// CHECK-EMPTY:
297+
// CHECK: [ B6 (live expressions at block exit) ]
298+
// CHECK-EMPTY:
299+
// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
300+
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
301+
// CHECK-EMPTY:
302+
// CHECK: IntegerLiteral {{.*}} 'int' 0
303+
// CHECK-EMPTY:
304+
// CHECK: IntegerLiteral {{.*}} 'int' 1

‎clang/test/Analysis/short-circuiting-eval.cpp

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
// RUN: %clang_analyze_cc1 -analyzer-checker=core.DivideZero -verify %s
2+
3+
int div0LogicalOpInTernary(bool b1) {
4+
int y = (b1 || b1) ? 0 : 1;
5+
return 1 / y; // expected-warning {{Division by zero}}
6+
}
7+
8+
int div0LogicalAndArith(bool b1, int x) {
9+
int y = (b1 || (x < 3)) ? 0 : 1;
10+
return 1 / y; // expected-warning {{Division by zero}}
11+
}
12+
13+
int div0NestedLogicalOp(bool b1) {
14+
int y = (b1 && b1 || b1 && b1) ? 0 : 1;
15+
return 1 / y; // expected-warning {{Division by zero}}
16+
}
17+
18+
int div0TernaryInTernary(bool b) {
19+
int y = ((b || b) ? false : true) ? 0 : 1;
20+
return 1 / y; // expected-warning {{Division by zero}}
21+
}
22+
23+
int div0LogicalOpParensInTernary(bool b1) {
24+
int y = ((((b1)) || ((b1)))) ? 0 : 1;
25+
return 1 / y; // expected-warning {{Division by zero}}
26+
}
27+
28+
int div0LogicalOpInsideStExpr(bool b1) {
29+
int y = ({1; (b1 || b1);}) ? 0 : 1;
30+
// expected-warning@-1 {{expression result unused}}
31+
return 1 / y; // expected-warning {{Division by zero}}
32+
}
33+
34+
int div0StExprInsideLogicalOp(bool b1) {
35+
int y = (({1; b1;}) || ({1; b1;})) ? 0 : 1;
36+
// expected-warning@-1 {{expression result unused}}
37+
// expected-warning@-2 {{expression result unused}}
38+
return 1 / y; // expected-warning {{Division by zero}}
39+
}

0 commit comments

Comments
 (0)