diff --git a/CLAUDE.md b/CLAUDE.md index 5a9418cc7..9a7ed53d4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1206,7 +1206,19 @@ silent: (NTS)`, and `php-smoke.php` is **44 of 45** on the VM -- PHP_INT_SIZE 8, strlen, max, str_repeat at 100k, preg, json, and `/net/tcp/stats` read as 404 bytes. Notes in `docs/notes/php.md`.** -***THE ONE THAT MATTERED WAS `exit()`, AND IT WAS kencc's 32-BIT +***AND THE SECOND ONE WAS EVERY MULTIPLICATION.*** +`zend_multiply.h:135`'s fallback arm -- the one this port takes -- +declared the product `long`, which is 64 bits on LP64 and **32 +here**, so any product over 2**32 was truncated, the overflow check +fired on an overflow that had not happened, and PHP returned a +FLOAT: `2000000 * 2000000` was `float(4.0E+12)`. *Every other arm in +that file is guarded by `SIZEOF_LONG == SIZEOF_ZEND_LONG`; that one +assumes it.* Found sideways, because php compiles unary minus as +`(-1) * x` -- **the smoke test covered `+`, `-`, `max`, `strlen` and +`str_repeat` and not `*`**, and a test with five operators and not +the sixth catches the sixth only by luck. Section 3b asks directly +now, with two cases a never-report-overflow fix would fail. +***THE ONE BEFORE IT WAS `exit()`, AND IT WAS kencc's 32-BIT `sizeof`***: `zend_object_properties_size` is `sizeof(zval) * (count - 1)` with count 0, which C makes 2**64-16 and kencc makes 4294967280 by multiplying in 32 bits and zero-extending -- so every `exit()` asked diff --git a/docs/notes/php.md b/docs/notes/php.md index d6e7ab9da..ac1ccde88 100644 --- a/docs/notes/php.md +++ b/docs/notes/php.md @@ -746,6 +746,67 @@ general form is worth keeping**: a header-only helper that calls into a file the port does not compile is a link error here and silence on gcc. +### CLOSED: every multiplication over 2**32 was a float + +`exit()` fixed, the smoke test runs to completion and **the exit +status is usable again** -- the shell printed `2`, which is the count. +The two that remained were both + + is_int(-9223372036854775807 - 1) -> false + (-9223372036854775807 - 1) === PHP_INT_MIN -> false + +while `PHP_INT_MIN` itself was a clean int and `PHP_INT_MIN + +PHP_INT_MAX` was -1. So the constant was fine and something in the +expression produced a float. + +***THE SUBTRACTION WAS INNOCENT AND THE COMPILER SAID SO.*** The host +kencc compiled php's real `Zend/zend_operators.c`, and +`fast_long_sub_function` is `SUBQ`, then `ANDQ` with the sign mask in +a register, then `CMPQ` -- 64 bits throughout, and tracing it by hand +with 0x8000000000000001 and 1 takes the integer arm. *Reading the +generated code of the real function beat three rounds of reasoning +about which `#elif` arm the config selects.* + +**PHP compiles unary minus as `(-1) * x`**, so the float came from +the MULTIPLY. `zend_multiply.h:132`'s fallback -- the arm taken when +there are no `__builtin_*_overflow`, no `__GNUC__` asm and no Win32, +which is exactly this port -- opens + + long __lres = (a) * (b); + +**Every other arm in that file is guarded by +`SIZEOF_LONG == SIZEOF_ZEND_LONG` or `SIZEOF_ZEND_LONG == 4`; this one +assumes it.** `long` is 32 bits here and `zend_long` is 64, so +`__lres` held the product's low half, the delta test against +`__dres` reported an overflow that had not happened, and the answer +came back a double. + +***SO EVERY INTEGER MULTIPLICATION WHOSE PRODUCT DID NOT FIT IN 32 +BITS SILENTLY RETURNED A FLOAT.*** `2000000 * 2000000` was +`float(4.0E+12)`. That is a wrong answer in the commonest arithmetic +there is, it is silent, and it would have reached every php program +ever run here. + +`zend_long __lres` now. The before/after `-S` diff is the whole story +in four lines: + + < IMULQ SI,DI > IMULQ SI,CX + < MOVQL DI,CX (the product truncated to 32 bits) + < CVTSL2SD CX,X1 > CVTSQ2SD CX,X1 + < MOVLQSX CX,SI > MOVQ CX,(DX) + +**And the test is the thing to correct, not just the code.** Its +first version covered `+`, `-`, `max`, `strlen` and `str_repeat` and +**not `*`** -- it caught this only sideways, through unary minus, and +nothing in the failing output said the word multiply. Section 3b asks +directly now, including two cases that a fix which simply never +reports an overflow would fail: `PHP_INT_MAX * 2` and `PHP_INT_MIN * +2` must still be floats. + +*This is the tree's most-cited invariant -- `long` is 32 bits on +amd64 -- arriving in upstream code that is correct on every LP64 +platform, for the second time in two rounds after `sizeof`.* + ### CLOSED: `exit()` was kencc's 32-bit `sizeof` Three one-liners on the VM settled it with no rebuild: diff --git a/sys/lib/tests/php-smoke.php b/sys/lib/tests/php-smoke.php index a253ecfc1..2cc6042a7 100644 --- a/sys/lib/tests/php-smoke.php +++ b/sys/lib/tests/php-smoke.php @@ -137,6 +137,45 @@ function eq($what, $got, $want) { eq("a padding count is never near 2**32", (max(0, 1 + 0 + $widths[1]) - strlen($cells[1])) < 1024, true); +/* ---------------------------------------------------------------- */ +echo "\nSection 3b: MULTIPLICATION, which the first version never asked\n"; +/* + * ***THE FIRST VERSION OF THIS FILE TESTED +, -, max, strlen AND + * str_repeat, AND NOT `*'.*** It still found the bug, but only + * through the back door: PHP compiles unary minus as `(-1) * x', so + * `-9223372036854775807' was a float and the two PHP_INT_MIN cases + * failed while nothing said the word "multiply". + * + * `zend_multiply.h:135' declared the product `long`, which is 64 bits + * on every LP64 platform and **32 on kencc**, so the product was + * truncated, the overflow check fired on a product that had not + * overflowed, and the answer came back a DOUBLE. **Every integer + * multiplication whose result did not fit in 32 bits was a float.** + * That is a silent wrong answer in the commonest arithmetic there is, + * and a test with five operators and not the sixth could only ever + * have caught it sideways. + */ +eq("small product is an int", 6 * 7, 42); +eq("product just under 2**31", 46340 * 46340, 2147395600); +eq("PRODUCT OVER 2**32 STAYS AN INT", is_int(2000000 * 2000000), true); +eq("...and is right", 2000000 * 2000000, 4000000000000); +eq("a product near PHP_INT_MAX stays an int", + is_int(1000000007 * 1000000009), true); +eq("...and is right", 1000000007 * 1000000009, 1000000016000000063); +eq("a negative product over 2**32 stays an int", + is_int(-2000000 * 2000000), true); +eq("...and is right", -2000000 * 2000000, -4000000000000); +eq("unary minus on a big literal stays an int", + is_int(-9223372036854775807), true); +/* + * And the check must still FIRE when there is a real overflow -- a + * fix that simply never reports one would pass everything above. + */ +eq("a REAL overflow still becomes a float", + is_float(PHP_INT_MAX * 2), true); +eq("...and so does a real negative overflow", + is_float(PHP_INT_MIN * 2), true); + /* ---------------------------------------------------------------- */ echo "\nSection 4: str_repeat, which is where it died\n"; eq("str_repeat small", str_repeat("-", 5), "-----"); diff --git a/sys/src/external/php/Zend/zend_multiply.h b/sys/src/external/php/Zend/zend_multiply.h index 200bedd88..e1f6f520c 100644 --- a/sys/src/external/php/Zend/zend_multiply.h +++ b/sys/src/external/php/Zend/zend_multiply.h @@ -131,8 +131,28 @@ #else +/* + * ***`long' HERE MUST BE `zend_long', AND ON LLP64 THEY ARE NOT THE + * SAME TYPE.*** Every other arm above is guarded by + * `SIZEOF_LONG == SIZEOF_ZEND_LONG' or by `SIZEOF_ZEND_LONG == 4'; + * this fallback is the one that assumes it. On kencc `long' is 32 + * bits and `zend_long' is 64 (it is chosen from `__x86_64__' in + * `zend_long.h'), so `__lres' held the product's LOW 32 BITS, the + * delta test against `__dres' duly reported an overflow that had not + * happened, and the result came back a DOUBLE. + * + * **So every integer multiplication whose product did not fit in 32 + * bits silently became a float.** PHP compiles unary minus as + * `(-1) * x', so `-9223372036854775807' was a float too, which is how + * `php-smoke.php' found it: `is_int(-9223372036854775807 - 1)' was + * false while `PHP_INT_MIN' itself was a clean int. + * + * *This is the tree's most-cited invariant* -- `long' is 32 bits on + * amd64 and `size_t`/`zend_long` are not -- reaching upstream code + * that is correct on every LP64 platform. `docs/notes/php.md'. + */ #define ZEND_SIGNED_MULTIPLY_LONG(a, b, lval, dval, usedval) do { \ - long __lres = (a) * (b); \ + zend_long __lres = (a) * (b); \ long double __dres = (long double)(a) * (long double)(b); \ long double __delta = (long double) __lres - __dres; \ if ( ((usedval) = (( __dres + __delta ) != __dres))) { \