Skip to content

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
php:masterfrom
2akouwu:fix/issue-20221
Open

Fix inconsistent min()/max() tie-breaking on signed zero via the frameless 2-arg fast path#23505
2akouwu wants to merge 1 commit into
php:masterfrom
2akouwu:fix/issue-20221

Conversation

@2akouwu

@2akouwu 2akouwu commented Aug 29, 2026

Copy link
Copy Markdown

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:

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.

…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>
@devnexen

Copy link
Copy Markdown
Member
@devnexen
devnexen requested a review from TimWolla August 29, 2026 20:55
@Sjord

Sjord commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Link to issue: #20221

Ping @TimWolla

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

3 participants