Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
aixtools opened this issue Jan 20, 2020 · 14 comments
Closed
Labels
3.9 only security fixes tests Tests in the Lib/test dir

Comments

@aixtools
Copy link
Contributor

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
    Copy link
    Contributor Author

    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.

    @aixtools aixtools added 3.9 only security fixes labels Jan 20, 2020
    @vstinner
    Copy link
    Member

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

    Please elaborate. What is the error?

    @aixtools
    Copy link
    Contributor Author

    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
    Copy link
    Member

    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

    @vstinner vstinner added the tests Tests in the Lib/test dir label Jan 20, 2020
    @vstinner vstinner changed the title AIX: math.nextafter(a, b) breaks AIX bot AIX: self.assertEqualSign(math.nextafter(-0.0, +0.0), +0.0) test fails on AIX Jan 20, 2020
    @vstinner vstinner added the tests Tests in the Lib/test dir label Jan 20, 2020
    @vstinner vstinner changed the title AIX: math.nextafter(a, b) breaks AIX bot AIX: self.assertEqualSign(math.nextafter(-0.0, +0.0), +0.0) test fails on AIX Jan 20, 2020
    @aixtools
    Copy link
    Contributor Author

    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
    Copy link
    Member

    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
    Copy link
    Contributor Author

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    I don't have an issue reference to hand

    See bpo-18513.

    @vstinner
    Copy link
    Member

    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
    Copy link
    Member

    My change should fix nextafter test failure on AIX.

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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes tests Tests in the Lib/test dir
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants