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

Extension type from documentation doesn't compile in C++20 mode #99202

Open
arogge opened this issue Nov 7, 2022 · 9 comments
Open

Extension type from documentation doesn't compile in C++20 mode #99202

arogge opened this issue Nov 7, 2022 · 9 comments
Labels
docs Documentation in the Doc dir topic-C-API type-bug An unexpected behavior, bug, or error

Comments

@arogge
Copy link

arogge commented Nov 7, 2022

Bug report

C++20 added support for designated initializers and fails to compile if you mix named and unnamed initializers.

For demonstration I'll use the example from the documentation section "Defining Extension Types: Tutorial" with .tp_doc removed from CustomType so it builds on Python 3.10.

When building with C++ compiler in C++20 mode using
g++ $(python3-config --cflags) -std=c++20 -Wall -Wextra -Wno-missing-field-initializers -c pymod.cc
it fails with the following message:

pymod.cc:11:5: error: either all initializer clauses should be designated or none of them should be
   11 |     .tp_name = "custom.Custom",
      |     ^
pymod.cc:20:5: error: either all initializer clauses should be designated or none of them should be
   20 |     .m_name = "custom",
      |     ^

The problem is the macros that produce non-designated initializers here.

Additional information

The following compiles work flawlessly:
GNU C compiler:
gcc $(python3-config --cflags) -Wall -Wextra -Wno-missing-field-initializers -c pymod.cc

GNU C++ compiler with no C++ version specified:
g++ $(python3-config --cflags) -Wall -Wextra -Wno-missing-field-initializers -c pymod.cc

GNU C++ Compiler in C++17 mode:
g++ $(python3-config --cflags) -std=c++17 -Wall -Wextra -Wno-missing-field-initializers -c pymod.cc

Your environment

  • Fedora 36 container with python-devel, gcc and gcc-c++ installed
  • CPython: 3.10.7
  • GCC: 12.2.1 20220819 (Red Hat 12.2.1-2)

The issue leading up to this was our build failing in C++20 mode on at least RHEL 7/8/9, Fedora 34 to 37, SLES 12, SLES 15, Debian 10/11, Ubuntu 16 to 22.
So I guess building with pretty much every GNU C++ compiler will break if you turn on C++20.

pymod.cc:

#define PY_SSIZE_T_CLEAN
#include <Python.h>

typedef struct {
    PyObject_HEAD
    /* Type-specific fields go here. */
} CustomObject;

static PyTypeObject CustomType = {
    PyVarObject_HEAD_INIT(NULL, 0)
    .tp_name = "custom.Custom",
    .tp_basicsize = sizeof(CustomObject),
    .tp_itemsize = 0,
    .tp_flags = Py_TPFLAGS_DEFAULT,
    .tp_new = PyType_GenericNew,
};

static PyModuleDef custommodule = {
    PyModuleDef_HEAD_INIT,
    .m_name = "custom",
    .m_doc = "Example module that creates an extension type.",
    .m_size = -1,
};

PyMODINIT_FUNC
PyInit_custom(void)
{
    PyObject *m;
    if (PyType_Ready(&CustomType) < 0)
        return NULL;

    m = PyModule_Create(&custommodule);
    if (m == NULL)
        return NULL;

    Py_INCREF(&CustomType);
    if (PyModule_AddObject(m, "Custom", (PyObject *) &CustomType) < 0) {
        Py_DECREF(&CustomType);
        Py_DECREF(m);
        return NULL;
    }

    return m;
}

Linked PRs

@arogge arogge added the type-bug An unexpected behavior, bug, or error label Nov 7, 2022
arogge added a commit to alaaeddineelamri/bareos that referenced this issue Nov 7, 2022
C++20 supports designated initializers. However, it doesn't allow
mixing of named and unnamed initialization. As we need to use the
PyVarObject_HEAD_INIT() macro, which doesn't name the attributes it
sets, we have to downgrade the compiler to C++17, so designated
initializers are handled as compiler extension, which will allow the
mixing that occurs here.

See also: python/cpython#99202
arogge added a commit to alaaeddineelamri/bareos that referenced this issue Nov 7, 2022
C++20 supports designated initializers. However, it doesn't allow
mixing of named and unnamed initialization. As we need to use the
PyVarObject_HEAD_INIT() macro, which doesn't name the attributes it
sets, we have to downgrade the compiler to C++17, so designated
initializers are handled as compiler extension, which will allow the
mixing that occurs here.

See also: python/cpython#99202
arogge added a commit to alaaeddineelamri/bareos that referenced this issue Nov 7, 2022
C++20 supports designated initializers. However, it doesn't allow
mixing of named and unnamed initialization. As we need to use the
PyVarObject_HEAD_INIT() macro, which doesn't name the attributes it
sets, we have to downgrade the compiler to C++17, so designated
initializers are handled as compiler extension, which will allow the
mixing that occurs here.

See also: python/cpython#99202
arogge added a commit to alaaeddineelamri/bareos that referenced this issue Nov 7, 2022
C++20 supports designated initializers. However, it doesn't allow
mixing of named and unnamed initialization. As we need to use the
PyVarObject_HEAD_INIT() macro, which doesn't name the attributes it
sets, we have to downgrade the compiler to C++17, so designated
initializers are handled as compiler extension, which will allow the
mixing that occurs here.

See also: python/cpython#99202
@vstinner
Copy link
Member

One option would be to use designed initializers in HEAD macros (ex: PyObject_HEAD) when using C++ 20 or newer.

I added _Py_NULL, Py_STATIC_CAST() and _Py_CAST() macros to Python 3.11 to enhance C++ compatibility. _Py_NULL is defined differently depending on the C++ version:

// Static inline functions should use _Py_NULL rather than using directly NULL
// to prevent C++ compiler warnings. On C++11 and newer, _Py_NULL is defined as
// nullptr.
#if defined(__cplusplus) && __cplusplus >= 201103
#  define _Py_NULL nullptr
#else
#  define _Py_NULL NULL
#endif

_Py_STATIC_CAST():

// Macro to use C++ static_cast<> in the Python C API.
#ifdef __cplusplus
#  define _Py_STATIC_CAST(type, expr) static_cast<type>(expr)
#else
#  define _Py_STATIC_CAST(type, expr) ((type)(expr))
#endif

_Py_CAST() had an implementation to avoid old-style C cast, but sadly this work had to be reverted before Python 3.11 final to fix a build error. Currently, it's just:

#define _Py_CAST(type, expr) ((type)(expr))

cc @erlend-aasland

@erlend-aasland erlend-aasland added docs Documentation in the Doc dir topic-C-API labels Nov 15, 2022
@encukou
Copy link
Member

encukou commented Nov 15, 2022

One option would be to use designed initializers in HEAD macros (ex: PyObject_HEAD) when using C++ 20 or newer.

That would mean projects that list all the fields would now break in C++20.
Maybe we need a new variant of PyVarObject_HEAD_INIT with designated initializers :/

@erlend-aasland
Copy link
Contributor

We should be able to use the preprocessor to provide PyVarObject_HEAD_INIT with designated initializers based on the C++ standard.

@vstinner
Copy link
Member

vstinner commented Nov 15, 2022

I really hate defining a structure without naming members. If we have to chose, I propose to move away from that. Nobody likes that, and the majority of code creating structures in Python... just name the members with comments (which are hard to write and maintain because of indentation, trying to align text, etc.).

"Random" example:

PyTypeObject PyType_Type = {
    PyVarObject_HEAD_INIT(&PyType_Type, 0)
    "type",                                     /* tp_name */
    sizeof(PyHeapTypeObject),                   /* tp_basicsize */
    sizeof(PyMemberDef),                        /* tp_itemsize */
    (destructor)type_dealloc,                   /* tp_dealloc */
    offsetof(PyTypeObject, tp_vectorcall),      /* tp_vectorcall_offset */
    0,                                          /* tp_getattr */
    0,                                          /* tp_setattr */
    0,                                          /* tp_as_async */
    (reprfunc)type_repr,                        /* tp_repr */
    &type_as_number,                            /* tp_as_number */
    0,                                          /* tp_as_sequence */
    0,                                          /* tp_as_mapping */
    0,                                          /* tp_hash */
    (ternaryfunc)type_call,                     /* tp_call */
    0,                                          /* tp_str */
    (getattrofunc)type_getattro,                /* tp_getattro */
    (setattrofunc)type_setattro,                /* tp_setattro */
    0,                                          /* tp_as_buffer */
    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC |
    Py_TPFLAGS_BASETYPE | Py_TPFLAGS_TYPE_SUBCLASS |
    Py_TPFLAGS_HAVE_VECTORCALL,                 /* tp_flags */
    type_doc,                                   /* tp_doc */
    (traverseproc)type_traverse,                /* tp_traverse */
    (inquiry)type_clear,                        /* tp_clear */
    0,                                          /* tp_richcompare */
    offsetof(PyTypeObject, tp_weaklist),        /* tp_weaklistoffset */
    0,                                          /* tp_iter */
    0,                                          /* tp_iternext */
    type_methods,                               /* tp_methods */
    type_members,                               /* tp_members */
    type_getsets,                               /* tp_getset */
    0,                                          /* tp_base */
    0,                                          /* tp_dict */
    0,                                          /* tp_descr_get */
    0,                                          /* tp_descr_set */
    offsetof(PyTypeObject, tp_dict),            /* tp_dictoffset */
    type_init,                                  /* tp_init */
    0,                                          /* tp_alloc */
    type_new,                                   /* tp_new */
    PyObject_GC_Del,                            /* tp_free */
    (inquiry)type_is_gc,                        /* tp_is_gc */
    .tp_vectorcall = type_vectorcall,
};

It's made of 3 parts:

  • PyVarObject_HEAD_INIT(&PyType_Type, 0): designed initalizers? Who knows!
  • "type", /* tp_name */: member names as comments
  • and.... at the very end... .tp_vectorcall = type_vectorcall,: designated initialized! Yeah!

I would prefer to convert all PyTypeObject definition in Python to only use designed initializers. Static types converted to heap types use designed initializers and it's so way readable, since most fields use the default value (0/NULL)! So it's way shorter.

By the way, it's a little bit surprising for me that PyVarObject_HEAD_INIT(&PyType_Type, 0) usage doesn't end with a , but it can no longer be changed :-(

@vstinner
Copy link
Member

pymod.cc:11:5: error: either all initializer clauses should be designated or none of them should be

FYI I got it as a warning with older C++ versions. I tried to fix them by using designed initializers in HEAD macros, but I get issues and I gave up. My attempt was to build C++ extensions in "strict mode" (turn on all warnings, treat warnings as errors) in test_cppext.

@erlend-aasland
Copy link
Contributor

@vstinner, I agree that using designated initializers consistently in the CPython code base would be a great improvement in readability. I would be in favour of such a change.

@bruno-at-bareos
Copy link

Also seeing on openSUSE/SLE 15.4 gcc-12.2.1

e992cf44cf4d:/tmp/cpython # g++ --version
g++ (SUSE Linux) 12.2.1 20220830 [revision e927d1cf141f221c5a32574bde0913307e140984]
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

g++ $(python3-config --cflags) -std=c++17 -Wall -Wextra -Wno-missing-field-initializers -c pymod.cc

g++ $(python3-config --cflags) -std=c++20 -Wall -Wextra -Wno-missing-field-initializers -c pymod.cc
pymod.cc:11:5: error: either all initializer clauses should be designated or none of them should be
   11 |     .tp_name = "custom.Custom",
      |     ^
pymod.cc:20:5: error: either all initializer clauses should be designated or none of them should be
   20 |     .m_name = "custom",
      |     ^

@vstinner
Copy link
Member

vstinner commented Apr 5, 2023

One option would be to use designed initializers in HEAD macros (ex: PyObject_HEAD) when using C++ 20 or newer.

The problem with this is that it's backward incompatible with code which is currently valid in C++20! Code using designed initializers like .ob_base = ...:

static PyTypeObject CustomType = {
    .ob_base = PyVarObject_HEAD_INIT(NULL, 0)
    .tp_name = "custom.Custom",
    (...)
};

PR #102518 is backward compatible.

@vstinner
Copy link
Member

vstinner commented Apr 5, 2023

Well, there are two parts.

Part 1: outer level, code using INIT macros:

static PyTypeObject CustomType = {
    .ob_base = xxx
    (...)
};

Part 2: INIT macro declares {...} without designed initializers:

#define PyObject_HEAD_INIT(type)        \
    { _PyObject_EXTRA_INIT              \
    1, (type) },

PR #102518 fix part 1, it leaves the part 2 unchanged (unfixed?).

The current status is that test_cppext builds a C++ extension for C++03 and C++11. The test fails if the compiler emits warnings, but doesn't pass any specific compiler flags.

The pythoncapi-compat project has a similar test with stricter compiler flags:

    # Treat warnings as error
    '-Werror',

    # Enable all warnings
    '-Wall', '-Wextra',

    # Extra warnings
    '-Wconversion',

    # /usr/lib64/pypy3.7/include/pyport.h:68:20: error: redefinition of typedef
    # 'Py_hash_t' is a C11 feature
    "-Wno-typedef-redefinition",

So far, I failed to use -Wold-style-cast and -Wzero-as-null-pointer-constant flags in pythoncapi-compat, especially because of old-style C casts in old Python versions: #94731 My attempt to avoid old-style C casts in the Python C API was reverted during Python 3.11 beta phase: commit 6cbb57f. C++ is hard and CPython lacks C++ experts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir topic-C-API type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

5 participants