classification
Title: HAVE_SNPRINTF and MSVC std::snprintf support
Type: compile error Stage:
Components: Windows Versions: Python 3.8, Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: matrixise, palotasb-conti, paul.moore, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2019-02-18 10:18 by palotasb-conti, last changed 2019-03-24 06:12 by matrixise.

Pull Requests
URL Status Linked Edit
PR 11913 closed matrixise, 2019-02-18 11:04
Messages (12)
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.
History
Date User Action Args
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 ->
2019-02-18 11:04:27matrixisesetkeywords: + patch
stage: patch review
pull_requests: + pull_request11939
2019-02-18 10:18:26palotasb-conticreate