Skip to content

Commit f90d779

Browse files
committed
C++: Fix SimpleRangeAnalysis for AssignOperation
The range analysis wasn't producing useful bounds for `AssignOperation`s (`+=`, `-=`) unless their RHS involved a variable. This is because a shortcut was made in the `analyzableDef` predicate, which used to specify that an analyzable definition was one for which we'd specified the dependencies. But we can't distinguish between having _no dependencies_ and having _no specification of the dependencies_. The fix is to be more explicit about which definitions are analyzable. To avoid too much repetition I'm still calling out to `analyzableExpr` in the new code.
1 parent 8cbd497 commit f90d779

File tree

4 files changed

+19
-11
lines changed

4 files changed

+19
-11
lines changed

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

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -391,9 +391,17 @@ private predicate assignmentDef(RangeSsaDefinition def, StackVariable v, Expr ex
391391
)
392392
}
393393

394-
/** See comment above sourceDef. */
394+
/** See comment above assignmentDef. */
395395
private predicate analyzableDef(RangeSsaDefinition def, StackVariable v) {
396-
assignmentDef(def, v, _) or defDependsOnDef(def, v, _, _)
396+
assignmentDef(def, v, _)
397+
or
398+
analyzableExpr(def.(AssignOperation)) and
399+
v = def.getAVariable()
400+
or
401+
analyzableExpr(def.(CrementOperation)) and
402+
v = def.getAVariable()
403+
or
404+
phiDependsOnDef(def, v, _, _)
397405
}
398406

399407
/**

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -427,19 +427,19 @@
427427
| test.c:408:7:408:7 | i | 10 |
428428
| test.c:410:3:410:3 | i | -2147483648 |
429429
| test.c:411:3:411:3 | i | 10 |
430-
| test.c:412:7:412:7 | i | -2147483648 |
430+
| test.c:412:7:412:7 | i | 20 |
431431
| test.c:414:3:414:3 | i | -2147483648 |
432432
| test.c:415:3:415:3 | i | 40 |
433-
| test.c:416:7:416:7 | i | -2147483648 |
433+
| test.c:416:7:416:7 | i | 30 |
434434
| test.c:418:3:418:3 | i | -2147483648 |
435435
| test.c:418:7:418:7 | j | -2147483648 |
436436
| test.c:419:7:419:7 | i | 40 |
437437
| test.c:421:3:421:3 | i | -2147483648 |
438438
| test.c:421:8:421:8 | j | 40 |
439439
| test.c:422:7:422:7 | i | 50 |
440440
| test.c:424:3:424:3 | i | -2147483648 |
441-
| test.c:424:13:424:13 | j | -2147483648 |
442-
| test.c:425:7:425:7 | i | -2147483648 |
441+
| test.c:424:13:424:13 | j | 50 |
442+
| test.c:425:7:425:7 | i | 60 |
443443
| test.c:432:12:432:12 | a | 0 |
444444
| test.c:432:17:432:17 | a | 3 |
445445
| test.c:432:33:432:33 | b | 0 |

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -422,7 +422,7 @@ void test17() {
422422
out(i); // 50
423423

424424
i = 20 + (j -= 10);
425-
out(i); // 60 [BUG: the analysis thinks it's 2^-31 .. 2^31-1]
425+
out(i); // 60
426426
}
427427

428428
// Tests for unsigned multiplication.

cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/upperBound.expected

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -427,19 +427,19 @@
427427
| test.c:408:7:408:7 | i | 10 |
428428
| test.c:410:3:410:3 | i | 2147483647 |
429429
| test.c:411:3:411:3 | i | 10 |
430-
| test.c:412:7:412:7 | i | 2147483647 |
430+
| test.c:412:7:412:7 | i | 20 |
431431
| test.c:414:3:414:3 | i | 2147483647 |
432432
| test.c:415:3:415:3 | i | 40 |
433-
| test.c:416:7:416:7 | i | 2147483647 |
433+
| test.c:416:7:416:7 | i | 30 |
434434
| test.c:418:3:418:3 | i | 2147483647 |
435435
| test.c:418:7:418:7 | j | 2147483647 |
436436
| test.c:419:7:419:7 | i | 40 |
437437
| test.c:421:3:421:3 | i | 2147483647 |
438438
| test.c:421:8:421:8 | j | 40 |
439439
| test.c:422:7:422:7 | i | 50 |
440440
| test.c:424:3:424:3 | i | 2147483647 |
441-
| test.c:424:13:424:13 | j | 2147483647 |
442-
| test.c:425:7:425:7 | i | 2147483647 |
441+
| test.c:424:13:424:13 | j | 50 |
442+
| test.c:425:7:425:7 | i | 60 |
443443
| test.c:432:12:432:12 | a | 4294967295 |
444444
| test.c:432:17:432:17 | a | 4294967295 |
445445
| test.c:432:33:432:33 | b | 4294967295 |

0 commit comments

Comments
 (0)