Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HAVE_SNPRINTF and MSVC std::snprintf support #80201

Closed
palotasb-conti mannequin opened this issue Feb 18, 2019 · 21 comments
Closed

HAVE_SNPRINTF and MSVC std::snprintf support #80201

palotasb-conti mannequin opened this issue Feb 18, 2019 · 21 comments
Labels
3.9 3.10 build OS-windows

Comments

@palotasb-conti
Copy link
Mannequin

@palotasb-conti palotasb-conti mannequin commented Feb 18, 2019

BPO 36020
Nosy @pfmoore, @vstinner, @tjguk, @skrah, @zware, @zooba, @matrixise, @miss-islington
PRs
  • #11913
  • #17980
  • #20889
  • #20897
  • #20899
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2020-06-15.22:58:16.111>
    created_at = <Date 2019-02-18.10:18:26.141>
    labels = ['3.10', 'build', '3.9', 'OS-windows']
    title = 'HAVE_SNPRINTF and MSVC std::snprintf support'
    updated_at = <Date 2020-06-15.22:58:16.110>
    user = 'https://bugs.python.org/palotasb-conti'

    bugs.python.org fields:

    activity = <Date 2020-06-15.22:58:16.110>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-06-15.22:58:16.111>
    closer = 'vstinner'
    components = ['Windows']
    creation = <Date 2019-02-18.10:18:26.141>
    creator = 'palotasb-conti'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 36020
    keywords = ['patch']
    message_count = 21.0
    messages = ['335803', '335808', '335810', '335811', '335817', '335823', '335824', '335825', '335978', '336080', '336140', '338722', '371558', '371559', '371572', '371585', '371586', '371592', '371595', '371601', '371602']
    nosy_count = 9.0
    nosy_names = ['paul.moore', 'vstinner', 'tim.golden', 'skrah', 'zach.ware', 'steve.dower', 'matrixise', 'miss-islington', 'palotasb-conti']
    pr_nums = ['11913', '17980', '20889', '20897', '20899']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'compile error'
    url = 'https://bugs.python.org/issue36020'
    versions = ['Python 3.9', 'Python 3.10']

    @palotasb-conti
    Copy link
    Mannequin Author

    @palotasb-conti palotasb-conti mannequin commented Feb 18, 2019

    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:

    cpython/Include/pyerrors.h

    Lines 326 to 330 in ac28147

    #if defined(MS_WIN32) && !defined(HAVE_SNPRINTF)
    # define HAVE_SNPRINTF
    # define snprintf _snprintf
    # define vsnprintf _vsnprintf
    #endif

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

    @palotasb-conti palotasb-conti mannequin added 3.7 3.8 OS-windows build labels Feb 18, 2019
    @matrixise
    Copy link
    Member

    @matrixise matrixise commented Feb 18, 2019

    @palotasb-conti

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

    Could tell me if this PR fixes your issue?

    Thank you

    @matrixise
    Copy link
    Member

    @matrixise matrixise commented Feb 18, 2019

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

    @matrixise
    Copy link
    Member

    @matrixise matrixise commented Feb 18, 2019

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

    @matrixise
    Copy link
    Member

    @matrixise matrixise commented Feb 18, 2019

    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

    @palotasb-conti
    Copy link
    Mannequin Author

    @palotasb-conti palotasb-conti mannequin commented Feb 18, 2019

    @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

    @palotasb-conti
    Copy link
    Mannequin Author

    @palotasb-conti palotasb-conti mannequin commented Feb 18, 2019

    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]
    }

    @palotasb-conti
    Copy link
    Mannequin Author

    @palotasb-conti palotasb-conti mannequin commented Feb 18, 2019

    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

    @zooba
    Copy link
    Member

    @zooba zooba commented Feb 19, 2019

    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.

    @matrixise
    Copy link
    Member

    @matrixise matrixise commented Feb 20, 2019

    Hi @Steve

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

    @zooba
    Copy link
    Member

    @zooba zooba commented Feb 20, 2019

    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.

    @matrixise
    Copy link
    Member

    @matrixise matrixise commented Mar 24, 2019

    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.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jun 15, 2020

    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

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jun 15, 2020

    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:
    jupyter-xeus/xeus-python#283

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jun 15, 2020

    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

    @skrah
    Copy link
    Mannequin

    @skrah skrah mannequin commented Jun 15, 2020

    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.

    @skrah
    Copy link
    Mannequin

    @skrah skrah mannequin commented Jun 15, 2020

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

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jun 15, 2020

    New changeset e822e37 by Victor Stinner in branch 'master':
    bpo-36020: Remove snprintf macro in pyerrors.h (GH-20889)
    e822e37

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Jun 15, 2020

    New changeset b498c7f by Miss Islington (bot) in branch '3.9':
    bpo-36020: Remove snprintf macro in pyerrors.h (GH-20889)
    b498c7f

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jun 15, 2020

    New changeset 7ab92d5 by Victor Stinner in branch 'master':
    bpo-36020: Require vsnprintf() to build Python (GH-20899)
    7ab92d5

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jun 15, 2020

    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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 3.10 build OS-windows
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants