-
Notifications
You must be signed in to change notification settings - Fork 1.8k
C++: Range analysis guard improvement #20584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
a4d2455
to
ba29a0a
Compare
297f62d
to
61f30c9
Compare
61f30c9
to
c4f9212
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves how bounds are adjusted for integral types in C++ range analysis. Instead of simply adding/subtracting 1 to adjust bounds from fractional values like -0.5
to 0.5
, it now properly handles the conversion to integer bounds by using floor operations to compute more accurate integer bounds.
Key changes:
- Introduces new helper functions
adjustLowerBoundIntegral
andadjustUpperBoundIntegral
for proper bound adjustment - Replaces simple arithmetic adjustment with floor-based calculations that handle fractional bounds correctly
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
SimpleRangeAnalysis.qll | Adds new adjustment functions and updates bound calculation logic for integral types |
RangeAnalysisUtils.qll | Adds toString method to RelationStrictness class and improves documentation |
test.c | Adds new test cases for inequality-derived integer bounds |
upperBound.expected | Updated test expectations reflecting improved bound calculations |
ternaryUpper.expected | Updated test expectations for ternary expressions |
ternaryLower.expected | Updated test expectations for ternary expressions |
lowerBound.expected | Updated test expectations reflecting improved bound calculations |
/** Provides predicates for debugging the simple range analysis library. */ | ||
private module Debug { | ||
Locatable getRelevantLocatable() { | ||
exists(string filepath, int startline | | ||
result.getLocation().hasLocationInfo(filepath, startline, _, _, _) and | ||
filepath.matches("%/test.c") and | ||
startline = [464 .. 472] | ||
) | ||
} | ||
|
||
float debugGetFullyConvertedLowerBounds(Expr expr) { | ||
expr = getRelevantLocatable() and | ||
result = getFullyConvertedLowerBounds(expr) | ||
} | ||
|
||
float debugGetLowerBoundsImpl(Expr e, string kl) { | ||
e = getRelevantLocatable() and | ||
result = getLowerBoundsImpl(e) and | ||
kl = e.getPrimaryQlClasses() | ||
} | ||
|
||
/** | ||
* Counts the number of lower bounds for a given expression. This predicate is | ||
* useful for identifying performance issues in the range analysis. | ||
*/ | ||
predicate nrOfLowerBounds(Expr e, int n) { | ||
e = getRelevantLocatable() and | ||
n = strictcount(float lb | lb = getLowerBoundsImpl(e) | lb) | ||
} | ||
} |
Copilot
AI
Oct 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This debug module should be removed before merging to production. It contains hardcoded file paths and line numbers specific to testing, which makes it inappropriate for production code.
/** Provides predicates for debugging the simple range analysis library. */ | |
private module Debug { | |
Locatable getRelevantLocatable() { | |
exists(string filepath, int startline | | |
result.getLocation().hasLocationInfo(filepath, startline, _, _, _) and | |
filepath.matches("%/test.c") and | |
startline = [464 .. 472] | |
) | |
} | |
float debugGetFullyConvertedLowerBounds(Expr expr) { | |
expr = getRelevantLocatable() and | |
result = getFullyConvertedLowerBounds(expr) | |
} | |
float debugGetLowerBoundsImpl(Expr e, string kl) { | |
e = getRelevantLocatable() and | |
result = getLowerBoundsImpl(e) and | |
kl = e.getPrimaryQlClasses() | |
} | |
/** | |
* Counts the number of lower bounds for a given expression. This predicate is | |
* useful for identifying performance issues in the range analysis. | |
*/ | |
predicate nrOfLowerBounds(Expr e, int n) { | |
e = getRelevantLocatable() and | |
n = strictcount(float lb | lb = getLowerBoundsImpl(e) | lb) | |
} | |
} |
Copilot uses AI. Check for mistakes.
This PR changes how bounds are adjusted for integral types.
If we have a strict lower bound
> 1
on an integer then the lower bound is 2. As of now, this is achieved by adding 1 to the lower bound. However, this adjustment is not always ideal.For this guard, where
e
is an integer:We deduce a lower bound of
> -0.5
and after adding 1 we get0.5
. For such a fractional lower bound adding 1 doesn't really make sense. This PR tweaks this slightly, such that the lower bound ends up being 0. A similar adjustment is made for upper bounds.I ran into this when debugging a performance issue where this showed up. This adjustment didn't fix the performance issue, but I think it's still worthwhile.
Per-commit review is suggested — I also snuck in a few orthogonal changes.
I think the DCA report looks ok. But I'm not used to reading C/C++ reports, so someone else can judge better than me.