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]