From 065f4bff2b90afa48d6d708877b56ba5160b7057 Mon Sep 17 00:00:00 2001 From: Sergey B Kirpichev Date: Tue, 17 Sep 2024 15:03:34 +0300 Subject: [PATCH 1/7] gh-123836: workaround fmod(x, y) bug on Windows Buildbot failure on Windows 10 with MSC v.1916 64 bit (AMD64): FAIL: testFmod (test.test_math.MathTests.testFmod) ---------------------------------------------------------------------- Traceback (most recent call last): File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\test_math.py", line 605, in testFmod self.ftest('fmod(-10, 1)', math.fmod(-10, 1), -0.0) ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\test_math.py", line 258, in ftest self.fail("{}: {}".format(name, failure)) ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError: fmod(-10, 1): expected -0.0, got 0.0 (zero has wrong sign) Here Windows loose sign of the result; if y is nonzero, the result should have the same sign as x. This amends 28aea5d07d. --- Modules/_math.h | 18 ++++++++++++++++++ Modules/mathmodule.c | 2 +- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/Modules/_math.h b/Modules/_math.h index 2285b64747c0bd..ef2ca433be9e28 100644 --- a/Modules/_math.h +++ b/Modules/_math.h @@ -23,3 +23,21 @@ _Py_log1p(double x) } #define m_log1p _Py_log1p + +static double +_Py_fmod(double x, double y) +{ + /* Some platforms (e.g. Windows 10 with MSC v.1916) loose sign + for zero result. But C99+ says: "if y is nonzero, the result + has the same sign as x". + */ + double r = fmod(x, y); + + if (r == 0.0 && y != 0.0) { + r = copysign(r, x); + } + + return r; +} + +#define m_fmod _Py_fmod diff --git a/Modules/mathmodule.c b/Modules/mathmodule.c index b7eb745177f37f..d52c6da76e293f 100644 --- a/Modules/mathmodule.c +++ b/Modules/mathmodule.c @@ -2347,7 +2347,7 @@ math_fmod_impl(PyObject *module, double x, double y) if (isinf(y) && isfinite(x)) return PyFloat_FromDouble(x); errno = 0; - r = fmod(x, y); + r = m_fmod(x, y); if (isnan(r)) { if (!isnan(x) && !isnan(y)) errno = EDOM; From 49c13a6a2d4ae64241b71a7dee7d2e8b67ae58ad Mon Sep 17 00:00:00 2001 From: Sergey B Kirpichev Date: Tue, 17 Sep 2024 16:41:28 +0300 Subject: [PATCH 2/7] address review: drop m_fmod --- Modules/_math.h | 2 -- Modules/mathmodule.c | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/Modules/_math.h b/Modules/_math.h index ef2ca433be9e28..ed053f3fc8db16 100644 --- a/Modules/_math.h +++ b/Modules/_math.h @@ -39,5 +39,3 @@ _Py_fmod(double x, double y) return r; } - -#define m_fmod _Py_fmod diff --git a/Modules/mathmodule.c b/Modules/mathmodule.c index d52c6da76e293f..79bff518081636 100644 --- a/Modules/mathmodule.c +++ b/Modules/mathmodule.c @@ -2347,7 +2347,7 @@ math_fmod_impl(PyObject *module, double x, double y) if (isinf(y) && isfinite(x)) return PyFloat_FromDouble(x); errno = 0; - r = m_fmod(x, y); + r = _Py_fmod(x, y); if (isnan(r)) { if (!isnan(x) && !isnan(y)) errno = EDOM; From d3a0af603d46ea43700adfe8c334e03a399ab023 Mon Sep 17 00:00:00 2001 From: Sergey B Kirpichev Date: Tue, 17 Sep 2024 17:25:56 +0300 Subject: [PATCH 3/7] XXX filter warnings? --- Tools/build/.warningignore_macos | 2 +- Tools/build/.warningignore_ubuntu | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Tools/build/.warningignore_macos b/Tools/build/.warningignore_macos index 3034638595353a..1ac257009ff0b2 100644 --- a/Tools/build/.warningignore_macos +++ b/Tools/build/.warningignore_macos @@ -94,7 +94,7 @@ Modules/cjkcodecs/multibytecodec.c 2 Modules/clinic/_testclinic.c.h 1 Modules/clinic/arraymodule.c.h 1 Modules/clinic/unicodedata.c.h 10 -Modules/cmathmodule.c 1 +Modules/cmathmodule.c 2 Modules/expat/siphash.h 8 Modules/expat/xmlparse.c 45 Modules/expat/xmltok.c 17 diff --git a/Tools/build/.warningignore_ubuntu b/Tools/build/.warningignore_ubuntu index e98305e81808d6..f6c6a73f947bbb 100644 --- a/Tools/build/.warningignore_ubuntu +++ b/Tools/build/.warningignore_ubuntu @@ -120,6 +120,7 @@ Modules/cjkcodecs/_codecs_kr.c 7 Modules/cjkcodecs/alg_jisx0201.h 2 Modules/cjkcodecs/cjkcodecs.h 1 Modules/cjkcodecs/multibytecodec.c 12 +Modules/cmathmodule.c 1 Modules/expat/pyexpatns.h 3 Modules/expat/siphash.h 1 Modules/expat/xmlparse.c 43 From aedcf74d2c96be16262cb7bb1ece13bab1ff6cd5 Mon Sep 17 00:00:00 2001 From: Sergey B Kirpichev Date: Tue, 17 Sep 2024 17:45:07 +0300 Subject: [PATCH 4/7] Revert "XXX filter warnings?" This reverts commit d3a0af603d46ea43700adfe8c334e03a399ab023. --- Tools/build/.warningignore_macos | 2 +- Tools/build/.warningignore_ubuntu | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/Tools/build/.warningignore_macos b/Tools/build/.warningignore_macos index 1ac257009ff0b2..3034638595353a 100644 --- a/Tools/build/.warningignore_macos +++ b/Tools/build/.warningignore_macos @@ -94,7 +94,7 @@ Modules/cjkcodecs/multibytecodec.c 2 Modules/clinic/_testclinic.c.h 1 Modules/clinic/arraymodule.c.h 1 Modules/clinic/unicodedata.c.h 10 -Modules/cmathmodule.c 2 +Modules/cmathmodule.c 1 Modules/expat/siphash.h 8 Modules/expat/xmlparse.c 45 Modules/expat/xmltok.c 17 diff --git a/Tools/build/.warningignore_ubuntu b/Tools/build/.warningignore_ubuntu index f6c6a73f947bbb..e98305e81808d6 100644 --- a/Tools/build/.warningignore_ubuntu +++ b/Tools/build/.warningignore_ubuntu @@ -120,7 +120,6 @@ Modules/cjkcodecs/_codecs_kr.c 7 Modules/cjkcodecs/alg_jisx0201.h 2 Modules/cjkcodecs/cjkcodecs.h 1 Modules/cjkcodecs/multibytecodec.c 12 -Modules/cmathmodule.c 1 Modules/expat/pyexpatns.h 3 Modules/expat/siphash.h 1 Modules/expat/xmlparse.c 43 From fbecd8b68274c0f9da83e357497f7119857777fd Mon Sep 17 00:00:00 2001 From: Sergey B Kirpichev Date: Tue, 17 Sep 2024 17:46:44 +0300 Subject: [PATCH 5/7] Move workaround to math_fmod_impl --- Modules/_math.h | 16 ---------------- Modules/mathmodule.c | 9 ++++++++- 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/Modules/_math.h b/Modules/_math.h index ed053f3fc8db16..2285b64747c0bd 100644 --- a/Modules/_math.h +++ b/Modules/_math.h @@ -23,19 +23,3 @@ _Py_log1p(double x) } #define m_log1p _Py_log1p - -static double -_Py_fmod(double x, double y) -{ - /* Some platforms (e.g. Windows 10 with MSC v.1916) loose sign - for zero result. But C99+ says: "if y is nonzero, the result - has the same sign as x". - */ - double r = fmod(x, y); - - if (r == 0.0 && y != 0.0) { - r = copysign(r, x); - } - - return r; -} diff --git a/Modules/mathmodule.c b/Modules/mathmodule.c index 79bff518081636..c47909632abacb 100644 --- a/Modules/mathmodule.c +++ b/Modules/mathmodule.c @@ -2347,7 +2347,14 @@ math_fmod_impl(PyObject *module, double x, double y) if (isinf(y) && isfinite(x)) return PyFloat_FromDouble(x); errno = 0; - r = _Py_fmod(x, y); + r = fmod(x, y); + /* Some platforms (e.g. Windows 10 with MSC v.1916) loose sign + for zero result. But C99+ says: "if y is nonzero, the result + has the same sign as x". + */ + if (r == 0.0 && y != 0.0) { + r = copysign(r, x); + } if (isnan(r)) { if (!isnan(x) && !isnan(y)) errno = EDOM; From 5a15cbe3419e5fb41e9e86f4f8ce83900fc141c1 Mon Sep 17 00:00:00 2001 From: Sergey B Kirpichev Date: Tue, 17 Sep 2024 18:52:17 +0300 Subject: [PATCH 6/7] add news --- .../next/Library/2024-09-17-18-06-42.gh-issue-124171.PHCvRJ.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2024-09-17-18-06-42.gh-issue-124171.PHCvRJ.rst diff --git a/Misc/NEWS.d/next/Library/2024-09-17-18-06-42.gh-issue-124171.PHCvRJ.rst b/Misc/NEWS.d/next/Library/2024-09-17-18-06-42.gh-issue-124171.PHCvRJ.rst new file mode 100644 index 00000000000000..ed94b6b10e3411 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-09-17-18-06-42.gh-issue-124171.PHCvRJ.rst @@ -0,0 +1,2 @@ +Add workaround for broken :c:func:`!fmod()` implementations (e.g. on +Windows), that loose zero sign. Patch by Sergey B Kirpichev. From 32aa012dc3299b6b7b8c275b2f010894851d9d26 Mon Sep 17 00:00:00 2001 From: Sergey B Kirpichev Date: Tue, 17 Sep 2024 19:47:30 +0300 Subject: [PATCH 7/7] address review: restrict fix to Windows --- .../Library/2024-09-17-18-06-42.gh-issue-124171.PHCvRJ.rst | 5 +++-- Modules/mathmodule.c | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2024-09-17-18-06-42.gh-issue-124171.PHCvRJ.rst b/Misc/NEWS.d/next/Library/2024-09-17-18-06-42.gh-issue-124171.PHCvRJ.rst index ed94b6b10e3411..c2f0bb14f55251 100644 --- a/Misc/NEWS.d/next/Library/2024-09-17-18-06-42.gh-issue-124171.PHCvRJ.rst +++ b/Misc/NEWS.d/next/Library/2024-09-17-18-06-42.gh-issue-124171.PHCvRJ.rst @@ -1,2 +1,3 @@ -Add workaround for broken :c:func:`!fmod()` implementations (e.g. on -Windows), that loose zero sign. Patch by Sergey B Kirpichev. +Add workaround for broken :c:func:`!fmod()` implementations on Windows, that +loose zero sign (e.g. ``fmod(-10, 1)`` returns ``0.0``). Patch by Sergey B +Kirpichev. diff --git a/Modules/mathmodule.c b/Modules/mathmodule.c index c47909632abacb..baf2dc439b8959 100644 --- a/Modules/mathmodule.c +++ b/Modules/mathmodule.c @@ -2348,13 +2348,15 @@ math_fmod_impl(PyObject *module, double x, double y) return PyFloat_FromDouble(x); errno = 0; r = fmod(x, y); - /* Some platforms (e.g. Windows 10 with MSC v.1916) loose sign +#ifdef _MSC_VER + /* Windows (e.g. Windows 10 with MSC v.1916) loose sign for zero result. But C99+ says: "if y is nonzero, the result has the same sign as x". */ if (r == 0.0 && y != 0.0) { r = copysign(r, x); } +#endif if (isnan(r)) { if (!isnan(x) && !isnan(y)) errno = EDOM;