Fix inconsistent min()/max() tie-breaking on signed zero via the frameless 2-arg fast path - #23505
Open
2akouwu wants to merge 1 commit into
Open
Fix inconsistent min()/max() tie-breaking on signed zero via the frameless 2-arg fast path#235052akouwu wants to merge 1 commit into
2akouwu wants to merge 1 commit into
Conversation
…variadic/array min()/max() paths The 2-argument frameless fast path for min() used strict `<` for every comparison, so on a tie (e.g. -0.0 vs 0, which compare equal under IEEE-754 but are distinguishable via var_dump) it returned the second operand. Every other min()/max() code path -- the array form, the variadic form, and the frameless max() fast path -- keeps the first-seen operand on a tie. Switch the frameless min() comparisons from `<` to `<=` so it agrees with the rest of the implementation. Fixes phpGH-20221 Signed-off-by: ulofiai <309826581+ulofiai@users.noreply.github.com>
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.
Root cause
min($a, $b)andmax($a, $b)calls with exactly two positional arguments are compiled by the engine to theZEND_FRAMELESS_FUNCTION(min, 2)/ZEND_FRAMELESS_FUNCTION(max, 2)fast paths inext/standard/array.c(wired up inZend/zend_compile.cfor 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 keepslhs(the first-seen operand). That matchesphp_array_data_minmax()(used formin(array)/max(array)) and the variadicPHP_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 returnedrhs(the second operand) instead oflhs. This is invisible for most values, but-0.0and0compare equal under ordinary IEEE-754 double comparison while still being distinguishable viavar_dump()(float(-0)vsint(0)). That's exactly whatround(-0.01 / 2, 0)produces (-0.0), so: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
-0formatting 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 toMath.min/Math.maxin some other languages). I rejected that: PHP's comparison operators andzend_compare()have no such total-ordering concept for-0.0anywhere else in the engine (arraymin/max, the variadicmin/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 thezend_compare(...) < 0generic fallback inZEND_FRAMELESS_FUNCTION(min, 2)to<=, mirroring the>=already used inZEND_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'sgh<issue>.phptnaming convention (see e.g.gh18480.phpt,gh17977.phptin the same directory). It reproduces the issue's exact scenario (-0.0vs0) through the 2-argument frameless calls in both operand orders for bothmin()andmax(), 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
.phptwas not executed throughrun-tests.php. I traced the expected output by hand against the modified control flow inext/standard/array.cfor 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.