Issue 4575: Py_IS_INFINITY defect causes test_cmath failure on x86 (original) (raw)

Created on 2008-12-07 12:42 by mark.dickinson, last changed 2022-04-11 14:56 by admin. 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
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
2022-04-11 14:56:42 admin set github: 48825
2009-02-09 17🔞53 mark.dickinson set status: open -> closedmessages: +
2009-02-09 14:19:16 mark.dickinson set resolution: fixedmessages: + versions: - Python 2.7
2009-01-04 23:14:36 mark.dickinson set messages: +
2009-01-04 23:01:21 mark.dickinson set messages: +
2009-01-04 22:57:09 mark.dickinson set nosy: + tim.petersmessages: +
2009-01-04 22:27:00 mark.dickinson set messages: +
2008-12-23 12:54:24 mark.dickinson set files: + force_to_memory5.patchmessages: +
2008-12-15 17:09:12 mark.dickinson set messages: +
2008-12-15 16:55:39 skip.montanaro set messages: +
2008-12-14 22:03:10 mark.dickinson set messages: +
2008-12-14 21:47:45 rpetrov set files: + force-inf.cmessages: +
2008-12-14 21:30:52 rpetrov set messages: +
2008-12-14 21:26:38 rpetrov set messages: +
2008-12-14 21:22:35 rpetrov set messages: +
2008-12-14 21:08:25 skip.montanaro set messages: +
2008-12-14 20:41:29 rpetrov set nosy: + rpetrovmessages: +
2008-12-14 18:05:35 mark.dickinson set files: + force_to_memory2.patchmessages: +
2008-12-07 12:42:43 mark.dickinson create