Skip to content

Commit 399da68

Browse files
MathiasVPMathiasVP
authored
Merge pull request #4227 from jbj/SimpleRangeAnalysis-NotExpr
C++: Support `(bool)x` and `!x` in SimpleRangeAnalysis
2 parents 31495b8 + ceb198f commit 399da68

File tree

9 files changed

+235
-35
lines changed

9 files changed

+235
-35
lines changed

‎cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeAnalysisUtils.qll‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,65 @@ predicate eqOpWithSwapAndNegate(EqualityOperation cmp, Expr a, Expr b, boolean i
171171
eqOpWithSwap(cmp, a, b, branch.booleanNot()) and isEQ = false
172172
}
173173

174+
/**
175+
* Holds if `cmp` is an unconverted conversion of `a` to a Boolean that
176+
* evalutes to `isEQ` iff `a` is 0.
177+
*
178+
* Note that `a` can be `cmp` itself or a conversion thereof.
179+
*/
180+
private predicate eqZero(Expr cmp, Expr a, boolean isEQ) {
181+
// The `!a` expression tests `a` equal to zero when `a` is a number converted
182+
// to a Boolean.
183+
isEQ = true and
184+
exists(Expr notOperand | notOperand = cmp.(NotExpr).getOperand().getFullyConverted() |
185+
// In C++ code there will be a BoolConversion in `!myInt`
186+
a = notOperand.(BoolConversion).getExpr()
187+
or
188+
// In C code there is no conversion since there was no bool type before C99
189+
a = notOperand and
190+
not a instanceof BoolConversion // avoid overlap with the case above
191+
)
192+
or
193+
// The `(bool)a` expression tests `a` NOT equal to zero when `a` is a number
194+
// converted to a Boolean. To avoid overlap with the case above, this case
195+
// excludes conversions that are right below a `!`.
196+
isEQ = false and
197+
linearAccess(cmp, _, _, _) and
198+
// This test for `isCondition` implies that `cmp` is unconverted and that the
199+
// parent of `cfg` is not a `NotExpr` -- the CFG doesn't do branching from
200+
// inside `NotExpr`.
201+
cmp.isCondition() and
202+
// The GNU two-operand conditional expression is not supported for the
203+
// purpose of guards, but the value of the conditional expression itself is
204+
// modeled in the range analysis.
205+
not exists(ConditionalExpr cond | cmp = cond.getCondition() and cond.isTwoOperand()) and
206+
(
207+
// In C++ code there will be a BoolConversion in `if (myInt)`
208+
a = cmp.getFullyConverted().(BoolConversion).getExpr()
209+
or
210+
// In C code there is no conversion since there was no bool type before C99
211+
a = cmp.getFullyConverted() and
212+
not a instanceof BoolConversion // avoid overlap with the case above
213+
)
214+
}
215+
216+
/**
217+
* Holds if `branch` of `cmp` is taken when `a` compares `isEQ` to zero.
218+
*
219+
* Note that `a` can be `cmp` itself or a conversion thereof.
220+
*/
221+
predicate eqZeroWithNegate(Expr cmp, Expr a, boolean isEQ, boolean branch) {
222+
// The comparison for _equality_ to zero is on the `true` branch when `cmp`
223+
// compares equal to zero and on the `false` branch when `cmp` compares not
224+
// equal to zero.
225+
eqZero(cmp, a, branch) and isEQ = true
226+
or
227+
// The comparison for _inequality_ to zero is on the `false` branch when
228+
// `cmp` compares equal to zero and on the `true` branch when `cmp` compares
229+
// not equal to zero.
230+
eqZero(cmp, a, branch.booleanNot()) and isEQ = false
231+
}
232+
174233
/**
175234
* Holds if `expr` is equivalent to `p*v + q`, where `p` is a non-zero
176235
* number. This takes into account the associativity, commutativity and

‎cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeSSA.qll‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -40,21 +40,20 @@ library class RangeSSA extends SSAHelper {
4040
}
4141
}
4242

43-
private predicate guard_defn(
44-
VariableAccess v, ComparisonOperation guard, BasicBlock b, boolean branch
45-
) {
43+
private predicate guard_defn(VariableAccess v, Expr guard, BasicBlock b, boolean branch) {
4644
guardCondition(guard, v, branch) and
4745
guardSuccessor(guard, branch, b)
4846
}
4947

50-
private predicate guardCondition(ComparisonOperation guard, VariableAccess v, boolean branch) {
48+
private predicate guardCondition(Expr guard, VariableAccess v, boolean branch) {
5149
exists(Expr lhs | linearAccess(lhs, v, _, _) |
5250
relOpWithSwapAndNegate(guard, lhs, _, _, _, branch) or
53-
eqOpWithSwapAndNegate(guard, lhs, _, _, branch)
51+
eqOpWithSwapAndNegate(guard, lhs, _, _, branch) or
52+
eqZeroWithNegate(guard, lhs, _, branch)
5453
)
5554
}
5655

57-
private predicate guardSuccessor(ComparisonOperation guard, boolean branch, BasicBlock succ) {
56+
private predicate guardSuccessor(Expr guard, boolean branch, BasicBlock succ) {
5857
branch = true and succ = guard.getATrueSuccessor()
5958
or
6059
branch = false and succ = guard.getAFalseSuccessor()
@@ -98,7 +97,7 @@ class RangeSsaDefinition extends ControlFlowNodeBase {
9897
* If this definition is a phi node corresponding to a guard,
9998
* then return the variable and the guard.
10099
*/
101-
predicate isGuardPhi(VariableAccess v, ComparisonOperation guard, boolean branch) {
100+
predicate isGuardPhi(VariableAccess v, Expr guard, boolean branch) {
102101
guard_defn(v, guard, this, branch)
103102
}
104103

‎cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll‎

Lines changed: 35 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -427,11 +427,11 @@ private predicate exprDependsOnDef(Expr e, RangeSsaDefinition srcDef, StackVaria
427427
private predicate phiDependsOnDef(
428428
RangeSsaDefinition phi, StackVariable v, RangeSsaDefinition srcDef, StackVariable srcVar
429429
) {
430-
exists(VariableAccess access, ComparisonOperation guard |
430+
exists(VariableAccess access, Expr guard |
431431
access = v.getAnAccess() and
432432
phi.isGuardPhi(access, guard, _)
433433
|
434-
exprDependsOnDef(guard.getAnOperand(), srcDef, srcVar) or
434+
exprDependsOnDef(guard.(ComparisonOperation).getAnOperand(), srcDef, srcVar) or
435435
exprDependsOnDef(access, srcDef, srcVar)
436436
)
437437
or
@@ -1132,9 +1132,7 @@ private float boolConversionUpperBound(Expr expr) {
11321132
* use the guard to deduce that the lower bound is 2 inside the block.
11331133
*/
11341134
private float getPhiLowerBounds(StackVariable v, RangeSsaDefinition phi) {
1135-
exists(
1136-
VariableAccess access, ComparisonOperation guard, boolean branch, float defLB, float guardLB
1137-
|
1135+
exists(VariableAccess access, Expr guard, boolean branch, float defLB, float guardLB |
11381136
access = v.getAnAccess() and
11391137
phi.isGuardPhi(access, guard, branch) and
11401138
lowerBoundFromGuard(guard, access, guardLB, branch) and
@@ -1146,23 +1144,21 @@ private float getPhiLowerBounds(StackVariable v, RangeSsaDefinition phi) {
11461144
or
11471145
exists(VariableAccess access, float neConstant, float lower |
11481146
isNEPhi(v, phi, access, neConstant) and
1149-
lower = getFullyConvertedLowerBounds(access) and
1147+
lower = getTruncatedLowerBounds(access) and
11501148
if lower = neConstant then result = lower + 1 else result = lower
11511149
)
11521150
or
11531151
exists(VariableAccess access |
11541152
isUnsupportedGuardPhi(v, phi, access) and
1155-
result = getFullyConvertedLowerBounds(access)
1153+
result = getTruncatedLowerBounds(access)
11561154
)
11571155
or
11581156
result = getDefLowerBounds(phi.getAPhiInput(v), v)
11591157
}
11601158

11611159
/** See comment for `getPhiLowerBounds`, above. */
11621160
private float getPhiUpperBounds(StackVariable v, RangeSsaDefinition phi) {
1163-
exists(
1164-
VariableAccess access, ComparisonOperation guard, boolean branch, float defUB, float guardUB
1165-
|
1161+
exists(VariableAccess access, Expr guard, boolean branch, float defUB, float guardUB |
11661162
access = v.getAnAccess() and
11671163
phi.isGuardPhi(access, guard, branch) and
11681164
upperBoundFromGuard(guard, access, guardUB, branch) and
@@ -1174,13 +1170,13 @@ private float getPhiUpperBounds(StackVariable v, RangeSsaDefinition phi) {
11741170
or
11751171
exists(VariableAccess access, float neConstant, float upper |
11761172
isNEPhi(v, phi, access, neConstant) and
1177-
upper = getFullyConvertedUpperBounds(access) and
1173+
upper = getTruncatedUpperBounds(access) and
11781174
if upper = neConstant then result = upper - 1 else result = upper
11791175
)
11801176
or
11811177
exists(VariableAccess access |
11821178
isUnsupportedGuardPhi(v, phi, access) and
1183-
result = getFullyConvertedUpperBounds(access)
1179+
result = getTruncatedUpperBounds(access)
11841180
)
11851181
or
11861182
result = getDefUpperBounds(phi.getAPhiInput(v), v)
@@ -1334,7 +1330,7 @@ private predicate unanalyzableDefBounds(RangeSsaDefinition def, StackVariable v,
13341330
* inferences about `v`.
13351331
*/
13361332
bindingset[guard, v, branch]
1337-
predicate nonNanGuardedVariable(ComparisonOperation guard, VariableAccess v, boolean branch) {
1333+
predicate nonNanGuardedVariable(Expr guard, VariableAccess v, boolean branch) {
13381334
getVariableRangeType(v.getTarget()) instanceof IntegralType
13391335
or
13401336
getVariableRangeType(v.getTarget()) instanceof FloatingPointType and
@@ -1353,9 +1349,7 @@ predicate nonNanGuardedVariable(ComparisonOperation guard, VariableAccess v, boo
13531349
* predicate uses the bounds information for `r` to compute a lower bound
13541350
* for `v`.
13551351
*/
1356-
private predicate lowerBoundFromGuard(
1357-
ComparisonOperation guard, VariableAccess v, float lb, boolean branch
1358-
) {
1352+
private predicate lowerBoundFromGuard(Expr guard, VariableAccess v, float lb, boolean branch) {
13591353
exists(float childLB, RelationStrictness strictness |
13601354
boundFromGuard(guard, v, childLB, true, strictness, branch)
13611355
|
@@ -1375,9 +1369,7 @@ private predicate lowerBoundFromGuard(
13751369
* predicate uses the bounds information for `r` to compute a upper bound
13761370
* for `v`.
13771371
*/
1378-
private predicate upperBoundFromGuard(
1379-
ComparisonOperation guard, VariableAccess v, float ub, boolean branch
1380-
) {
1372+
private predicate upperBoundFromGuard(Expr guard, VariableAccess v, float ub, boolean branch) {
13811373
exists(float childUB, RelationStrictness strictness |
13821374
boundFromGuard(guard, v, childUB, false, strictness, branch)
13831375
|
@@ -1397,7 +1389,7 @@ private predicate upperBoundFromGuard(
13971389
* `linearBoundFromGuard`.
13981390
*/
13991391
private predicate boundFromGuard(
1400-
ComparisonOperation guard, VariableAccess v, float boundValue, boolean isLowerBound,
1392+
Expr guard, VariableAccess v, float boundValue, boolean isLowerBound,
14011393
RelationStrictness strictness, boolean branch
14021394
) {
14031395
exists(float p, float q, float r, boolean isLB |
@@ -1410,6 +1402,15 @@ private predicate boundFromGuard(
14101402
or
14111403
p < 0 and isLowerBound = isLB.booleanNot()
14121404
)
1405+
or
1406+
// When `!e` is true, we know that `0 <= e <= 0`
1407+
exists(float p, float q, Expr e |
1408+
linearAccess(e, v, p, q) and
1409+
eqZeroWithNegate(guard, e, true, branch) and
1410+
boundValue = (0.0 - q) / p and
1411+
isLowerBound = [false, true] and
1412+
strictness = Nonstrict()
1413+
)
14131414
}
14141415

14151416
/**
@@ -1487,6 +1488,15 @@ private predicate isNEPhi(
14871488
linearAccess(linearExpr, access, p, q) and
14881489
neConstant = (r - q) / p
14891490
)
1491+
or
1492+
exists(Expr op, boolean branch, Expr linearExpr, float p, float q |
1493+
access.getTarget() = v and
1494+
phi.isGuardPhi(access, op, branch) and
1495+
eqZeroWithNegate(op, linearExpr, false, branch) and
1496+
v.getUnspecifiedType() instanceof IntegralOrEnumType and // Float `!` is too imprecise
1497+
linearAccess(linearExpr, access, p, q) and
1498+
neConstant = (0.0 - q) / p
1499+
)
14901500
}
14911501

14921502
/**
@@ -1496,10 +1506,13 @@ private predicate isNEPhi(
14961506
* compile-time constant.
14971507
*/
14981508
private predicate isUnsupportedGuardPhi(Variable v, RangeSsaDefinition phi, VariableAccess access) {
1499-
exists(ComparisonOperation cmp, boolean branch |
1509+
exists(Expr cmp, boolean branch |
1510+
eqOpWithSwapAndNegate(cmp, _, _, false, branch)
1511+
or
1512+
eqZeroWithNegate(cmp, _, false, branch)
1513+
|
15001514
access.getTarget() = v and
15011515
phi.isGuardPhi(access, cmp, branch) and
1502-
eqOpWithSwapAndNegate(cmp, _, _, false, branch) and
15031516
not isNEPhi(v, phi, access, _)
15041517
)
15051518
}

‎cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/lowerBound.expected‎

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -540,7 +540,7 @@
540540
| test.c:548:9:548:9 | n | 0 |
541541
| test.c:551:8:551:8 | n | 0 |
542542
| test.c:552:9:552:9 | n | 0 |
543-
| test.c:554:9:554:9 | n | 0 |
543+
| test.c:554:9:554:9 | n | 1 |
544544
| test.c:557:10:557:10 | n | 0 |
545545
| test.c:558:5:558:5 | n | 1 |
546546
| test.c:561:7:561:7 | n | 0 |
@@ -549,7 +549,7 @@
549549
| test.c:569:9:569:9 | n | 0 |
550550
| test.c:571:9:571:9 | n | 1 |
551551
| test.c:574:7:574:7 | n | 0 |
552-
| test.c:575:9:575:9 | n | 0 |
552+
| test.c:575:9:575:9 | n | 1 |
553553
| test.c:577:9:577:9 | n | 0 |
554554
| test.c:580:10:580:10 | n | 0 |
555555
| test.c:581:5:581:5 | n | 1 |
@@ -563,6 +563,25 @@
563563
| test.c:601:7:601:7 | n | -32768 |
564564
| test.c:601:22:601:22 | n | -32767 |
565565
| test.c:602:9:602:9 | n | -32766 |
566+
| test.c:605:7:605:7 | n | -32768 |
567+
| test.c:606:5:606:5 | n | 0 |
568+
| test.c:606:10:606:10 | n | 1 |
569+
| test.c:606:14:606:14 | n | 0 |
570+
| test.c:607:6:607:6 | n | 0 |
571+
| test.c:607:10:607:10 | n | 0 |
572+
| test.c:607:14:607:14 | n | 1 |
573+
| test.c:618:7:618:8 | ss | -32768 |
574+
| test.c:619:9:619:10 | ss | 0 |
575+
| test.c:622:7:622:8 | ss | -32768 |
576+
| test.c:623:9:623:10 | ss | -32768 |
577+
| test.c:626:14:626:15 | us | 0 |
578+
| test.c:627:9:627:10 | us | 0 |
579+
| test.c:630:14:630:15 | us | 0 |
580+
| test.c:631:9:631:10 | us | 0 |
581+
| test.c:634:7:634:8 | ss | -32768 |
582+
| test.c:635:9:635:10 | ss | -32768 |
583+
| test.c:638:7:638:8 | ss | -32768 |
584+
| test.c:639:9:639:10 | ss | -1 |
566585
| test.cpp:10:7:10:7 | b | -2147483648 |
567586
| test.cpp:11:5:11:5 | x | -2147483648 |
568587
| test.cpp:13:10:13:10 | x | -2147483648 |
@@ -616,3 +635,16 @@
616635
| test.cpp:97:10:97:10 | i | -2147483648 |
617636
| test.cpp:97:22:97:22 | i | -2147483648 |
618637
| test.cpp:98:5:98:5 | i | -2147483648 |
638+
| test.cpp:105:7:105:7 | n | -32768 |
639+
| test.cpp:108:7:108:7 | n | 0 |
640+
| test.cpp:109:5:109:5 | n | 1 |
641+
| test.cpp:111:5:111:5 | n | 0 |
642+
| test.cpp:114:8:114:8 | n | 0 |
643+
| test.cpp:115:5:115:5 | n | 0 |
644+
| test.cpp:117:5:117:5 | n | 1 |
645+
| test.cpp:120:3:120:3 | n | 0 |
646+
| test.cpp:120:8:120:8 | n | 1 |
647+
| test.cpp:120:12:120:12 | n | 0 |
648+
| test.cpp:121:4:121:4 | n | 0 |
649+
| test.cpp:121:8:121:8 | n | 0 |
650+
| test.cpp:121:12:121:12 | n | 1 |

‎cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/ternaryLower.expected‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,3 +13,7 @@
1313
| test.c:386:10:386:21 | ... ? ... : ... | 100.0 | 100.0 | 5.0 |
1414
| test.c:387:10:387:38 | ... ? ... : ... | 0.0 | 100.0 | 5.0 |
1515
| test.c:394:20:394:36 | ... ? ... : ... | 0.0 | 0.0 | 100.0 |
16+
| test.c:606:5:606:14 | ... ? ... : ... | 0.0 | 1.0 | 0.0 |
17+
| test.c:607:5:607:14 | ... ? ... : ... | 0.0 | 0.0 | 1.0 |
18+
| test.cpp:120:3:120:12 | ... ? ... : ... | 0.0 | 1.0 | 0.0 |
19+
| test.cpp:121:3:121:12 | ... ? ... : ... | 0.0 | 0.0 | 1.0 |

‎cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/ternaryUpper.expected‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,3 +13,7 @@
1313
| test.c:386:10:386:21 | ... ? ... : ... | 4.294967295E9 | 4.294967295E9 | 5.0 |
1414
| test.c:387:10:387:38 | ... ? ... : ... | 255.0 | 4.294967295E9 | 5.0 |
1515
| test.c:394:20:394:36 | ... ? ... : ... | 100.0 | 99.0 | 100.0 |
16+
| test.c:606:5:606:14 | ... ? ... : ... | 32767.0 | 32767.0 | 0.0 |
17+
| test.c:607:5:607:14 | ... ? ... : ... | 32767.0 | 0.0 | 32767.0 |
18+
| test.cpp:120:3:120:12 | ... ? ... : ... | 32767.0 | 32767.0 | 0.0 |
19+
| test.cpp:121:3:121:12 | ... ? ... : ... | 32767.0 | 0.0 | 32767.0 |

‎cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/test.c‎

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -551,7 +551,7 @@ int notequal_type_endpoint(unsigned n) {
551551
if (!n) {
552552
out(n); // 0 .. 0
553553
} else {
554-
out(n); // 1 .. [BUG: lower bound is deduced to be 0]
554+
out(n); // 1 ..
555555
}
556556

557557
while (n != 0) {
@@ -572,7 +572,7 @@ void notequal_refinement(short n) {
572572
}
573573

574574
if (n) {
575-
out(n); // 1 .. [BUG: lower bound is deduced to be 0]
575+
out(n); // 1 ..
576576
} else {
577577
out(n); // 0 .. 0
578578
}
@@ -601,4 +601,41 @@ void notequal_variations(short n, float f) {
601601
if (n != -32768 && n != -32767) {
602602
out(n); // -32766 ..
603603
}
604+
605+
if (n >= 0) {
606+
n ? n : n; // ? 1.. : 0..0
607+
!n ? n : n; // ? 0..0 : 1..
608+
}
609+
}
610+
611+
void two_bounds_from_one_test(short ss, unsigned short us) {
612+
// These tests demonstrate how the range analysis is often able to deduce
613+
// both an upper bound and a lower bound even when there is only one
614+
// inequality in the source. For example `signedInt < 4U` establishes that
615+
// `signedInt >= 0` since if `signedInt` were negative then it would be
616+
// greater than 4 in the unsigned comparison.
617+
618+
if (ss < sizeof(int)) { // Lower bound added in `linearBoundFromGuard`
619+
out(ss); // 0 .. 3
620+
}
621+
622+
if (ss < 0x8001) { // Lower bound removed in `getDefLowerBounds`
623+
out(ss); // -32768 .. 32767
624+
}
625+
626+
if ((short)us >= 0) {
627+
out(us); // 0 .. 32767
628+
}
629+
630+
if ((short)us >= -1) {
631+
out(us); // 0 .. 65535
632+
}
633+
634+
if (ss >= sizeof(int)) { // test is true for negative numbers
635+
out(ss); // -32768 .. 32767
636+
}
637+
638+
if (ss + 1 < sizeof(int)) {
639+
out(ss); // -1 .. 2
640+
}
604641
}

0 commit comments

Comments
 (0)