[GIT-PULLS] [php-src] PR #23505: Fix inconsistent min()/max() tie-breaking on signed zero via the frameless 2-arg fast path
[email protected] (2akouwu)
| Newsgroups | php.git-pulls |
|---|---|
| Message-ID | <[email protected]> |
Pull Request: https://github.com/php/php-src/pull/23505 Author: 2akouwu ## Root cause `min($a, $b)` and `max($a, $b)` calls with exactly two positional arguments are compiled by the engine to the `ZEND_FRAMELESS_FUNCTION(min, 2)` / `ZEND_FRAMELESS_FUNCTION(max, 2)` fast paths in `ext/standard/array.c` (wired up in `Zend/zend_compile.c` for any call site whose argument count matches a registered frameless variant), bypassing the normal argument-array based implementation. `ZEND_FRAMELESS_FUNCTION(max, 2)` already resolves ties with `>=`, so when the two operands compare equal it keeps `lhs` (the first-seen operand). That matches `php_array_data_minmax()` (used for `min(array)`/`max(array)`) and the variadic `PHP_FUNCTION(min)`/`PHP_FUNCTION(max)` implementations, both of which only replace the running result on a *strict* improvement and therefore also keep the first-seen operand on a tie. `ZEND_FRAMELESS_FUNCTION(min, 2)` was the one path that didn't follow this convention: every comparison used strict `<`, so on a tie it returned `rhs` (the second operand) instead of `lhs`. This is invisible for most values, but `-0.0` and `0` compare equal under ordinary IEEE-754 double comparison while still being distinguishable via `var_dump()` (`float(-0)` vs `int(0)`). That's exactly what `round(-0.01 / 2, 0)` produces (`-0.0`), so: ```php var_dump(min(round(-0.01 / 2, 0), 0)); // returned int(0) -- wrong operand picked on tie var_dump(max(round(-0.01 / 2, 0), 0)); // returned float(-0) -- correct, first operand kept ``` min() and max() disagreeing on which operand "wins" a tie, given the exact same two operands in the exact same order, is the actual bug -- not merely a cosmetic `-0` formatting issue. ## Why this fix I considered special-casing signed zero directly (e.g. using `signbit()` so min() always prefers the negative-zero representation and max() always prefers positive zero, regardless of argument order, similar to `Math.min`/`Math.max` in some other languages). I rejected that: PHP's comparison operators and `zend_compare()` have no such total-ordering concept for `-0.0` anywhere else in the engine (array `min`/`max`, the variadic `min`/`max`, `<`, `>`, sort callbacks, etc. all treat `-0.0 == 0.0`), so introducing it only for the 2-arg frameless path would create a *new*, narrower inconsistency instead of fixing the existing one. Instead, the minimal, targeted fix is to make the frameless `min()` tie-break the same way as every other min/max code path already does: keep the first-seen operand on a tie. Concretely, this changes the three `<` comparisons (long/long, double/double, double/long) and the `zend_compare(...) < 0` generic fallback in `ZEND_FRAMELESS_FUNCTION(min, 2)` to `<=`, mirroring the `>=` already used in `ZEND_FRAMELESS_FUNCTION(max, 2)`. No behavior changes for non-tied values; only the tie-break operand selection changes, and only for the frameless 2-arg path. ## Testing Added `ext/standard/tests/array/gh20221.phpt`, following this project's `gh<issue>.phpt` naming convention (see e.g. `gh18480.phpt`, `gh17977.phpt` in the same directory). It reproduces the issue's exact scenario (`-0.0` vs `0`) through the 2-argument frameless calls in both operand orders for both `min()` and `max()`, and cross-checks the same values through the 3-argument variadic path and the array-argument path (`min([...])`/`max([...])`), asserting that the frameless results now agree with those already-correct paths. I could not build the PHP interpreter in this sandboxed source-only bundle (no toolchain/build artifacts are present), so the `.phpt` was not executed through `run-tests.php`. I traced the expected output by hand against the modified control flow in `ext/standard/array.c` for every branch the test exercises (long/long, double/double, double/long, and both operand orders), which is why the test includes the redundant array/variadic cross-checks -- so a reviewer running it in CI has an internally-consistent oracle, not just my hand-derived expectations.