-
Notifications
You must be signed in to change notification settings - Fork 1.9k
C++: Remove safeFloor in simple range analysis
#21113
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
C++: Remove safeFloor in simple range analysis
#21113
Conversation
safeFloor in simple range analysis
7555440 to
f5ddb1d
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 removes the safeFloor predicate from the simple range analysis library, which was a workaround for QL's floor() method operating on 32-bit integers. The new floorFloat predicate directly operates on float values without range limitations, enabling correct handling of fractional numbers greater than 2^32.
Key Changes:
- Removed the
safeFloorpredicate and its workaround logic - Replaced
safeFloorcalls with directfloorFloat()method calls on float expressions - Added test case demonstrating correct analysis of large numbers (2^53 - 1)
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll | Removed safeFloor predicate and updated right shift operations to use floorFloat() |
| cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/test.c | Added test case for shift operations on large numbers (2^53) |
| cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.cpp | Updated comments to reflect corrected analysis (removed "INCORRECT MESSAGE" notes) |
| cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.expected | Updated expected results with corrected integer bounds (removed .5 fractional values) |
| cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/*.expected | Updated test expectations with new line numbers and corrected bounds for large number handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
geoffw0
left a comment
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.
LGTM. Glad to see the back of safeFloor. 👍
Please check there are no surprises when the DCA run finishes before merging.
cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/test.c
Outdated
Show resolved
Hide resolved
Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
The DCA report seems fine, it shows something about a bad join, but I don't think it's related to the PR? |
geoffw0
left a comment
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.
The DCA report seems fine, it shows something about a bad join, but I don't think it's related to the PR?
I don't generally worry about such things unless there's an overall regression to be explained (which I don't see here). And yeah, the predicate its on doesn't seem related to your changes.
This PR removes the
safeFloorpredicate from the simple range analysis library. The predicate is a workaround for the fact that QL'sflooroperates on 32 bit integers. We no longer need this after the recently introducedfloorFloatpredicate, which operates directly onfloatwithout any loss in range.In addition to getting rid of
safeFloor, as the test shows, this also enables correct flooring on fractional numbers greater than 2^32. This problem was pointed out by @geoffw0 back in #3445: