Issue39396
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2020-01-20 12:20 by Michael.Felt, last changed 2022-04-11 14:59 by admin. This issue is now closed.
Pull Requests | |||
---|---|---|---|
URL | Status | Linked | Edit |
PR 18094 | merged | vstinner, 2020-01-21 08:01 |
Messages (14) | |||
---|---|---|---|
msg360312 - (view) | Author: Michael Felt (Michael.Felt) * | Date: 2020-01-20 12:20 | |
As issue39288 (that introduces this breakage) is closed, opening a new issue. Back from away - and only starting my investigation - and that will probably be slow. Have not done anything with IEEE754 in over 30 years. |
|||
msg360313 - (view) | Author: STINNER Victor (vstinner) * | Date: 2020-01-20 12:23 | |
> AIX: math.nextafter(a, b) breaks AIX bot Please elaborate. What is the error? |
|||
msg360314 - (view) | Author: Michael Felt (Michael.Felt) * | Date: 2020-01-20 12:46 | |
As I said, was investigating. a) is a bug in most AIX system libraries. b) there is a fix, but not one I can install - as my bots and build systems run on AIX 6.1 and AIX 7.1 c) the fix is APAR IV95512 which includes fixes in the following filesets: IV95512 shipped nextafter(+0.0, -0.0) returns +0.0 instead of -0.0. bos.rte.shell 7.2.2.0 bos.adt.prof 7.2.2.0 bos.adt.libm 7.2.2.0 So, nothing for me "to fix" -- see https://www-01.ibm.com/support/docview.wss?uid=isg1IV95512 FYI: The document URL (above) was last modified on 30 October 2017 |
|||
msg360315 - (view) | Author: STINNER Victor (vstinner) * | Date: 2020-01-20 12:57 | |
Ah, I saw an error on POWER6 AIX 3.x ("POWER6 (aka ppc64-be) using (GCC) 4.7.4"): https://buildbot.python.org/all/#/builders/119/builds/175 The following test fails: self.assertEqualSign(math.nextafter(-0.0, +0.0), +0.0) Well, C code specific to AIX can be added to nextafter() to handle this corner case: diff --git a/Modules/mathmodule.c b/Modules/mathmodule.c index 81d871786f..82ffb4c3d1 100644 --- a/Modules/mathmodule.c +++ b/Modules/mathmodule.c @@ -3287,7 +3287,16 @@ static PyObject * math_nextafter_impl(PyObject *module, double x, double y) /*[clinic end generated code: output=750c8266c1c540ce input=02b2d50cd1d9f9b6]*/ { - double f = nextafter(x, y); + double f; +#if defined(_AIX) + if (x == y) { + f = y; + } + else +#endif + { + f = nextafter(x, y); + } return PyFloat_FromDouble(f); } Another option is to not make the assumption on the libc nextafter() and handle x==y the same way on all platforms. Error: ====================================================================== FAIL: test_nextafter (test.test_math.IsCloseTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/buildbot/buildarea/3.x.aixtools-aix-power6/build/Lib/test/test_math.py", line 2040, in test_nextafter self.assertEqualSign(math.nextafter(-0.0, +0.0), +0.0) File "/home/buildbot/buildarea/3.x.aixtools-aix-power6/build/Lib/test/test_math.py", line 2018, in assertEqualSign self.assertEqual(math.copysign(1.0, x), math.copysign(1.0, y)) AssertionError: -1.0 != 1.0 Same error in test_cmath: ====================================================================== FAIL: test_nextafter (test.test_cmath.IsCloseTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/buildbot/buildarea/3.x.aixtools-aix-power6/build/Lib/test/test_math.py", line 2040, in test_nextafter self.assertEqualSign(math.nextafter(-0.0, +0.0), +0.0) File "/home/buildbot/buildarea/3.x.aixtools-aix-power6/build/Lib/test/test_math.py", line 2018, in assertEqualSign self.assertEqual(math.copysign(1.0, x), math.copysign(1.0, y)) AssertionError: -1.0 != 1.0 |
|||
msg360317 - (view) | Author: Michael Felt (Michael.Felt) * | Date: 2020-01-20 13:08 | |
A hard call to make, imho. Thinking aloud... Currently, for AIX 6.1 and AIX 7.1 your proposal for the code change would be great, but less so for AIX 7.2. However! Since libm (on AIX) is a static library - the behavior depends on the support libm has on the build system. I'll have to puzzle a bit with the configure process and figure out how to best determine the behavior of the build system. On a properly patched AIX 7.2 system the standard code would apply, on others the "adjusted behavior". So, would you go for modified based on what version of libm is installed, better how this function is behaving? |
|||
msg360318 - (view) | Author: STINNER Victor (vstinner) * | Date: 2020-01-20 13:22 | |
Checking how Python is linked or checking the libm version sounds overkill to me. "if (x == y)" test is really cheap: I don't expect nextafter() to be used in performance critical code path anyway. That's why I even consider to add this code path on all platforms, not only AIX. Mark: what's your call on that one? Make the code conditional on AIX? In practice, it seems like only (old libm) AIX nextafter() requires to be "patched". |
|||
msg360319 - (view) | Author: Michael Felt (Michael.Felt) * | Date: 2020-01-20 14:04 | |
FYI: On AIX 5.3, after your proposal I get: ====================================================================== FAIL: test_specific_values (test.test_cmath.CMathTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/data/prj/python/git/python3-3.9/Lib/test/test_cmath.py", line 418, in test_specific_values self.rAssertAlmostEqual(expected.imag, actual.imag, File "/data/prj/python/git/python3-3.9/Lib/test/test_cmath.py", line 130, in rAssertAlmostEqual self.fail(msg or 'zero has wrong sign: expected {!r}, ' AssertionError: exp0001: exp(complex(0.0, -0.0)) Expected: complex(1.0, -0.0) Received: complex(1.0, 0.0) Received value insufficiently close to expected value. ---------------------------------------------------------------------- So, test_nextafter works with the patch, but test_specific_values remains 'active' |
|||
msg360328 - (view) | Author: Mark Dickinson (mark.dickinson) * | Date: 2020-01-20 19:21 | |
> Mark: what's your call on that one? I don't know. It's a hard problem in general: where do we draw the line between simply wrapping the platform libm, bugs and all, on one hand and trying to abstract away platform differences and make guarantees about corner-case behaviour on the other. For this particular case, I'd be fine with adding a special case for `x == y` in the code (suitably commented to explain why the special case is there, and under what conditions it can be removed). But I fear it's the thin end of the wedge: as Michael's last message shows, to get the test suite to pass on his particular flavour of AIX, we'd need to add a special case for cmath.exp, too, and there may be more failures once that's fixed. Actually, the cmath.exp failure looks a little odd to my eyes: it would be a bit surprising to have cmath.exp fail if all of test_math passed. I suspect a dodgy compiler optimisation. But that's another issue. |
|||
msg360329 - (view) | Author: Mark Dickinson (mark.dickinson) * | Date: 2020-01-20 19:29 | |
BTW, stupid question: why is `test_nextafter` in `test.test_math.IsCloseTests` and `test.test_cmath.IsCloseTests`? The first seems inappropriate; the second even more so. |
|||
msg360362 - (view) | Author: STINNER Victor (vstinner) * | Date: 2020-01-21 08:11 | |
> For this particular case, I'd be fine with adding a special case for `x == y` in the code (suitably commented to explain why the special case is there, and under what conditions it can be removed). I proposed PR 18094. Because of your parenthesis, I chose to use "#if defined(_AIX)" with an an explicit comment on AIX version. For example, we may drop this workaround once we would like to drop support for AIX 7.1. > Actually, the cmath.exp failure looks a little odd to my eyes: it would be a bit surprising to have cmath.exp fail if all of test_math passed. I suspect a dodgy compiler optimisation. But that's another issue. Michael: would you mind to open a separated issue for "FAIL: test_specific_values (test.test_cmath.CMathTests)"? Is it on a buildbot? Please try to get the machine code of cmath_exp_impl() function. Example: objdump -d build/lib.linux-x86_64-3.9-pydebug/cmath.cpython-39d-x86_64-linux-gnu.so or try gdb: --- $ gdb ./python GNU gdb (GDB) Fedora 8.3.50.20190824-26.fc31 (gdb) run >>> import cmath <press CTRL+C> Program received signal SIGINT, Interrupt. (gdb) set pagination off (gdb) disassemble cmath_exp_impl --- |
|||
msg360374 - (view) | Author: Mark Dickinson (mark.dickinson) * | Date: 2020-01-21 09:46 | |
So on cmath.exp, we've seen something like this before, on macOS. What was happening was that the C source contained matching `sin` and `cos` calls, and a compiler optimization replaced that pair of calls with a single call to C's cexp. And then cexp didn't handle the zero signs correctly. I don't have an issue reference to hand; all this is from (highly fallible) memory. |
|||
msg360375 - (view) | Author: Mark Dickinson (mark.dickinson) * | Date: 2020-01-21 09:48 | |
> I don't have an issue reference to hand See #18513. |
|||
msg360382 - (view) | Author: STINNER Victor (vstinner) * | Date: 2020-01-21 10:14 | |
New changeset 85ead4fc62829cb7ef2eb0af1a2933282f58c629 by Victor Stinner in branch 'master': bpo-39396: Fix math.nextafter(-0.0, +0.0) on AIX 7.1 (GH-18094) https://github.com/python/cpython/commit/85ead4fc62829cb7ef2eb0af1a2933282f58c629 |
|||
msg360383 - (view) | Author: STINNER Victor (vstinner) * | Date: 2020-01-21 10:18 | |
My change should fix nextafter test failure on AIX. Again, please open a separated issue for "FAIL: test_specific_values (test.test_cmath.CMathTests)". |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:59:25 | admin | set | github: 83577 |
2020-01-21 10:18:14 | vstinner | set | status: open -> closed resolution: fixed messages: + msg360383 stage: patch review -> resolved |
2020-01-21 10:14:17 | vstinner | set | messages: + msg360382 |
2020-01-21 09:48:59 | mark.dickinson | set | messages: + msg360375 |
2020-01-21 09:46:24 | mark.dickinson | set | messages: + msg360374 |
2020-01-21 08:11:22 | vstinner | set | messages: + msg360362 |
2020-01-21 08:01:55 | vstinner | set | keywords:
+ patch stage: patch review pull_requests: + pull_request17484 |
2020-01-20 19:29:10 | mark.dickinson | set | messages: + msg360329 |
2020-01-20 19:21:14 | mark.dickinson | set | messages: + msg360328 |
2020-01-20 14:04:41 | Michael.Felt | set | messages: + msg360319 |
2020-01-20 13:22:47 | vstinner | set | messages: + msg360318 |
2020-01-20 13:08:42 | Michael.Felt | set | messages: + msg360317 |
2020-01-20 12:57:02 | vstinner | set | title: AIX: math.nextafter(a, b) breaks AIX bot -> AIX: self.assertEqualSign(math.nextafter(-0.0, +0.0), +0.0) test fails on AIX nosy: + mark.dickinson messages: + msg360315 components: + Tests |
2020-01-20 12:46:03 | Michael.Felt | set | messages: + msg360314 |
2020-01-20 12:23:12 | vstinner | set | nosy:
+ vstinner messages: + msg360313 |
2020-01-20 12:20:42 | Michael.Felt | create |