homepage

We are using python as an embedded scripting environment in our ERP-product.
Since upgrading to python2.7 we have serious issues with floats:
>>> 28710.0
'2870:.0'
>>> round(28710.0)
2870.0
We are embedding Python in a Delphi-application.
The problem was already discussed in issue9980 and has to do with Delphi setting the FPU precision to 64bit (and relying on this setting) while the standard with Microsoft Tools is 53bits.
The routines _Py_dg_dtoa and _Py_dg_strtod in dtoa.c rely on the FPU precision set to 53bits.
Issue9980 was closed as "won't fix" but I propose to reconsider this decision for the following reasons:
- Delphi is still an important development environment for native win32 applications and has excellent Python embedding support through PythonForDelphi (http://code.google.com/p/python4delphi).
- Ensuring 53bit before calling python code is not practical in an embedded python environment with extensions in delphi (python may also call code that is implemented in delphi).
- The changes needed in pythoncore are minimal. Tests documented in issue9980 found no measurable performance impact.
- FPU precision switching is needed and already (partially) implemented for linx, where 64bit prec is standard.
Fixing this issues is absolutely vital for us, so we needed to compile a custom version of pythoncore.
I appended a .diff file detailling the patch.
Changes are needed in 2 places:
- pyport.h, defining the _PY_SET_53_BIT_PRECISION macros for MS visual c compiler
- floatobject.c, insert precision switching macros in _Py_double_round function.
In pystrtod.c the precision switching macros are already used.
pystrtod.c and floatobject.c are the only places in CPython where the dtoa.c functions are called.
The macros for visual-c are activated by defining HAVE_VC_FUNC_FOR_X87 preprocessor-symbol.
Hoping for inclusion of this patch.
Cheers
Samuel

Samuel, can you regenerate your patch? The contents don't look right---I see changes to pyport.h, but none to floatobject.c (and some other spurious-looking changes to the project file).

I'm a bit disturbed that we're not using the _Py_SET_53BIT_PRECISION_* macros in _Py_double_round; unless I'm missing something, that seems like a major omission.

New changeset 5b8800004955 by Mark Dickinson in branch '3.2':
Issue #13889: Add missing _Py_SET_53BIT_PRECISION_* calls around uses of dtoa.c functions in float round.
http://hg.python.org/cpython/rev/5b8800004955
New changeset 265d35e8fe82 by Mark Dickinson in branch 'default':
Merge 3.2 -> default (issue 13889)
http://hg.python.org/cpython/rev/265d35e8fe82
New changeset eaf553b063a7 by Mark Dickinson in branch '2.7':
Issue #13889: Add missing _Py_SET_53BIT_PRECISION_* calls around uses of dtoa.c functions in float round.
http://hg.python.org/cpython/rev/eaf553b063a7 

> but none to floatobject.c 
Bah, reading comprehension fail. Now that I look at your patch again, the floatobject.c changes are there (and pretty much match what I just committed).
I assume that the changes to pythoncore.vcproj can be disregarded, though?

Hi Marc, the changes to the pythoncore.vcproj Visual-Studio file define the HAVE_VC_FUNC_FOR_X87 symbol. I use this symbol to enable the precision-setting macros in pyport.h. I made this similar to the existing code for gcc (linux).
You can change this but currently this symbol has to be defined somewhere for the macros to have an effect.

Should this bug be marked as "closed+committed"?

> Should this bug be marked as "closed+committed"?
No, the main issue is still under consideration.
There were two issues here:
(1) a clear bug, namely that the Py_SET_53BIT_PRECISION_* macros weren't being used for the dtoa calls when rounding, and
(2) a proposal to alter the way that the FPU settings are treated on Windows; this is still open.
I'll change the type from behaviour, though.

> Hi Marc, the changes to the pythoncore.vcproj Visual-Studio file define > the HAVE_VC_FUNC_FOR_X87 symbol.
Okay, makes sense. I was distracted by the spurious reordering of in the diff for pythoncore.vcproj.
Just to be clear, the intent of the patch is that the FPU state is *always* switched on Windows prior to calling the dtoa.c functions; is that right?
Things to think about:
 - can we avoid *writing* to the x87 / SSE control word if no change is necessary (as is currently done with the gcc code)? We want to avoid unnecessary FPU pipeline flushes.
 - we need to make sure that the patch works on 64-bit. There's a bit of text at:
 http://msdn.microsoft.com/en-us/library/c9676k6h.aspx
 that suggests that in x64 mode, setting the precision is an error.
 - what happens if the x87 and SSE2 control words have different precisions? Does the patch restore both those precisions correctly?
 - in the patch, isn't new387controlword unused?
I'm not sure that this part of the patch can go into the maintenance branches (2.7, 3.2); if this is a new feature (and I think it is, but I'm willing to be persuaded otherwise), it can only target 3.3.

I would definitely classify this as a bug in Python 2.7 as it breaks backwards-compatibility for embedding environments that default to 64bit FPU precision (e.g. Delphi).
Additionally the bug is very hard to detect and leads to wrong values with possibly disastrous effects.
Appended a patch with a new implementation of the Py_SET_53BIT_PRECISION_* macros for win32:
- precision is set only when needed
- setting and restoring only the x87 controlword (using __control87_2
 function).
- macros are not used for WIN64 as there's no x87 there
- there's no need for a custom symbol in the vc project anymore, 
 as I'm using the predefined _WIN32 symbol.

There's an excess space after a line continuation backslash in my last patch, so it doesn't compile (pyport.h, line 567).
Here's an additional patch that removes the space.
Didn't find out how to combine the 2 revisions in one patch-file...
Sorry about that.

General shape of the patch looks good.
I'd suggest using a mask of _MCW_PC | _MCW_RC instead of just _MCW_PC, so that the rounding mode is also set correctly. Probably rarely an issue in practice, but it's the same thing that we're doing on Linux.
If this is going near the maintenance branches (2.7, 3.2), we need to be very careful.
Have you had the opportunity to test the patch (e.g., run the complete Python test suite) both on 32-bit and 64-bit Windows?

I can run the tests on 32bit. Would be nice if somebody else could do this on 64bit (my VS2008 machine is currently on 32bit-Windows).

I can run the tests on win64, but it I think it would be nice to
have a patch with exactly the same logic as the gcc-asm version
(Mark already mentioned _MCW_PC | _MCW_RC).
Another thing: _WIN32 might be defined on MinGW, but I'm not sure.
If that's the case, it would be nice to guard the VS specific
passage with _MSC_VER.

Completed the patch by adding the rounding-control bits to the mask (_MCW_RC) and making sure the macros only get defined for MS-VC compiler (#ifdef _MSC_VER).
Ran the tests (python_d -m test.autotest) on win32. Seems OK. The tests that were skipped or failed don't seem to be connected to this patch:
- test_repr failed on trying to import a very long package and module name
- test_popen failed with SyntaxError: unexpected EOF while parsing.
Here's the summary:
323 tests OK.
2 tests failed:
 test_popen test_repr
2 tests altered the execution environment:
 test_distutils test_site
62 tests skipped:
 test_aepack test_al test_applesingle test_bsddb test_bsddb185
 test_bsddb3 test_bz2 test_cd test_cl test_codecmaps_cn
 test_codecmaps_hk test_codecmaps_jp test_codecmaps_kr
 test_codecmaps_tw test_commands test_crypt test_curses test_dbm
 test_dl test_epoll test_fcntl test_fork1 test_gdb test_gdbm
 test_gl test_grp test_imgfile test_ioctl test_kqueue
 test_largefile test_linuxaudiodev test_macos test_macostools
 test_mhlib test_nis test_openpty test_ossaudiodev test_pipes
 test_poll test_posix test_pty test_pwd test_py3kwarn test_readline
 test_resource test_scriptpackages test_smtpnet test_socketserver
 test_sqlite test_ssl test_sunaudiodev test_tcl test_threadsignals
 test_timeout test_tk test_ttk_guionly test_ttk_textonly
 test_urllib2net test_urllibnet test_wait3 test_wait4
 test_zipfile64
10 skips unexpected on win32:
 test_bsddb test_bz2 test_gdb test_readline test_sqlite test_ssl
 test_tcl test_tk test_ttk_guionly test_ttk_textonly
[43048 refs]

Patch looks fine to me. A couple of minor points (which I'm happy to fix at commit time if you agree, and don't want to create a new patch):
- it looks to me as though the #ifdef _WIN32 isn't necessary any more; it's enough that we're on MSVC and that this isn't a 64-bit build.
- I'd suggest replacing _PC_53 with _PC_53 | _RC_NEAR. I know it's technically redundant, since _RC_NEAR happens to be 0, but it makes the intent of the code clearer.
I'd still want to see all the tests pass on both x64 and x86 Windows before applying this; I may have an opportunity to test this on x64 in the near future.

I've looked at the latest patch: It seems that new_387controlword is
not set if old_387controlword already has the desired precision and
rounding mode.
Attached is a revised patch that uses the same logic as the Linux
version. A couple of remarks:
 - It would be possible to negate (_PC_53|_RC_NEAR) instead of
 enumerating (_MCW_DN|_MCW_EM|_MCW_IC). I found it nice to
 see all possibilities listed.
 - Technically we might need to use #pragma fenv_access (on). I'm not sure
 where though: If it is set in pyport.h, VS complains that Py_MATH_PI / 180.0
 is not constant.
The patch is tested on win32/x64. Additionally, the patch is tested
with setting the rounding mode to _PC_64 in main.c. Then, the
patch is tested with replacing the 'if' bodies by 'abort()'. This
shows that in the regular build (_PC_53 on startup) the bodies of
the if statements are never executed.
Finally, inserting an #error after #if defined(_MSC_VER) && !defined(_WIN64)
on the x64 build shows that !defined(_WIN64) really does its job.

BTW, the MSDN documentation at
http://msdn.microsoft.com/en-us/library/e9b52ceh(v=vs.100).aspx
is a bit confusing. Question 1: when doing __control87_2(new, mask, &old, NULL), does the resulting value in old reflect the *new* FPU state or the old one?
Question 2: in the example near the bottom of that page, there's code like:
 control_word_x87 = __control87_2(_PC_24, MCW_PC,
 &control_word_x87, 0);
This looks very odd: we're assigning to control_word_x87, *and* passing it as an output parameter to the call. Moreover, from the documentation the return value from __control87_2 is always 1 to indicate success, so I'm not sure why it's being assigned to control_word_x87.
Am I the only person who's confused by this?

Mark Dickinson <report@bugs.python.org> wrote:
> http://msdn.microsoft.com/en-us/library/e9b52ceh(v=vs.100).aspx
> Question 1: when doing __control87_2(new, mask, &old, NULL), does the
> resulting value in old reflect the *new* FPU state or the old one?
The new one, but I had to test that manually (see below).
> Question 2: in the example near the bottom of that page, there's code like:
> 
> control_word_x87 = __control87_2(_PC_24, MCW_PC,
> &control_word_x87, 0);
> 
> This looks very odd: we're assigning to control_word_x87, *and* passing it as an output parameter to the call. Moreover, from the documentation the return value from __control87_2 is always 1 to indicate success, so I'm not sure why it's being assigned to control_word_x87.
> 
> Am I the only person who's confused by this?
I was confused as well. I'm positive that it's a cut-and-paste bug in the docs:
Probably they had _control87() in that place before, which *does* return the
new control word. As you say, the example above clobbers the value that was set
via the pointer reference, so the result is always 1. :)

Stefan Krah <report@bugs.python.org> wrote:
> 
> Stefan Krah <stefan-usenet@bytereef.org> added the comment:
> 
> Mark Dickinson <report@bugs.python.org> wrote:
> > http://msdn.microsoft.com/en-us/library/e9b52ceh(v=vs.100).aspx
> > Question 1: when doing __control87_2(new, mask, &old, NULL), does the
> > resulting value in old reflect the *new* FPU state or the old one?
> 
> The new one, but I had to test that manually (see below).
Well of course, unless both new and mask are 0, in which case it's
the existing state.

Thanks for the responses.
As far as I'm concerned, this is good to go into all 3 maintained branches. Having said that, the 2.7 and 3.2 branches are currently at release candidate stage, so I think we should wait until after the releases.

Slightly reworked patch. I plan to apply this shortly.
- Use ~(_MCW_PC | _MCW_RC) rather than (_MCW_DN | ...), since this seems more future proof: there's a possibility that more flags could be added later.
- Put the usual do { ... } while (0) around the _Py_SET_53BIT_PRECISION_END
- Make it clear that we don't care about the return value of the second two __control87_2 patches.
- Minor style fixes (e.g., spaces around a "bitwise and" ampersand to help distinguish it from an "address of" ampersand).
I'm not too worried about the fenv_access pragma: it seems that the main thing it would guard against is compile-time folding and evaluation of expressions, where the compiler isn't taking the possibility of control word changes into account. As far as I can tell, we shouldn't really care about this, since at the time that Python itself is compiled, the control word is likely to be what we want.

New changeset dfc9a98a5fef by Mark Dickinson in branch '3.2':
Issue #13889: On MSVC builds, set FPU control word at runtime for all string <-> float conversions. Patch by Samuel Iseli and Stefan Krah.
http://hg.python.org/cpython/rev/dfc9a98a5fef
New changeset 7eca620feb10 by Mark Dickinson in branch 'default':
Issue #13889: Merge fix from 3.2.
http://hg.python.org/cpython/rev/7eca620feb10 

New changeset bf3b77722c9f by Mark Dickinson in branch '2.7':
Issue #13889: On MSVC builds, set FPU control word at runtime for all string <-> float conversions. Patch by Samuel Iseli and Stefan Krah.
http://hg.python.org/cpython/rev/bf3b77722c9f