classification
Title: HAVE_SNPRINTF and MSVC std::snprintf support
Type: compile error Stage: resolved
Components: Windows Versions: Python 3.10, Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: matrixise, miss-islington, palotasb-conti, paul.moore, skrah, steve.dower, tim.golden, vstinner, zach.ware
Priority: normal Keywords: patch

Created on 2019-02-18 10:18 by palotasb-conti, last changed 2020-06-15 22:58 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 11913 closed matrixise, 2019-02-18 11:04
PR 17980 closed python-dev, 2020-01-13 10:38
PR 20889 merged vstinner, 2020-06-15 14:54
PR 20897 merged miss-islington, 2020-06-15 20:00
PR 20899 merged vstinner, 2020-06-15 20:22
Messages (21)
msg335803 - (view) Author: palotasb-conti (palotasb-conti) Date: 2019-02-18 10:18
Abstract: pyerrors.h defines snprintf as a macro on MSVC even on versions of MSVC where this workaround causes bugs.

snprintf is defined as _snprintf in pyerrors.h, see: https://github.com/python/cpython/blob/ac28147e78c45a6217d348ce90ca5281d91f676f/Include/pyerrors.h#L326-L330

The conditions for this should exclude _MSC_VER >= 1900 where (std::)snprintf is correctly defined. Since this is not the case, subsequent user code that tries to use std::snprintf will fail with an err (_snprintf is not a member of namespace std).
msg335808 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-02-18 11:04
@palotasb-conti

I don't have MSVC because on I work on Linux.

Could tell me if this PR fixes your issue?

Thank you
msg335810 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-02-18 11:06
@steve.dower, @zach.ware

I don't have a Windows machine, I could install an official VM of Windows and install MSVC but I don't have time, I am working for my customer but I could check my PR later (I will wait for the feedback of AppVeyor).

Thank you for your feedback,
msg335811 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-02-18 11:07
For the version of MSVC, I have found this link:
https://en.wikipedia.org/wiki/Microsoft_Visual_C%2B%2B
msg335817 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-02-18 11:21
Hi,

Do you suggest if this VM is correct for the tests? I will try to check with this one.

https://developer.microsoft.com/en-us/windows/downloads/virtual-machines
msg335823 - (view) Author: palotasb-conti (palotasb-conti) Date: 2019-02-18 12:00
@matrixise:

It works on my machine. ;)

I would also add a check for `defined(_MSC_VER)` before `_MSC_VER < 1900` because the `MS_WIN32` does not seem to imply that the `_MSC_VER` macro is defined [1]. The correct check could be either

    #if defined(MS_WIN32) && !defined(HAVE_SNPRINTF) && defined(_MSC_VER) && _MSC_VER < 1900

or

    #if defined(MS_WIN32) && !defined(HAVE_SNPRINTF) && (!defined(_MSC_VER) || _MSC_VER < 1900)

I don't know whether (MS_WIN32 && !defined(_MSC_VER)) means that there is an `(std::)snprintf` function available or if it needs to be redefined to _snprintf -- or even is such a configuration exists and is supported by Python. If I had to guess though, then the first version should be correct since the macro was already defined for MS_WIN32 regardless of _MSC_VER.

The VMs seem OK to me for testing.

[1] see https://github.com/python/cpython/blob/8a1657b93469580ef345c7c91738587f3d76e87d/PC/pyconfig.h#L68,L84
msg335824 - (view) Author: palotasb-conti (palotasb-conti) Date: 2019-02-18 12:03
I have the following internal, almost-minimal test case for this bug. It also relies on Boost Python, but that could be eliminated to produce a similar test case.

# CMakeLists.txt
cmake_minimum_required(VERSION 3.0)
project(python-boost-mcve)
set(Boost_USE_STATIC_LIBS ON)
find_package(Boost COMPONENTS python36 REQUIRED)
find_package(Python3 COMPONENTS Development REQUIRED)
add_library(python-boost-mcve)
target_link_libraries(python-boost-mcve PUBLIC Python3::Python Boost::python36)
target_sources(python-boost-mcve PUBLIC
    "${CMAKE_CURRENT_SOURCE_DIR}/python-boost-mcve.cpp")

// python-boost-mcve.cpp
// 1. This is a bug with MSVC Python
#if !defined(_MSC_VER) || _MSC_VER < 1900
#  error "The MCVE requires Visual Studio 14.0 or higher"
#endif
// 2. An MSVC system header is required to reproduce the error for some reason,
// perhaps it sets some macro. This needs to be BEFORE <boost/python.hpp>
#include <array>
// 3. Boost Python is required to include the system (MSVC) Python headers that
// define _snprintf as a macro under the circumstances
// #define HAVE_SNPRINTF // Define this macro as a workaround to fix the compile error
#include <boost/python.hpp>
// 4. Now we can observe that the following function won't compile
#include <cstdio>
void test() {
    char buf[2];
    std::snprintf(buf, 1, "x");
    // error C2039: '_snprintf': is not a member of 'std' [python-boost-mcve.vcxproj]
}
msg335825 - (view) Author: palotasb-conti (palotasb-conti) Date: 2019-02-18 12:06
For the record, I am basing the version check code on Boost also: https://www.boost.org/doc/libs/1_65_1/boost/convert/detail/config.hpp
msg335978 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-02-19 16:57
We don't support older versions of MSVC at this point, so the version check seems unnecessary.

If we need to alias these within the CPython sources for non-MSVC compilers, then we should do it in source files. In general, we never want to pollute the user's namespace with aliases like this - when we do, it should get a "_Py_" prefix and ideally be behind one of the Py_BUILD_CORE variables.

So I'd rather see a change to remove those definitions from the header files and put them closer to where they belong. If that's too many places, they can go into an internal header with "_Py_" prefix and update all uses.
msg336080 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-02-20 12:36
Hi @steve

I'm not used to this, can you guide me?
Thanks
msg336140 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-02-20 19:35
Start by just deleting the definitions in pyerrors.h, and see where the build fails. Then either switch those to use PyOS_snprintf (best), or add the #ifndef checks in the source files (not header files) that need them.

Be aware that we shouldn't backport the deletion from the headers to 3.7. Backporting the code changes to use the PyOS_* functions is fine though.
msg338722 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-03-24 06:12
I will continue to work on this issue when I will have a Windows virtual machine or a computer, for the moment I close my PR because it's not the right solution. Sorry for my inactivity about this issue.
msg371558 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-06-15 15:00
I wrote PR 20889 which removes "snprintf" and "vsnprintf" macros from pyerrors.h. I propose to backport the change to 3.9, but leave 3.8 unchanged.

On Python 3.8 and older, the workaround is to manually undefine the macros:
---
#include <Python.h>
// Undefine macros to work around https://bugs.python.org/issue36020
#undef snprintf
#undef vsnprintf
---
msg371559 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-06-15 15:02
In April, the issue was discussed on the capi-sig mailing list:
https://mail.python.org/archives/list/capi-sig@python.org/thread/5NXBZWKBMAPJJLNIXASSAYRIAP2OHJ53/

This issue was also mention in:
https://github.com/jupyter-xeus/xeus-python/issues/283
msg371572 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-06-15 16:38
> On Python 3.8 and older, the workaround is to manually undefine the macros: (...)

pybind11 implemented a different workaround:
---
/* Don't let Python.h #define (v)snprintf as macro because they are implemented
   properly in Visual Studio since 2015. */
#if defined(_MSC_VER) && _MSC_VER >= 1900
#  define HAVE_SNPRINTF 1
#endif
---
https://github.com/pybind/pybind11/pull/2238/files
msg371585 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2020-06-15 19:03
Can't we just use #ifndef __cplusplus instead of changing the function?

I don't think anyone compiles the affected files with a C++ compiler.


There are many areas in Include/* that fail with C++, e.g. isnan() with -std=c++11.
msg371586 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2020-06-15 19:08
I've tested the MSVC _snprintf extremely extensively in _decimal and never had a problem.
msg371592 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-06-15 19:59
New changeset e822e37946f27c09953bb5733acf3b07c2db690f by Victor Stinner in branch 'master':
bpo-36020: Remove snprintf macro in pyerrors.h (GH-20889)
https://github.com/python/cpython/commit/e822e37946f27c09953bb5733acf3b07c2db690f
msg371595 - (view) Author: miss-islington (miss-islington) Date: 2020-06-15 20:20
New changeset b498c7f1b3890e43ea2e7d1570f8403707ea4cc6 by Miss Islington (bot) in branch '3.9':
bpo-36020: Remove snprintf macro in pyerrors.h (GH-20889)
https://github.com/python/cpython/commit/b498c7f1b3890e43ea2e7d1570f8403707ea4cc6
msg371601 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-06-15 22:54
New changeset 7ab92d54b5d4440d84f6c02b4bc5a70103eff915 by Victor Stinner in branch 'master':
bpo-36020: Require vsnprintf() to build Python (GH-20899)
https://github.com/python/cpython/commit/7ab92d54b5d4440d84f6c02b4bc5a70103eff915
msg371602 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-06-15 22:58
I removed the two offending macro defines from Python.h (pyerrors.h) in 3.9 and master branches. I close the issue.

I also simplified PyOS_snprintf() and PyOS_vsnprintf() implementation in the master branch: they no longer call Py_FatalError() on buffer overflow, on platforms which don't provide vsnprintf(). vsnprintf() is now required to build the master branch.
History
Date User Action Args
2020-06-15 22:58:16vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg371602

stage: patch review -> resolved
2020-06-15 22:54:48vstinnersetmessages: + msg371601
2020-06-15 20:22:35vstinnersetpull_requests: + pull_request20082
2020-06-15 20:20:13miss-islingtonsetmessages: + msg371595
2020-06-15 20:00:03miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request20080
2020-06-15 19:59:54vstinnersetmessages: + msg371592
2020-06-15 19:08:11skrahsetmessages: + msg371586
2020-06-15 19:03:22skrahsetnosy: + skrah
messages: + msg371585
2020-06-15 16:38:44vstinnersetmessages: + msg371572
2020-06-15 15:02:27vstinnersetmessages: + msg371559
2020-06-15 15:00:06vstinnersetmessages: + msg371558
versions: + Python 3.9, Python 3.10, - Python 3.7, Python 3.8
2020-06-15 14:54:13vstinnersetnosy: + vstinner
pull_requests: + pull_request20072
2020-01-13 10:38:15python-devsetstage: patch review
pull_requests: + pull_request17386
2019-03-24 06:12:31matrixisesetmessages: + msg338722
2019-02-20 19:35:08steve.dowersetmessages: + msg336140
2019-02-20 12:36:56matrixisesetmessages: + msg336080
2019-02-19 16:57:54steve.dowersetmessages: + msg335978
versions: - Python 3.6
2019-02-18 12:06:20palotasb-contisetmessages: + msg335825
2019-02-18 12:03:41palotasb-contisetmessages: + msg335824
2019-02-18 12:00:10palotasb-contisetmessages: + msg335823
2019-02-18 11:21:59matrixisesetmessages: + msg335817
2019-02-18 11:07:28matrixisesetmessages: + msg335811
2019-02-18 11:06:35matrixisesetmessages: + msg335810
2019-02-18 11:04:46matrixisesetnosy: + matrixise

messages: + msg335808
stage: patch review -> (no value)
2019-02-18 11:04:27matrixisesetkeywords: + patch
stage: patch review
pull_requests: + pull_request11939
2019-02-18 10:18:26palotasb-conticreate