From afa4dba091c9cd7159bb5d15f61395745f74ccf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 15 Sep 2023 17:22:06 +0200 Subject: [PATCH 1/6] Reimplement `php_round_helper()` using `modf()` This change makes the implementation much easier to understand, by explicitly handling the various cases. It fixes rounding for `0.49999999999999994`, because no loss of precision happens by adding / subtracing `0.5` before turning the result into an integral float. Instead the fractional parts are explicitly compared. see GH-12143 (this fixes one of the reported cases) Closes GH-12159 which was an alternative attempt to fix the rounding issue for `0.49999999999999994` --- ext/standard/math.c | 49 +++++++++++++------- ext/standard/tests/math/round_gh12143_1.phpt | 27 +++++++++++ ext/standard/tests/math/round_gh12143_2.phpt | 27 +++++++++++ ext/standard/tests/math/round_gh12143_3.phpt | 27 +++++++++++ ext/standard/tests/math/round_gh12143_4.phpt | 27 +++++++++++ 5 files changed, 140 insertions(+), 17 deletions(-) create mode 100644 ext/standard/tests/math/round_gh12143_1.phpt create mode 100644 ext/standard/tests/math/round_gh12143_2.phpt create mode 100644 ext/standard/tests/math/round_gh12143_3.phpt create mode 100644 ext/standard/tests/math/round_gh12143_4.phpt diff --git a/ext/standard/math.c b/ext/standard/math.c index 74ffa565031a3..da000fe4bb901 100644 --- a/ext/standard/math.c +++ b/ext/standard/math.c @@ -93,27 +93,42 @@ static inline double php_intpow10(int power) { /* {{{ php_round_helper Actually performs the rounding of a value to integer in a certain mode */ static inline double php_round_helper(double value, int mode) { - double tmp_value; + double integral, fractional; - if (value >= 0.0) { - tmp_value = floor(value + 0.5); - if ((mode == PHP_ROUND_HALF_DOWN && value == (-0.5 + tmp_value)) || - (mode == PHP_ROUND_HALF_EVEN && value == (0.5 + 2 * floor(tmp_value/2.0))) || - (mode == PHP_ROUND_HALF_ODD && value == (0.5 + 2 * floor(tmp_value/2.0) - 1.0))) - { - tmp_value = tmp_value - 1.0; - } - } else { - tmp_value = ceil(value - 0.5); - if ((mode == PHP_ROUND_HALF_DOWN && value == (0.5 + tmp_value)) || - (mode == PHP_ROUND_HALF_EVEN && value == (-0.5 + 2 * ceil(tmp_value/2.0))) || - (mode == PHP_ROUND_HALF_ODD && value == (-0.5 + 2 * ceil(tmp_value/2.0) + 1.0))) - { - tmp_value = tmp_value + 1.0; + fractional = fabs(modf(value, &integral)); + + switch (mode) { + case PHP_ROUND_HALF_UP: + if (fractional >= 0.5) { + return integral + copysign(1.0, integral); + } + + return integral; + + case PHP_ROUND_HALF_DOWN: + if (fractional > 0.5) { + return integral + copysign(1.0, integral); + } + + return integral; + + case PHP_ROUND_HALF_EVEN: + case PHP_ROUND_HALF_ODD: { + if (fractional == 0.5) { + bool even = !fmod(integral, 2.0); + + if ((mode == PHP_ROUND_HALF_EVEN && !even) || (mode == PHP_ROUND_HALF_ODD && even)) { + return integral + copysign(1.0, integral); + } + } else if (fractional > 0.5) { + return integral + copysign(1.0, integral); + } + + return integral; } } - return tmp_value; + ZEND_ASSERT(0 && "Unknown rounding mode"); } /* }}} */ diff --git a/ext/standard/tests/math/round_gh12143_1.phpt b/ext/standard/tests/math/round_gh12143_1.phpt new file mode 100644 index 0000000000000..390852dba5f26 --- /dev/null +++ b/ext/standard/tests/math/round_gh12143_1.phpt @@ -0,0 +1,27 @@ +--TEST-- +GH-12143: Test rounding of 0.49999999999999994. +--FILE-- + %+.17g\n", $mode, $number, round($number, 0, constant($mode))); + } +} +?> +--EXPECT-- +PHP_ROUND_HALF_UP : +0.49999999999999994 -> +0 +PHP_ROUND_HALF_DOWN : +0.49999999999999994 -> +0 +PHP_ROUND_HALF_EVEN : +0.49999999999999994 -> +0 +PHP_ROUND_HALF_ODD : +0.49999999999999994 -> +0 +PHP_ROUND_HALF_UP : -0.49999999999999994 -> -0 +PHP_ROUND_HALF_DOWN : -0.49999999999999994 -> -0 +PHP_ROUND_HALF_EVEN : -0.49999999999999994 -> -0 +PHP_ROUND_HALF_ODD : -0.49999999999999994 -> -0 diff --git a/ext/standard/tests/math/round_gh12143_2.phpt b/ext/standard/tests/math/round_gh12143_2.phpt new file mode 100644 index 0000000000000..dc5ac74259880 --- /dev/null +++ b/ext/standard/tests/math/round_gh12143_2.phpt @@ -0,0 +1,27 @@ +--TEST-- +GH-12143: Test rounding of 0.50000000000000011. +--FILE-- + %+.17g\n", $mode, $number, round($number, 0, constant($mode))); + } +} +?> +--EXPECT-- +PHP_ROUND_HALF_UP : +0.50000000000000011 -> +1 +PHP_ROUND_HALF_DOWN : +0.50000000000000011 -> +1 +PHP_ROUND_HALF_EVEN : +0.50000000000000011 -> +1 +PHP_ROUND_HALF_ODD : +0.50000000000000011 -> +1 +PHP_ROUND_HALF_UP : -0.50000000000000011 -> -1 +PHP_ROUND_HALF_DOWN : -0.50000000000000011 -> -1 +PHP_ROUND_HALF_EVEN : -0.50000000000000011 -> -1 +PHP_ROUND_HALF_ODD : -0.50000000000000011 -> -1 diff --git a/ext/standard/tests/math/round_gh12143_3.phpt b/ext/standard/tests/math/round_gh12143_3.phpt new file mode 100644 index 0000000000000..ee462fdebaef7 --- /dev/null +++ b/ext/standard/tests/math/round_gh12143_3.phpt @@ -0,0 +1,27 @@ +--TEST-- +GH-12143: Test rounding of 0.5. +--FILE-- + %+.17g\n", $mode, $number, round($number, 0, constant($mode))); + } +} +?> +--EXPECT-- +PHP_ROUND_HALF_UP : +0.5 -> +1 +PHP_ROUND_HALF_DOWN : +0.5 -> +0 +PHP_ROUND_HALF_EVEN : +0.5 -> +0 +PHP_ROUND_HALF_ODD : +0.5 -> +1 +PHP_ROUND_HALF_UP : -0.5 -> -1 +PHP_ROUND_HALF_DOWN : -0.5 -> -0 +PHP_ROUND_HALF_EVEN : -0.5 -> -0 +PHP_ROUND_HALF_ODD : -0.5 -> -1 diff --git a/ext/standard/tests/math/round_gh12143_4.phpt b/ext/standard/tests/math/round_gh12143_4.phpt new file mode 100644 index 0000000000000..d0f3166f1dc50 --- /dev/null +++ b/ext/standard/tests/math/round_gh12143_4.phpt @@ -0,0 +1,27 @@ +--TEST-- +GH-12143: Test rounding of 1.5. +--FILE-- + %+.17g\n", $mode, $number, round($number, 0, constant($mode))); + } +} +?> +--EXPECT-- +PHP_ROUND_HALF_UP : +1.5 -> +2 +PHP_ROUND_HALF_DOWN : +1.5 -> +1 +PHP_ROUND_HALF_EVEN : +1.5 -> +2 +PHP_ROUND_HALF_ODD : +1.5 -> +1 +PHP_ROUND_HALF_UP : -1.5 -> -2 +PHP_ROUND_HALF_DOWN : -1.5 -> -1 +PHP_ROUND_HALF_EVEN : -1.5 -> -2 +PHP_ROUND_HALF_ODD : -1.5 -> -1 From a995a2fcb4146bdbf6263aa7e117ea5a2217c55d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Sun, 17 Sep 2023 14:30:25 +0200 Subject: [PATCH 2/6] Use `ZEND_UNREACHABLE()` in `php_round_helper()` --- ext/standard/math.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/standard/math.c b/ext/standard/math.c index da000fe4bb901..a468e80f4f07a 100644 --- a/ext/standard/math.c +++ b/ext/standard/math.c @@ -128,7 +128,7 @@ static inline double php_round_helper(double value, int mode) { } } - ZEND_ASSERT(0 && "Unknown rounding mode"); + ZEND_UNREACHABLE(); } /* }}} */ From 945f5f9cbc944a19e0f6812de06cba9835cb5d43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Sun, 17 Sep 2023 14:30:58 +0200 Subject: [PATCH 3/6] Reorder branches for PHP_ROUND_HALF_EVEN / PHP_ROUND_HALF_ODD This is for visual consistency with the other modes. --- ext/standard/math.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/ext/standard/math.c b/ext/standard/math.c index a468e80f4f07a..0c6c396ca32f7 100644 --- a/ext/standard/math.c +++ b/ext/standard/math.c @@ -113,19 +113,20 @@ static inline double php_round_helper(double value, int mode) { return integral; case PHP_ROUND_HALF_EVEN: - case PHP_ROUND_HALF_ODD: { + case PHP_ROUND_HALF_ODD: + if (fractional > 0.5) { + return integral + copysign(1.0, integral); + } + if (fractional == 0.5) { bool even = !fmod(integral, 2.0); if ((mode == PHP_ROUND_HALF_EVEN && !even) || (mode == PHP_ROUND_HALF_ODD && even)) { return integral + copysign(1.0, integral); } - } else if (fractional > 0.5) { - return integral + copysign(1.0, integral); } return integral; - } } ZEND_UNREACHABLE(); From e5716cb2bcdccc9cbf664720699cdee43a4b0caa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Sun, 17 Sep 2023 14:50:57 +0200 Subject: [PATCH 4/6] Separate the HALF_EVEN and HALF_ODD modes into dedicated cases This makes the code even clearer to understand and also improves the assembly, allowing the compiler to use an actual jump table for the switch cases. --- ext/standard/math.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/ext/standard/math.c b/ext/standard/math.c index 0c6c396ca32f7..0f897a69c6339 100644 --- a/ext/standard/math.c +++ b/ext/standard/math.c @@ -113,6 +113,19 @@ static inline double php_round_helper(double value, int mode) { return integral; case PHP_ROUND_HALF_EVEN: + if (fractional > 0.5) { + return integral + copysign(1.0, integral); + } + + if (fractional == 0.5) { + bool even = !fmod(integral, 2.0); + + if (!even) { + return integral + copysign(1.0, integral); + } + } + + return integral; case PHP_ROUND_HALF_ODD: if (fractional > 0.5) { return integral + copysign(1.0, integral); @@ -121,7 +134,7 @@ static inline double php_round_helper(double value, int mode) { if (fractional == 0.5) { bool even = !fmod(integral, 2.0); - if ((mode == PHP_ROUND_HALF_EVEN && !even) || (mode == PHP_ROUND_HALF_ODD && even)) { + if (even) { return integral + copysign(1.0, integral); } } From f3cf18057cf7d6ecade235487c2c0598617fa445 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Tue, 19 Sep 2023 17:15:17 +0200 Subject: [PATCH 5/6] Add some explanatory comments to php_round_helper() --- ext/standard/math.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/ext/standard/math.c b/ext/standard/math.c index 0f897a69c6339..05f2252e304e8 100644 --- a/ext/standard/math.c +++ b/ext/standard/math.c @@ -95,11 +95,25 @@ static inline double php_intpow10(int power) { static inline double php_round_helper(double value, int mode) { double integral, fractional; + /* Split the input value into the integral and fractional part. + * + * Both parts will have the same sign as the input value. We take + * the absolute value of the fractional part (which will not result + * in branches in the assembly) to make the following cases simpler. + */ fractional = fabs(modf(value, &integral)); switch (mode) { case PHP_ROUND_HALF_UP: if (fractional >= 0.5) { + /* We must increase the magnitude of the integral part + * (rounding up / towards infinity). copysign(1.0, integral) + * will either result in 1.0 or -1.0 depending on the sign + * of the input, thus increasing the magnitude, but without + * generating branches in the assembly. + * + * This pattern is equally used for all the other modes. + */ return integral + copysign(1.0, integral); } @@ -120,6 +134,9 @@ static inline double php_round_helper(double value, int mode) { if (fractional == 0.5) { bool even = !fmod(integral, 2.0); + /* If the integral part is not even we can make it even + * by adding one in the direction of the existing sign. + */ if (!even) { return integral + copysign(1.0, integral); } From ff62f613eeac944e9ef34d0fffe1bb95ce229519 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Tue, 19 Sep 2023 18:03:58 +0200 Subject: [PATCH 6/6] [ci skip] NEWS / UPGRADING --- NEWS | 2 ++ UPGRADING | 9 +++++++++ 2 files changed, 11 insertions(+) diff --git a/NEWS b/NEWS index 1fd5c2e374d55..ce47d11ef0ab6 100644 --- a/NEWS +++ b/NEWS @@ -10,5 +10,7 @@ DOM: Standard: . Implement GH-12188 (Indication for the int size in phpinfo()). (timwolla) + . Partly fix GH-12143 (Incorrect round() result for 0.49999999999999994). + (timwolla) <<< NOTE: Insert NEWS from last stable release here prior to actual release! >>> diff --git a/UPGRADING b/UPGRADING index 73f50ea5d87d6..1dfb256097d2c 100644 --- a/UPGRADING +++ b/UPGRADING @@ -50,6 +50,15 @@ PHP 8.4 UPGRADE NOTES 5. Changed Functions ======================================== +- Standard: + . The internal implementation for rounding to integers has been rewritten + to be easier to verify for correctness and to be easier to maintain. + Some rounding bugs have been fixed as a result of the rewrite. For + example previously rounding 0.49999999999999994 to the nearest integer + would have resulted in 1.0 instead of the correct result 0.0. Additional + inputs might also be affected and result in different outputs compared to + earlier PHP versions. + ======================================== 6. New Functions ========================================