C++: Divide number of bounds between branches for phi nodes#21329
Open
paldepind wants to merge 3 commits intogithub:mainfrom
Open
C++: Divide number of bounds between branches for phi nodes#21329paldepind wants to merge 3 commits intogithub:mainfrom
paldepind wants to merge 3 commits intogithub:mainfrom
Conversation
f041b27 to
e5c8f38
Compare
e5c8f38 to
d0681c6
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the heuristic for calculating the number of bounds for guard phi nodes in C++ range analysis. The change addresses an exponential growth issue that occurred when analyzing series of if-else statements with guards on the same variable.
Changes:
- Modified
nrOfBoundsPhiGuardfunction to use a new heuristic:(varBounds + 1) / 2instead of special-casing certain phi nodes - Added test case
repeated_if_else_statementsdemonstrating the fixed scenario with 11 consecutive if-else statements - Added helper predicates
countNrOfLowerBoundsandcountNrOfUpperBoundsinSimpleRangeAnalysisInternalmodule - Updated test query
nrOfBounds.qlto output actual bounds counts alongside estimates
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll |
Changed bounds estimation heuristic in nrOfBoundsPhiGuard from special-casing to dividing by 2; added helper predicates for counting actual bounds |
cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/test.c |
Added repeated_if_else_statements function demonstrating the exponential growth scenario that is now fixed |
cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/nrOfBounds.ql |
Enhanced query to also output actual lower and upper bounds counts for validation |
cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/*.expected |
Updated expected test outputs to reflect the improved bounds estimates (line numbers shifted due to new test case) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR changes the heuristic for how the number of bounds is calculated for a guard phi node.
This is an improvement in a few ways:
It avoids a special case where nodes that are both normal phi nodes and guard phi nodes needed special treatment.
I think this heuristic makes sense intuitively.
It fixes a problem where a series of if-else statements with guards on the same variable caused the number of bounds to exponentially increase while the actual range analysis did in fact not introduce any bounds.
The first commit adds an example of this. In the last commit the change to the expected file around
test.c:453totest.c:463show how we now handle this case better.This fixes a recent problem observed over in coding standards. See: Revert "C++: Accept test changes after github/codeql#21313." codeql-coding-standards#1041.
For reviewing the comment inside
nrOfBoundsPhiGuardshould help explain what is going on.