[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[STINNER Victor]

STINNER Victor  added the comment:

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.

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[STINNER Victor]

STINNER Victor  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +20082
pull_request: https://github.com/python/cpython/pull/20899

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[miss-islington]

miss-islington  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[STINNER Victor]

STINNER Victor  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[miss-islington]

Change by miss-islington :


--
nosy: +miss-islington
nosy_count: 8.0 -> 9.0
pull_requests: +20080
pull_request: https://github.com/python/cpython/pull/20897

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[Stefan Krah]

Stefan Krah  added the comment:

I've tested the MSVC _snprintf extremely extensively in _decimal and never had 
a problem.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[Stefan Krah]

Stefan Krah  added the comment:

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.

--
nosy: +skrah

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[STINNER Victor]

STINNER Victor  added the comment:

> 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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[STINNER Victor]

STINNER Victor  added the comment:

In April, the issue was discussed on the capi-sig mailing list:
https://mail.python.org/archives/list/capi-...@python.org/thread/5NXBZWKBMAPJJLNIXASSAYRIAP2OHJ53/

This issue was also mention in:
https://github.com/jupyter-xeus/xeus-python/issues/283

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[STINNER Victor]

STINNER Victor  added the comment:

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 
// Undefine macros to work around https://bugs.python.org/issue36020
#undef snprintf
#undef vsnprintf
---

--
versions: +Python 3.10, Python 3.9 -Python 3.7, Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[STINNER Victor]

Change by STINNER Victor :


--
nosy: +vstinner
nosy_count: 6.0 -> 7.0
pull_requests: +20072
pull_request: https://github.com/python/cpython/pull/20889

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[Roundup Robot]

Change by Roundup Robot :


--
pull_requests: +17386
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/17980

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[Stéphane Wirtel]

Stéphane Wirtel  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[Steve Dower]

Steve Dower  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[Stéphane Wirtel]

Stéphane Wirtel  added the comment:

Hi @steve

I'm not used to this, can you guide me?
Thanks

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[Steve Dower]

Steve Dower  added the comment:

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.

--
versions:  -Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[palotasb-conti]

palotasb-conti  added the 
comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[palotasb-conti]

palotasb-conti  added the 
comment:

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 
#include 
// 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 
// 4. Now we can observe that the following function won't compile
#include 
void test() {
char buf[2];
std::snprintf(buf, 1, "x");
// error C2039: '_snprintf': is not a member of 'std' 
[python-boost-mcve.vcxproj]
}

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[palotasb-conti]

palotasb-conti  added the 
comment:

@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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[Stéphane Wirtel]

Stéphane Wirtel  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[Stéphane Wirtel]

Stéphane Wirtel  added the comment:

For the version of MSVC, I have found this link:
https://en.wikipedia.org/wiki/Microsoft_Visual_C%2B%2B

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[Stéphane Wirtel]

Stéphane Wirtel  added the comment:

@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,

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[Stéphane Wirtel]

Change by Stéphane Wirtel :


--
keywords: +patch
pull_requests: +11939
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[Stéphane Wirtel]

Stéphane Wirtel  added the comment:

@palotasb-conti

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

Could tell me if this PR fixes your issue?

Thank you

--
nosy: +matrixise
stage: patch review -> 

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36020] HAVE_SNPRINTF and MSVC std::snprintf support

[palotasb-conti]

New submission from palotasb-conti 
:

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).

--
components: Windows
messages: 335803
nosy: palotasb-conti, paul.moore, steve.dower, tim.golden, zach.ware
priority: normal
severity: normal
status: open
title: HAVE_SNPRINTF and MSVC std::snprintf support
type: compile error
versions: Python 3.6, Python 3.7, Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com