Revert "Merge pull request #20645 from paldepind/cpp/range-analysis-m… #20775
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the range analysis widening logic by removing the bounds estimation mechanism that was designed to prevent combinatorial explosions. The changes remove complexity estimation-based widening while retaining recursive binary operation widening.
Key Changes:
- Removed the
BoundsEstimatemodule that estimated the number of bounds to determine when widening should be applied - Inlined the
widenLowerBoundandwidenUpperBoundhelper functions - Simplified widening to only apply to recursive binary operations, removing the predicates
varHasTooManyBoundsandexprHasTooManyBounds - Removed test cases and test files related to bounds estimation functionality
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll | Removed BoundsEstimate module (~330 lines), inlined widening helper functions, simplified widening conditions to only check for recursive binary operations, removed debug utilities |
| cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/test.c | Removed three test functions that were designed to trigger combinatorial explosion in bounds estimation |
| cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/nrOfBounds.ql | Deleted test file that queried the bounds estimation predicate |
| cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/upperBound.expected | Auto-generated test output updated to reflect removed test cases |
| cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/lowerBound.expected | Auto-generated test output updated to reflect removed test cases |
| cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/ternaryUpper.expected | Auto-generated test output updated to reflect removed test cases |
| cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/ternaryLower.expected | Auto-generated test output updated to reflect removed test cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result = | ||
| max(float widenLB | | ||
| widenLB = wideningLowerBounds(getVariableRangeType(v)) and | ||
| not widenLB > truncatedLB |
There was a problem hiding this comment.
The condition not widenLB > truncatedLB is logically equivalent to widenLB <= truncatedLB, which appears to be the intended semantics for widening lower bounds. However, this condition allows widenLB to be equal to truncatedLB, which means when multiple widening bounds match the condition, the max aggregate will select the one closest to (but not exceeding) truncatedLB. Consider using the more explicit condition widenLB <= truncatedLB for better readability.
| not widenLB > truncatedLB | |
| widenLB <= truncatedLB |
| result = | ||
| min(float widenUB | | ||
| widenUB = wideningUpperBounds(getVariableRangeType(v)) and | ||
| not widenUB < truncatedUB |
There was a problem hiding this comment.
The condition not widenUB < truncatedUB is logically equivalent to widenUB >= truncatedUB, which appears to be the intended semantics for widening upper bounds. However, this condition allows widenUB to be equal to truncatedUB, which means when multiple widening bounds match the condition, the min aggregate will select the one closest to (but not below) truncatedUB. Consider using the more explicit condition widenUB >= truncatedUB for better readability.
| not widenUB < truncatedUB | |
| widenUB >= truncatedUB |
| result = | ||
| max(float widenLB | | ||
| widenLB = wideningLowerBounds(expr.getUnspecifiedType()) and | ||
| not widenLB > newLB |
There was a problem hiding this comment.
The condition not widenLB > newLB is logically equivalent to widenLB <= newLB, which appears to be the intended semantics for widening lower bounds. However, this condition allows widenLB to be equal to newLB, which means when multiple widening bounds match the condition, the max aggregate will select the one closest to (but not exceeding) newLB. Consider using the more explicit condition widenLB <= newLB for better readability.
| not widenLB > newLB | |
| widenLB <= newLB |
| result = | ||
| min(float widenUB | | ||
| widenUB = wideningUpperBounds(expr.getUnspecifiedType()) and | ||
| not widenUB < newUB |
There was a problem hiding this comment.
The condition not widenUB < newUB is logically equivalent to widenUB >= newUB, which appears to be the intended semantics for widening upper bounds. However, this condition allows widenUB to be equal to newUB, which means when multiple widening bounds match the condition, the min aggregate will select the one closest to (but not below) newUB. Consider using the more explicit condition widenUB >= newUB for better readability.
| not widenUB < newUB | |
| widenUB >= newUB |
|
This still needs something like this: diff --git a/cpp/ql/lib/CHANGELOG.md b/cpp/ql/lib/CHANGELOG.md
index 390e3d4653b..62234b1dd9d 100644
--- a/cpp/ql/lib/CHANGELOG.md
+++ b/cpp/ql/lib/CHANGELOG.md
@@ -1,8 +1,6 @@
## 6.0.1
-### Bug Fixes
-
-* Improve performance of the range analysis in cases where it would otherwise take an exorbitant amount of time.
+No user-facing changes.
## 6.0.0
diff --git a/cpp/ql/lib/change-notes/released/6.0.1.md b/cpp/ql/lib/change-notes/released/6.0.1.md
index 7e8cfdb2562..35b17912c81 100644
--- a/cpp/ql/lib/change-notes/released/6.0.1.md
+++ b/cpp/ql/lib/change-notes/released/6.0.1.md
@@ -1,5 +1,3 @@
## 6.0.1
-### Bug Fixes
-
-* Improve performance of the range analysis in cases where it would otherwise take an exorbitant amount of time.
+No user-facing changes. |
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
Looks good from my end. This seems to revert what I'd expect. I'll update the changelog myself once this is merged, before I update the other release branch.
Reverts #20645
Reverting as this causes stable timeout regressions in certain projects (of various sizes).