classification
Title: AIX: self.assertEqualSign(math.nextafter(-0.0, +0.0), +0.0) test fails on AIX
Type: Stage: resolved
Components: Tests Versions: Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Michael.Felt, mark.dickinson, vstinner
Priority: normal Keywords: patch

Created on 2020-01-20 12:20 by Michael.Felt, last changed 2020-01-21 10:18 by vstinner. 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2020-01-21 09:48
> I don't have an issue reference to hand

See #18513.
msg360382 - (view) Author: STINNER Victor (vstinner) * (Python committer) 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) * (Python committer) 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
2020-01-21 10:18:14vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg360383

stage: patch review -> resolved
2020-01-21 10:14:17vstinnersetmessages: + msg360382
2020-01-21 09:48:59mark.dickinsonsetmessages: + msg360375
2020-01-21 09:46:24mark.dickinsonsetmessages: + msg360374
2020-01-21 08:11:22vstinnersetmessages: + msg360362
2020-01-21 08:01:55vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request17484
2020-01-20 19:29:10mark.dickinsonsetmessages: + msg360329
2020-01-20 19:21:14mark.dickinsonsetmessages: + msg360328
2020-01-20 14:04:41Michael.Feltsetmessages: + msg360319
2020-01-20 13:22:47vstinnersetmessages: + msg360318
2020-01-20 13:08:42Michael.Feltsetmessages: + msg360317
2020-01-20 12:57:02vstinnersettitle: 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:03Michael.Feltsetmessages: + msg360314
2020-01-20 12:23:12vstinnersetnosy: + vstinner
messages: + msg360313
2020-01-20 12:20:42Michael.Feltcreate