AIX: self.assertEqualSign(math.nextafter(-0.0, +0.0), +0.0) test fails on AIX

[aixtools]

BPO	39396
Nosy	@mdickinson, @vstinner, @aixtools
PRs	bpo-39396: Fix math.nextafter(-0.0, +0.0) on AIX 7.1 #18094

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-01-21.10:18:14.130>
created_at = <Date 2020-01-20.12:20:42.953>
labels = ['tests', '3.9']
title = 'AIX: self.assertEqualSign(math.nextafter(-0.0, +0.0), +0.0) test fails on AIX'
updated_at = <Date 2020-01-21.10:18:14.124>
user = 'https://github.com/aixtools'

bugs.python.org fields:

activity = <Date 2020-01-21.10:18:14.124>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2020-01-21.10:18:14.130>
closer = 'vstinner'
components = ['Tests']
creation = <Date 2020-01-20.12:20:42.953>
creator = 'Michael.Felt'
dependencies = []
files = []
hgrepos = []
issue_num = 39396
keywords = ['patch']
message_count = 14.0
messages = ['360312', '360313', '360314', '360315', '360317', '360318', '360319', '360328', '360329', '360362', '360374', '360375', '360382', '360383']
nosy_count = 3.0
nosy_names = ['mark.dickinson', 'vstinner', 'Michael.Felt']
pr_nums = ['18094']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue39396'
versions = ['Python 3.9']

[aixtools]

As bpo-39288 (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.

[vstinner]

AIX: math.nextafter(a, b) breaks AIX bot

Please elaborate. What is the error?

[aixtools]

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

[vstinner]

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

[aixtools]

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?

[vstinner]

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".

[aixtools]

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'

[mdickinson]

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.

[mdickinson]

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.

[vstinner]

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
---

[mdickinson]

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.

[mdickinson]

I don't have an issue reference to hand

See bpo-18513.

[vstinner]

New changeset 85ead4f by Victor Stinner in branch 'master':
bpo-39396: Fix math.nextafter(-0.0, +0.0) on AIX 7.1 (GH-18094)
85ead4f

[vstinner]

My change should fix nextafter test failure on AIX.

Again, please open a separated issue for "FAIL: test_specific_values (test.test_cmath.CMathTests)".