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

Py_IS_INFINITY defect causes test_cmath failure on x86 #48825

Closed
mdickinson opened this issue Dec 7, 2008 · 18 comments
Closed

Py_IS_INFINITY defect causes test_cmath failure on x86 #48825

mdickinson opened this issue Dec 7, 2008 · 18 comments
Assignees
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@mdickinson
Copy link
Member

BPO 4575
Nosy @tim-one, @smontanaro, @mdickinson, @tiran
Files
  • force_to_memory.patch
  • force_to_memory2.patch
  • force-inf.c
  • force_to_memory5.patch
  • 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 = 'https://github.com/mdickinson'
    closed_at = <Date 2009-02-09.17:18:53.603>
    created_at = <Date 2008-12-07.12:42:43.696>
    labels = ['extension-modules', 'type-bug']
    title = 'Py_IS_INFINITY defect causes test_cmath failure on x86'
    updated_at = <Date 2009-02-09.17:18:53.601>
    user = 'https://github.com/mdickinson'

    bugs.python.org fields:

    activity = <Date 2009-02-09.17:18:53.601>
    actor = 'mark.dickinson'
    assignee = 'mark.dickinson'
    closed = True
    closed_date = <Date 2009-02-09.17:18:53.603>
    closer = 'mark.dickinson'
    components = ['Extension Modules']
    creation = <Date 2008-12-07.12:42:43.696>
    creator = 'mark.dickinson'
    dependencies = []
    files = ['12261', '12355', '12358', '12432']
    hgrepos = []
    issue_num = 4575
    keywords = ['patch']
    message_count = 18.0
    messages = ['77223', '77811', '77822', '77826', '77830', '77832', '77834', '77837', '77841', '77876', '77878', '78231', '79088', '79091', '79092', '79093', '81458', '81466']
    nosy_count = 5.0
    nosy_names = ['tim.peters', 'skip.montanaro', 'mark.dickinson', 'christian.heimes', 'rpetrov']
    pr_nums = []
    priority = 'high'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue4575'
    versions = ['Python 2.6', 'Python 3.0', 'Python 3.1']

    @mdickinson
    Copy link
    Member Author

    In bpo-4506, Skip reported that test_cmath fails on Solaris 10/x86 for
    3.0. If my guesses are correct, it probably fails on all x86 systems that
    (a) use the x87 coprocessor for floating-point (as opposed to using SSE2,
    for example), and (b) don't have isinf available.

    Problem: Py_IS_INFINITY is applied to a floating-point value sitting in an
    80-bit x87 register; that value is not infinity, but after moving it back
    to memory (and hence rounding from 80-bit extended precision to 64-bit
    double precision with its smaller exponent range) it becomes infinity.

    Solution: Add a macro to pymath.h that forces rounding from extended
    precision to double precision; apply this macro *before* calling
    Py_IS_INFINITY. See attached patch for an example.

    Problem: After applying the attached patch to the py3k branch, the cmath
    module fails to build. On OS X 10.5, I get:

    running build_ext
    building 'cmath' extension
    gcc -bundle -undefined dynamic_lookup build/temp.macosx-10.3-i386-
    3.1/Users/dickinsm/python_source/py3k/Modules/cmathmodule.o -
    L/usr/local/lib -o build/lib.macosx-10.3-i386-3.1/cmath.so
    *** WARNING: renaming "cmath" since importing it failed:
    dlopen(build/lib.macosx-10.3-i386-3.1/cmath.so, 2): Symbol not found:
    _Py_force_to_memory
    Referenced from: /Users/dickinsm/python_source/py3k/build/lib.macosx-
    10.3-i386-3.1/cmath.so
    Expected in: dynamic lookup

    Solution: ???

    Christian, as the architect of pymath.h, do you have any ideas what I'm
    doing wrong? Or comments on the patch in general? What do I need to do
    to make Py_force_to_memory visible to extension modules?

    @mdickinson mdickinson self-assigned this Dec 7, 2008
    @mdickinson mdickinson added extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error labels Dec 7, 2008
    @mdickinson
    Copy link
    Member Author

    Here's a patch (force_to_memory2.patch) that I'm hoping fixes the cmath
    test failures on Solaris 10/x86.

    Skip, could you give it a try?

    The patch isn't final: I need to look for more places where
    Py_FORCE_DOUBLE should be applied. But I'll wait to find out whether this
    really does fix the problem, first; I'm still just guessing about the
    cause.

    By the way, it looks like the problem with the original patch, on OS X,
    was that nothing in pymath.c is used in the Python *core*, so the symbols
    from pymath.o aren't compiled into the python.exe executable, and they're
    not available when loading modules. Rather than working out how to fix
    this, I just moved the definitions into Objects/floatobject.c instead.

    @rpetrov
    Copy link
    Mannequin

    rpetrov mannequin commented Dec 14, 2008

    may be proposed patch break platforms that need specific "export" decoration

    @smontanaro
    Copy link
    Contributor

    Mark> Skip, could you give it a try?

    Works for me on Solaris 10/x86. Based on Roumen's comment I am preparing to
    try it on Mac OS X/x86 and Solaris 10/sparc.

    Skip

    @rpetrov
    Copy link
    Mannequin

    rpetrov mannequin commented Dec 14, 2008

    I'm not sure that patch has to deal with "force to memory".
    FYI: when I port python trunk to mingw platform I encounter some
    inconsistency for isinf.

    My note about issue follow:

    • configure.in:
      ...
      dnl FIXME: For mingw "isinf" is a macro defined in <math.h> and can't be
      dnl detected as function. Also PC/pyconfig.h define HAVE_ISINF but it is
      dnl useless since header define Py_IS_INFINITY as (!_finite(X) &&
      !_isnan(X))
      dnl Also in pymath.h is commented too how PC/pyconfig.h define _isinf.
      dnl NOTE: For mingw to keep compatibility with native build we define
      dnl Py_IS_INFINITY in pyport.h.
      ...
    - pyport.h
    #ifdef __MINGW32__
    ...
    /*NOTE: mingw has isinf as macro defined in math.h.
      Since PC/pyconfig.h define Py_IS_INFINITY(X) that cover HAVE_ISINF
      here for Py_IS_INFINITY we define same as for native build.
      This makes HAVE_ISINF needless.
      Also see comments in configure.in and pymath.h. */
    #define Py_IS_INFINITY(X) (!_finite(X) && !_isnan(X))
    ....
    
    
    next lest see Py_IS_INFINITY in pymath.h
    ....
    #ifndef Py_IS_INFINITY
    #ifdef HAVE_ISINF
    #define Py_IS_INFINITY(X) isinf(X)
    #else
    #define Py_IS_INFINITY(X) ((X) && (X)*0.5 == (X))
    #endif
    #endif
    ....

    Is the macro Py_IS_INFINITY correct if isinf is not detected by
    configure script ?

    Also I think too that problem is that we has to classify result after
    conversion to double.

    @rpetrov
    Copy link
    Mannequin

    rpetrov mannequin commented Dec 14, 2008

    As Skip Montanaro report that work on ... may be macro from pymath.h has
    to be changed to force conversion to double.

    @rpetrov
    Copy link
    Mannequin

    rpetrov mannequin commented Dec 14, 2008

    about "export" decoration it is find in then patch PyAPI_FUNC(double) .
    found it after second review

    @rpetrov
    Copy link
    Mannequin

    rpetrov mannequin commented Dec 14, 2008

    The macro Py_IS_INFINITY don't work on linux. The test case(force-inf.c)
    is attached. The result don't depend from optimisation flag.

    isinf(x)=1
    Py_IS_INFINITY(x)=0
    Py_IS_INFINITY2(x)=1
    isinf(x)=0
    Py_IS_INFINITY(x)=0
    Py_IS_INFINITY2(x)=0

    @mdickinson
    Copy link
    Member Author

    The macro Py_IS_INFINITY don't work on linux. The test case(force-inf.c)
    is attached. The result don't depend from optimisation flag.

    Thanks, Roumen. I rather suspected that Py_IS_INFINITY was dodgy this
    way.

    On the other hand, this is only a problem when Py_IS_INFINITY is applied
    to a *computed* result; most of the time Py_IS_INFINITY is being directly
    applied to the incoming argument to some function, so should be safe.

    @smontanaro
    Copy link
    Contributor

    Took me awhile to locate a SPARC C compiler on our dwindling set of
    Solaris/SPARC boxes at work, but I eventually found one and got Subversion
    trunk to compile. test_cmath and test_math both pass with the
    force_to_memory2 patch. I don't know if I mentioned it earlier (replying by
    email), but it also works for me on Mac OS X 10.5.5 (Intel Core2 Duo).

    Skip

    @mdickinson
    Copy link
    Member Author

    Thanks, Skip. Looks like this problem is 'solved in principle'. Now I
    have to figure out a non-hackish solution.

    @mdickinson
    Copy link
    Member Author

    Final patch.

    Skip, could you please give this one a try, and then with any luck this
    can be fixed for 3.0.1. (Sorry for creating more work; this should be
    the last time.)

    I've added a configure test that detects x87 FPU usage, via the double
    rounding issue associated with the x87. If x87 is detected then
    _Py_force_double is used via a macro Py_FORCE_DOUBLE; otherwise,
    Py_FORCE_DOUBLE does nothing.

    When you configure on a machine that uses x87, you should see:

    checking for x87-style double rounding... yes

    in the configure output.

    @mdickinson
    Copy link
    Member Author

    Looking at this again, I don't like my solution.

    I think it would be better to fix Py_IS_INFINITY directly, putting all the
    complication into one place; then users of Py_IS_INFINITY don't have to
    spend time worrying about whether they should be calling Py_FORCE_DOUBLE
    or not.

    The fixed Py_IS_INFINITY will likely be slower, but this only matters on
    platforms that don't provide isinf, isnan; it seems that Solaris is the
    only such platform in common use.

    @mdickinson
    Copy link
    Member Author

    Tim, I'm in need of some advice on Py_IS_INFINITY. It's currently
    implemented (on platforms that don't provide isinf) as

    #define Py_IS_INFINITY(X) ((X) && (X)*0.5 == (X))

    I'd like to rewrite it as something like:

    #define Py_IS_INFINITY_D(X) ((X) < -DBL_MAX || (X) > DBL_MAX)
    #define Py_IS_INFINITY_F(X) ((X) < -FLT_MAX || (X) > FLT_MAX)
    #define Py_IS_INFINITY(X) (sizeof(X) == sizeof(double) ? Py_IS_INFINITY_D(X) : Py_IS_INFINITY_F(X))

    Are there any hidden (or obvious) numerical pitfalls with
    this approach?

    The reason for the rewrite is that the current Py_IS_INFINITY
    can give false positives on x86 for values that are
    pretending to be doubles, but are actually coming from an
    80-bit x87 register.

    @mdickinson
    Copy link
    Member Author

    s/false positives/false negatives/

    @mdickinson
    Copy link
    Member Author

    Answering my own question, there *are* pitfalls: (X) > DBL_LONG_MAX will
    evaluate to true for some finite extended precision values that are *just*
    larger than DBL_LONG_MAX, but nevertheless round to DBL_LONG_MAX rather
    than infinity.

    Another not-so-bright idea down the drain...

    @mdickinson
    Copy link
    Member Author

    Fixed (I hope!) in the trunk in r69459. I'll wait for buildbot results
    (just in case) and then merge to 2.6, 3.1 and 3.0.

    The same test_cmath failure can also be seen on OS X 10.5.6/Intel when
    compiling with -fmpmath=387. Annoyingly, the fix above doesn't work here:
    it seems that the OS X isinf is buggy. It doesn't seem worth working
    around this bug though, since there's little sane reason to be compiling
    with -fmpmath=387 in the first place, so it's unlikely that any regular
    Python users will encounter this problem.

    @mdickinson
    Copy link
    Member Author

    Merged to py3k in r69465.

    @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
    extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants