fix(strings): (int) over a numeric string caps like PHP instead of wrapping - #1066
Guikingone wants to merge 3 commits into
Conversation
|
|
The one red check here is not this change. against This PR touches #1024 is exactly this: it raises the bound to 20 ms — better than 2x over these samples, still ~6.6x below the gap the defect produces — and, because widening a tolerance can only move a test toward vacuous, it adds an assertion that the in-callback burn measured from the capture exceeds the slack by 2x, so a fully misbilled callback can never satisfy the comparison. Its comment already cites the 7.80 ms sample from run 1; run 2's 7.42 ms is a third data point. Re-running does not help — it has now been re-run once and reproduced. #1024 should land first, then this one goes green on a rebase. |
…apping
PHP applies two DIFFERENT rules, and the string path was using the wrong one. A
float VALUE is reduced modulo 2^64 -- `(int)1e19` is negative in PHP too, and
`__rt_php_float_to_int` is right to do that. A numeric STRING is CAPPED. Routing
the string through the wrapping helper produced silently wrong arithmetic on a very
common operation:
(int)"1e19" php: 9223372036854775807 was: -8446744073709551616
(int)"-1e19" php: -9223372036854775808 was: 8446744073709551616
(int)"1.8e19" php: 9223372036854775807 was: -446744073709551616
(int)"1e100" php: 9223372036854775807 was: 0
(int)"1e308" php: 9223372036854775807 was: 0
The reverse failure sat on the integer-form path. `strtoll` saturates a 310-digit
integer to PHP_INT_MAX, but PHP classifies a string whose value overflows the double
as IS_DOUBLE, and INF casts to 0:
(int)str_repeat("1", 310) php: 0 was: 9223372036854775807
so the non-finite check has to come BEFORE the integer/float form choice -- that
string is integer-form.
AArch64 needs no bounds of its own: `fcvtzs` saturates by definition, which is
exactly PHP's cap for every finite double. x86_64's `cvttsd2si` reports the "integer
indefinite" pattern instead, which is already correct for a negative overflow; only
a positive one has to be turned into PHP_INT_MAX.
The integer-form parse is left alone, and one fixture guards that: 2^53+1 is exactly
representable as an integer but not as a double, so capping the parsed double instead
of taking `strtoll`'s answer would round it down by one.
23 shapes verified byte-identical to PHP 8.5.10, and the same values through
`intval()`, `settype()`, mixed-array arithmetic and loose comparison. Both fixtures
fail without the fix. x86_64 assembles clean for x86_64-unknown-linux-gnu.
Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
CI caught the first version of this fix with an invariant I had not noticed: `test_runtime_float_consumers_call_the_shared_helper` requires every runtime float-to-int consumer to route through `__rt_php_float_to_int` rather than open-code the conversion, because a bare `fcvtzs` / `cvttsd2si` is where the per-target `(int)NAN` and `$a[INF]` divergence came from -- the two disagree with each other on NaN and on overflow. The fix inlined exactly such a conversion. It was guarded (non-finite handled first, so the hardware difference could not show), but the invariant is worth more than the guard: the next person to add a numeric-string consumer would have had a second open-coded copy to get right. So the cap becomes `__rt_php_float_to_int_cap`, beside the wrapping helper it deliberately is not. Both PHP rules now live in `runtime::numeric`, named, with the reason each exists, and `__rt_str_to_int` calls one of them. Two tests pin it: that `__rt_str_to_int` reaches the cap through the helper and contains no bare conversion, and that the helper checks non-finite BEFORE capping -- capping first would answer PHP_INT_MAX for `(int)"1e309"" instead of 0. Same 23 shapes still byte-identical to PHP 8.5.10; math 92, --lib 1680 pass. Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
ead76a3 to
68c83e2
Compare
| emitter.instruction("ret"); // return the PHP integer value in x9 | ||
| } | ||
|
|
||
| /// Emits the `__rt_php_float_to_int_cap` runtime helper for the active target. |
There was a problem hiding this comment.
Runtime semantics lack documentation
This adds a separate numeric-string conversion helper and changes string casts to cap values instead of applying the float-value wrapping rule. The repository requires compiler-internal runtime changes to be documented in the relevant docs/internals/ page. Please document why numeric strings use the capping helper while float values retain modulo-2^64 conversion; this requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/codegen_support/runtime/numeric.rs
Line: 84
Comment:
**Runtime semantics lack documentation**
This adds a separate numeric-string conversion helper and changes string casts to cap values instead of applying the float-value wrapping rule. The repository requires compiler-internal runtime changes to be documented in the relevant `docs/internals/` page. Please document why numeric strings use the capping helper while float values retain modulo-2^64 conversion; this requirement must be satisfied before merging.
**Context Used:** AGENTS.md ([source](https://github.com/illegalstudio/elephc/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…separate Raised in review. The runtime page listed `__rt_php_float_to_int` among the array/scalar entry points and said nothing about the capping sibling this PR adds, so the reason a numeric string does not use the wrapping helper was only in the code. Documents the rule table -- a float VALUE reduces modulo 2^64, a numeric STRING caps -- with php-src's answers for `(int)1e19` and `(int)"1e19"`, and the reason both helpers check for NaN and the infinities before converting: `fcvtzs` saturates while `cvttsd2si` reports its indefinite pattern, which is what made a bare conversion at a call site produce a per-target `(int)NAN`. Values verified against host PHP 8.5.10. Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
|
Addressed in It now documents the rule table and why the split exists:
plus the reason both helpers check NaN and the infinities before converting: |
Found while investigating #953, which lists
(int)$sas one of the paths into the unbounded__rt_cstrbuffer. The buffer is a separate problem; on the way there, the values turned outto be wrong.
PHP applies two different rules
A float value is reduced modulo 2^64. A numeric string is capped. elephc was routing
the string through the float rule.
__rt_php_float_to_intimplements the float rule correctly and is untouched;__rt_str_to_intjust should not have been calling it.
What was wrong
(int)"1e19"9223372036854775807-8446744073709551616(int)"-1e19"-92233720368547758088446744073709551616(int)"1.8e19"9223372036854775807-446744073709551616(int)"1e100"92233720368547758070(int)"1e308"92233720368547758070(int)str_repeat("1", 310)09223372036854775807(int)("-" . str_repeat("1", 310))0-9223372036854775808The wrapping cases are the ones that matter:
(int)$_GET['page']on an oversized value produceda large negative number instead of
PHP_INT_MAX, with no warning.The last two are the reverse failure, and they sit on the integer-form path:
strtollsaturates a 310-digit integer to
PHP_INT_MAX, but PHP classifies a string whose valueoverflows the double as
IS_DOUBLE, and INF casts to 0. So the non-finite check has to runbefore the integer/float form choice — that string is integer-form.
The fix
0, checked first, for either form.fcvtzssaturates bydefinition, which is exactly PHP's cap for every finite double. x86_64's
cvttsd2sireportsthe "integer indefinite" pattern instead — already correct for a negative overflow, so only a
positive one is turned into
PHP_INT_MAX.strtoll's exact value.That last point is load-bearing and has its own fixture:
2^53 + 1is exactly representable asan integer but not as a double, so capping the parsed double instead of taking
strtoll'sanswer would round
(int)"9007199254740993"down by one.Verification
…807,…808,…809),the finite/INF boundary (309 vs 310 digits), both signs, leading whitespace, and float-form
spellings.
intval(),settype(), mixed-array arithmetic and loose comparison.x86_64-unknown-linux-gnu;--lib1680,strings::392 andoperators::127 pass.🤖 Generated with Claude Code
https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr