C++: Remove safeFloor in simple range analysis#21113
Conversation
paldepind
changed the title
paldepind
changed the title
safeFloor in simple range analysis
paldepind
force-pushed
the
cpp/range-analysis-float-ceil
branch
from
7555440 to
f5ddb1d
Compare
There was a problem hiding this comment.
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
geoffw0
left a comment
There was a problem hiding this comment.
LGTM. Glad to see the back of safeFloor. 👍
Please check there are no surprises when the DCA run finishes before merging.
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
geoffw0
left a comment
There was a problem hiding this comment.
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: