classification
Title: Py_IS_INFINITY defect causes test_cmath failure on x86
Type: behavior Stage:
Components: Extension Modules Versions: Python 3.0, Python 3.1, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: christian.heimes, mark.dickinson, rpetrov, skip.montanaro, tim.peters
Priority: high Keywords: patch

Created on 2008-12-07 12:42 by mark.dickinson, last changed 2009-02-09 17:18 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
force_to_memory.patch mark.dickinson, 2008-12-07 12:42
force_to_memory2.patch mark.dickinson, 2008-12-14 18:05
force-inf.c rpetrov, 2008-12-14 21:47
force_to_memory5.patch mark.dickinson, 2008-12-23 12:54
Messages (18)
msg77223 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-12-07 12:42
In issue 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?
msg77811 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-12-14 18:05
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.
msg77822 - (view) Author: Roumen Petrov (rpetrov) * Date: 2008-12-14 20:41
may be proposed patch break platforms that need specific "export" decoration
msg77826 - (view) Author: Skip Montanaro (skip.montanaro) * (Python triager) Date: 2008-12-14 21:08
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
msg77830 - (view) Author: Roumen Petrov (rpetrov) * Date: 2008-12-14 21:22
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.
msg77832 - (view) Author: Roumen Petrov (rpetrov) * Date: 2008-12-14 21:26
As Skip Montanaro report that work on ... may be macro from pymath.h has
to be changed to force conversion to double.
msg77834 - (view) Author: Roumen Petrov (rpetrov) * Date: 2008-12-14 21:30
about "export" decoration it is find in then patch PyAPI_FUNC(double) .
found it after second review
msg77837 - (view) Author: Roumen Petrov (rpetrov) * Date: 2008-12-14 21:47
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
msg77841 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-12-14 22:03
> 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.
msg77876 - (view) Author: Skip Montanaro (skip.montanaro) * (Python triager) Date: 2008-12-15 16:55
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
msg77878 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-12-15 17:09
Thanks, Skip.  Looks like this problem is 'solved in principle'.  Now I 
have to figure out a non-hackish solution.
msg78231 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-12-23 12:54
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.
msg79088 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-01-04 22:27
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.
msg79091 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-01-04 22:57
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.
msg79092 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-01-04 23:01
s/false positives/false negatives/
msg79093 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-01-04 23:14
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...
msg81458 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-02-09 14:19
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.
msg81466 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-02-09 17:18
Merged to py3k in r69465.
History
Date User Action Args
2009-02-09 17:18:53mark.dickinsonsetstatus: open -> closed
messages: + msg81466
2009-02-09 14:19:16mark.dickinsonsetresolution: fixed
messages: + msg81458
versions: - Python 2.7
2009-01-04 23:14:36mark.dickinsonsetmessages: + msg79093
2009-01-04 23:01:21mark.dickinsonsetmessages: + msg79092
2009-01-04 22:57:09mark.dickinsonsetnosy: + tim.peters
messages: + msg79091
2009-01-04 22:27:00mark.dickinsonsetmessages: + msg79088
2008-12-23 12:54:24mark.dickinsonsetfiles: + force_to_memory5.patch
messages: + msg78231
2008-12-15 17:09:12mark.dickinsonsetmessages: + msg77878
2008-12-15 16:55:39skip.montanarosetmessages: + msg77876
2008-12-14 22:03:10mark.dickinsonsetmessages: + msg77841
2008-12-14 21:47:45rpetrovsetfiles: + force-inf.c
messages: + msg77837
2008-12-14 21:30:52rpetrovsetmessages: + msg77834
2008-12-14 21:26:38rpetrovsetmessages: + msg77832
2008-12-14 21:22:35rpetrovsetmessages: + msg77830
2008-12-14 21:08:25skip.montanarosetmessages: + msg77826
2008-12-14 20:41:29rpetrovsetnosy: + rpetrov
messages: + msg77822
2008-12-14 18:05:35mark.dickinsonsetfiles: + force_to_memory2.patch
messages: + msg77811
2008-12-07 12:42:43mark.dickinsoncreate