Fix --nan-check false positive for finite/+INFINITY division#8745
Open
tautschnig wants to merge 2 commits into
Open
Fix --nan-check false positive for finite/+INFINITY division#8745tautschnig wants to merge 2 commits into
tautschnig wants to merge 2 commits into
Conversation
81bf5e7 to
4e48600
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #8745 +/- ##
===========================================
- Coverage 80.59% 80.59% -0.01%
===========================================
Files 1711 1711
Lines 189454 189461 +7
Branches 73 73
===========================================
+ Hits 152697 152702 +5
- Misses 36757 36759 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1871271 to
5578125
Compare
5578125 to
2969dff
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a --nan-check false positive by aligning division NaN detection with IEEE 754 rules (finite/∞ is ±0, only ∞/∞ yields NaN), and adds regressions around infinity arithmetic.
Changes:
- Update NaN-check logic for division to flag only
0/0andinf/inf. - Extend SMT2 parser support with
fp.isNegative/fp.isPositive. - Add/adjust regression tests and math-library stubs for
__isfinite*.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util/ieee_float.cpp | Updates division comment to reflect IEEE 754 behavior. |
| src/solvers/smt2/smt2_parser.cpp | Adds parsing for fp.isNegative and fp.isPositive. |
| src/ansi-c/library_check.sh | Excludes __isfinite* symbols from library check list. |
| src/ansi-c/library/math.c | Adds __isfinite, __isfinitef, __isfinitel wrappers. |
| src/ansi-c/goto-conversion/goto_check_c.cpp | Fixes NaN check condition for division (inf/inf only). |
| src/ansi-c/c_typecheck_expr.cpp | Treats __builtin_isfinite as a special function. |
| regression/cbmc/float-nan-check/test.desc | Updates expected NaN-check failure pattern for division. |
| regression/cbmc/float-nan-check/main.c | Adjusts regression to use inf/inf instead of n/inf. |
| regression/cbmc/float-inf-div-inf/test.desc | Adds new regression expectations for inf/inf NaN checks. |
| regression/cbmc/float-inf-div-inf/main.c | New test exercising ±inf / ±inf producing NaN. |
| regression/cbmc/float-finite-div-infinity/test.desc | New regression ensuring finite/∞ does not trigger NaN checks. |
| regression/cbmc/float-finite-div-infinity/main.c | New test asserting finite/∞ results in signed zero. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2969dff to
5702d00
Compare
BSD systems appear to use `__` prefixed variants of several functions. Define these as needed. Also, avoid handling some GCC-style `__builtin_`-prefixed functions via models when others are done directly in the type checker: do all of them in the type checker. In addition to `__builtin_isfinite`, recognise its older glibc internal aliases `__builtin_finite`, `__builtin_finitef` and `__builtin_finitel` in the same isfinite_exprt branch of the type checker. Some glibc `<bits/mathcalls-helper-functions.h>` headers expose the legacy `finite()` API via these builtins, and without recognition CBMC would otherwise treat them as body-less functions returning nondet. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
Two fixes in goto_check_ct::nan_check: 1. Division: the existing condition treated x/inf as a NaN-producing operation, but per IEEE 754-2019 Section 6.1, finite/inf is +/-0, not NaN. The only div operations that produce NaN are 0/0 and inf/inf. Replace the over-broad isinf(op1) check with the conjunction isinf(op0) && isinf(op1). 2. Multiplication: the zero_times_inf clause was a copy-paste of inf_times_zero (both compared op1 against zero and op0 against isinf), so --nan-check failed to flag '0 * inf' when the zero was on the LHS. Fix by swapping the operand sides in zero_times_inf. The companion ieee_float.cpp comment update brings the runtime constant evaluator's comment in line with the now-correct semantics (it already evaluated both directions correctly; only the comment was misleading). New regression tests: - regression/cbmc/float-finite-div-infinity: positive test that finite/inf produces +/-0.0 with --nan-check enabled, no NaN assertion fired. - regression/cbmc/float-inf-div-inf: positive test that inf/inf is flagged as NaN under all four sign permutations. - regression/cbmc/float-nan-check: replace the (now-incorrect) n/inf case with inf/inf, and add a 'myzero * myinf' assertion that the buggy mult-branch was missing -- without fix 2, that assertion is not generated and the test fails. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
5702d00 to
73ed258
Compare
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.
Depends-on: #9020
Fixes: #8634