Skip to content

fix(optimizer): round() must not lower a RoundingMode enum to an int - #30

Open
Giandonn wants to merge 2 commits into
swoole:masterfrom
Giandonn:fix/round-mode-enum
Open

fix(optimizer): round() must not lower a RoundingMode enum to an int#30
Giandonn wants to merge 2 commits into
swoole:masterfrom
Giandonn:fix/round-mode-enum

Conversation

@Giandonn

@Giandonn Giandonn commented Aug 29, 2026

Copy link
Copy Markdown

Refs #29

php::fn::round() declares the mode as an Int, which models only the legacy
PHP_ROUND_* constants. Since PHP 8.4 the parameter is a RoundingMode enum,
and genRound sent it through convertIntExpr regardless. The object-to-int
conversion yields 1, PHP_ROUND_HALF_UP, so banker's rounding silently became
half away from zero.

The change

Every call with an explicit third argument now falls through to the dynamic
Zend path. The one and two argument forms are untouched and keep the native
wrapper.

The first revision of this PR only rerouted a mode that was not statically an
int. As @matyhtf pointed out in review, that is not sufficient: the Native
wrapper calls _php_math_round() directly and never runs Zend's validation, so
an integer outside 1-8 reaches php_round_helper and takes the process down.
On this machine round(2.5, 0, 99) compiled with the first revision is a plain
segmentation fault; on the dynamic path it raises the expected
ValueError: round(): Argument #3 ($mode) must be a valid rounding mode. A
static int type says nothing about the runtime value, so an int variable
reaches the same place, and three argument round() is uncommon enough that
the conservative route is the right trade.

Unpacked and named arguments are rejected as well. Both carry a single
Node\Arg whatever their runtime arity turns out to be, so genRound() was
reading the unpacked array itself as the number being rounded.

Verified on a binary

PHP 8.5.4 ZTS with embed, GCC 15, Linux x64:

phpx genRound round(2.5, 0, RoundingMode::HalfEven) round(2.5, 0, 99)
master master stack smashing abort segmentation fault
master first revision clean TypeError, no memory corruption segmentation fault
master this revision clean TypeError, no memory corruption ValueError, as PHP
swoole/phpx#98 this revision 2, same as PHP ValueError, as PHP

Tests

phpunit/src/RoundModeTest.php pins the lowering decisions in the generated
C++: an enum mode, a legacy integer mode and both unpack forms must all stay
off php::fn::round(, while the one and two argument calls must keep it.

tests/compiler/stdlib/round-mode.phpt now covers the runtime behaviour that
does not depend on phpx: valid legacy modes, out-of-range literal and variable
modes, and full and partial unpacking.

The enum case is still not in the runtime coverage. swoole/phpx#98 is merged,
but the pinned swoole/phpx ~2.6.7 currently resolves to v2.6.8 (4532c4df),
which predates it, so RoundingMode::HalfEven still does not materialise and
the call ends in TypeError: round(): Argument #3 ($mode) must be of type RoundingMode|int, int|float given. I have left Fixes #29 off this PR for
that reason. Once the dependency is bumped past 8d2ca8e9 I am happy to send
the enum PHPT as a follow-up, or to add it here if you would rather land both
together.

Since PHP 8.4 the third parameter of round() is a RoundingMode enum, but
php::fn::round() models the mode as an Int, which only covers the legacy
PHP_ROUND_* constants. genRound sent the argument through convertIntExpr
regardless, so the enum went through an object-to-int conversion that
yields 1 - PHP_ROUND_HALF_UP:

    round(2.5, 0, RoundingMode::HalfEven);
    // compiled: Warning: Object of class RoundingMode could not be
    //           converted to int
    // compiled: 3
    // PHP:      2

Banker's rounding silently became half away from zero. Code that spells
out HalfEven is usually money code, where that is the exact difference it
was avoiding.

A mode that is not statically an int now falls through to the dynamic
path, which passes the enum to the runtime function unchanged. The legacy
integer constants keep the native call - PHP_ROUND_HALF_DOWN still lowers
to a plain 2L - and the one and two argument forms are untouched.

Only the compile-time lowering is covered by a test here. A runtime PHPT
cannot pass yet: phpx resolves a class constant on an internal class by
reading the raw zval out of the constants table, so RoundingMode::HalfEven
does not materialise at all. That is reported separately; once it ships,
the runtime case can be added to type_conv-style coverage.
@Giandonn
Giandonn force-pushed the fix/round-mode-enum branch from a37e168 to 44c0f06 Compare August 30, 2026 03:15

@matyhtf matyhtf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for separating the TypePHP lowering bug from the PHPX class-constant materialization bug. Routing an enum mode away from the integer-only Native wrapper is the correct direction.

There are still two unsafe paths that need to be addressed before merge.

First, checking only Type::INT is not sufficient. PHP accepts integer rounding modes 1 through 8 and raises ValueError for any other integer:

round(2.5, 0, 99);

The PHPX Native wrapper calls _php_math_round() directly and bypasses Zend's mode validation. I compiled and ran this case locally; it terminates in PHP's php_round_helper with:

Assertion '0' failed

An int variable can reach the same path because its static type says nothing about whether its runtime value is a valid mode. Consequently, this PR removes the enum-triggered corruption but still leaves malformed integer modes able to abort the process.

The safest and simplest fix is to send every call with an explicit third argument to the dynamic Zend path:

if (count($e->args) >= 3) {
    return false;
}

If the Native optimization must be retained, it should only accept a compile-time-proven integer value in the valid 1-8 range; a statically typed int is not enough. Given how uncommon three-argument round() is, I strongly prefer the conservative dynamic path.

Second, the custom handler still does not reject argument unpacking. For example:

$args = [2.5, 0, RoundingMode::HalfEven];
round(...$args);

This has one unpacked Node\Arg in the AST, so genRound() treats the array itself as the first argument and calls the Native wrapper with it. Full and partial unpack forms must return false before interpreting the syntactic argument count.

Please add runtime coverage for invalid integer modes and code-generation/runtime coverage for full and partial unpack calls.

Finally, swoole/phpx#98 is still open. Without that fix, this PR changes the enum case from memory corruption to a clean TypeError, but it does not yet produce PHP's correct result. Please avoid closing #29 via Fixes #29 until the PHPX fix is integrated and an end-to-end PHPT can pass, or coordinate the dependency update as part of completing this PR.

The two new PHPUnit tests pass locally, but there are currently no GitHub checks reported for the PR.

Checking only Type::INT was not enough. php::fn::round() calls
_php_math_round() directly and never runs Zend's validation of the mode,
so an integer outside 1-8 reaches php_round_helper and terminates the
process rather than raising ValueError:

    round(2.5, 0, 99);   // segmentation fault

A static int type does not prove the runtime value is a valid mode, so
an int variable reaches the same path. Since three-argument round() is
uncommon, take the conservative option and route every call with an
explicit mode to the dynamic Zend path, which validates the argument and
accepts both a RoundingMode enum and a legacy PHP_ROUND_* constant.

Reject unpacked and named arguments as well: they carry a single
Node\Arg whatever their runtime arity is, so genRound() was reading the
unpacked array as the number being rounded.

Add tests/compiler/stdlib/round-mode.phpt covering valid legacy modes,
out-of-range literal and variable modes, and full and partial unpacking.
The enum case still cannot produce PHP's result until the swoole/phpx
class-constant fix is part of the pinned dependency, so it stays out of
the runtime coverage for now.
@Giandonn

Copy link
Copy Markdown
Author

Thank you — both points were right, and the first one is more serious than I had it. I have taken the conservative option you preferred.

On the invalid integer mode. I reproduced your finding. Compiled with the previous revision of this PR, round(2.5, 0, 99) does not just assert — on this build it is a plain segmentation fault:

=== PR-AS-SUBMITTED OUTPUT ===
Segmentation fault

Termsig=11

With the mode routed to the dynamic path it raises what PHP raises:

caught=round(): Argument #3 ($mode) must be a valid rounding mode (RoundingMode::*)

So genRound() now returns false for every call with an explicit third argument, exactly as you suggested. I did not try to salvage the Native path for a proven 1-8 literal: as you say, three-argument round() is uncommon, and a statically typed int proves nothing about the runtime value — the PHPT covers an int variable holding 99 for that reason.

On unpacking. Rejected before anything reads the syntactic argument count, and before the Type::DECIMAL branch, which had the same exposure. I included named arguments in the same guard since they share the single-Node\Arg shape.

On #29 and the phpx dependency. Changed to Refs #29 and updated the PR description. swoole/phpx#98 is merged, but the pinned swoole/phpx ~2.6.7 resolves to v2.6.8 (4532c4df), which predates it, so the enum case here still ends in:

Fatal error: Uncaught TypeError: round(): Argument #3 ($mode) must be of type RoundingMode|int, int|float given

The new tests/compiler/stdlib/round-mode.phpt therefore covers only what is independent of phpx: valid legacy modes, out-of-range literal and variable modes, and full and partial unpacking. It passes today. Once the dependency is bumped past 8d2ca8e9 the enum assertion is a two-line addition — happy to send it as a follow-up, or to add it here if you would rather land the whole thing at once.

Test decisions pinned. RoundModeTest now asserts that the enum mode, the legacy integer mode and both unpack forms all stay off php::fn::round(, and that round(2.5) and round(2.567, 2) still use it. I inverted the earlier testLegacyIntegerModeKeepsTheNativeCall expectation, since that behaviour is precisely what this revision changes.

Verified locally on PHP 8.5.4 ZTS with embed: PHPT green, RoundModeTest green, PHPStan clean on the touched file, full PHPUnit run matching master.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants