fix(optimizer): round() must not lower a RoundingMode enum to an int - #30
fix(optimizer): round() must not lower a RoundingMode enum to an int#30Giandonn wants to merge 2 commits into
Conversation
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.
a37e168 to
44c0f06
Compare
matyhtf
left a comment
There was a problem hiding this comment.
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.
|
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, With the mode routed to the dynamic path it raises what PHP raises: So On unpacking. Rejected before anything reads the syntactic argument count, and before the On #29 and the phpx dependency. Changed to The new Test decisions pinned. Verified locally on PHP 8.5.4 ZTS with embed: PHPT green, |
Refs #29
php::fn::round()declares the mode as anInt, which models only the legacyPHP_ROUND_*constants. Since PHP 8.4 the parameter is aRoundingModeenum,and
genRoundsent it throughconvertIntExprregardless. The object-to-intconversion yields 1,
PHP_ROUND_HALF_UP, so banker's rounding silently becamehalf 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, soan integer outside 1-8 reaches
php_round_helperand takes the process down.On this machine
round(2.5, 0, 99)compiled with the first revision is a plainsegmentation fault; on the dynamic path it raises the expected
ValueError: round(): Argument #3 ($mode) must be a valid rounding mode. Astatic int type says nothing about the runtime value, so an int variable
reaches the same place, and three argument
round()is uncommon enough thatthe conservative route is the right trade.
Unpacked and named arguments are rejected as well. Both carry a single
Node\Argwhatever their runtime arity turns out to be, sogenRound()wasreading the unpacked array itself as the number being rounded.
Verified on a binary
PHP 8.5.4 ZTS with embed, GCC 15, Linux x64:
round(2.5, 0, RoundingMode::HalfEven)round(2.5, 0, 99)ValueError, as PHP2, same as PHPValueError, as PHPTests
phpunit/src/RoundModeTest.phppins the lowering decisions in the generatedC++: 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.phptnow covers the runtime behaviour thatdoes 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.7currently resolves to v2.6.8 (4532c4df),which predates it, so
RoundingMode::HalfEvenstill does not materialise andthe call ends in
TypeError: round(): Argument #3 ($mode) must be of type RoundingMode|int, int|float given. I have leftFixes #29off this PR forthat reason. Once the dependency is bumped past
8d2ca8e9I am happy to sendthe enum PHPT as a follow-up, or to add it here if you would rather land both
together.