PyTuple_SET_ITEM fails to compile in C++ source #93442

nascheme · 2022-06-02T20:58:40Z

CPython versions tested on: 3.11b3
Operating system and architecture: Debian Linux, GCC 10.2.1-6

The kiwisolver extension doesn't compile with beta 3. The error is:

/usr/local/python3.11.0b3/include/python3.11/pyport.h:47:24: error: invalid ‘static_cast’ from type ‘int’ to type ‘_object*’
   47 |                 return static_cast<type>(const_cast<expr_type &>(expr));
      |                        ^~~~~

This seems to be an issue with the new inline functions for things that used to be macros. E.g. PyTuple_SET_ITEM. The C++ code triggering the issue is:

--- a/py/src/symbolics.h
+++ b/py/src/symbolics.h
@@ -123,7 +123,7 @@ PyObject* BinaryMul::operator()( Expression* first, double second )
                return 0;
        Py_ssize_t end = PyTuple_GET_SIZE( first->terms );
        for( Py_ssize_t i = 0; i < end; ++i )  // memset 0 for safe error return
-               PyTuple_SET_ITEM( terms.get(), i, 0 );
+               PyTuple_SetItem( terms.get(), i, 0 );
        for( Py_ssize_t i = 0; i < end; ++i )
        {
                PyObject* item = PyTuple_GET_ITEM( first->terms, i );

Replacing the macro with the function version seems to allow the extension to compile.

This seems related to #92898

The text was updated successfully, but these errors were encountered:

vstinner · 2022-06-02T22:29:09Z

What is the result type of terms.get()? Can you write a short C++ code reproducing the issue?

I guess that the cast which fails is _PyObject_CAST(0): 3rd argument of PyTuple_SET_ITEM(). The function expects a PyObject* and you pass 0 as a pointer.

nascheme · 2022-06-02T22:49:03Z

My understanding is that in C NULL and 0 can be used interchangeably. Maybe in modern C++ versions that's not the case?

https://stackoverflow.com/questions/176989/do-you-use-null-or-0-zero-for-pointers-in-c

For the kiwi source code, using NULL instead of 0 doesn't fix the error. Making a local variable for val does work:

diff --git a/py/src/util.h b/py/src/util.h
index 4b00fa7..6f51bca 100644
--- a/py/src/util.h
+++ b/py/src/util.h
@@ -116,8 +116,10 @@ make_terms( const std::map<PyObject*, double>& coeffs )
     if( !terms )
         return 0;
     Py_ssize_t size = PyTuple_GET_SIZE( terms.get() );
-    for( Py_ssize_t i = 0; i < size; ++i ) // zero tuple for safe early return
-        PyTuple_SET_ITEM( terms.get(), i, 0 );
+    for( Py_ssize_t i = 0; i < size; ++i ) { // zero tuple for safe early return
+        PyObject *val = 0;
+        PyTuple_SET_ITEM( terms.get(), i, val );
+    }
     Py_ssize_t i = 0;
     iter_t it = coeffs.begin();
     iter_t end = coeffs.end();

My guess is that the macro for PyTuple_GET_SIZE does not treat 0/NULL correctly but I don't know how you would fix it.

vstinner · 2022-06-02T22:50:16Z

Maybe in modern C++ versions that's not the case?

In C++, nullptr should be used to pass a "null pointer" (0), no?

Well, Python should accept 0 as PyObject*.

vstinner · 2022-06-02T23:03:55Z

The issue can be reproduced by running test_cppext with this change:

diff --git a/Lib/test/_testcppext.cpp b/Lib/test/_testcppext.cpp
index eade7ccdaa..70f434e678 100644
--- a/Lib/test/_testcppext.cpp
+++ b/Lib/test/_testcppext.cpp
@@ -74,6 +74,10 @@ test_api_casts(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
     Py_INCREF(strong_ref);
     Py_DECREF(strong_ref);
 
+    // gh-93442: Pass 0 as NULL for PyObject*
+    Py_XINCREF(0);
+    Py_XDECREF(0);
+
     Py_DECREF(obj);
     Py_RETURN_NONE;
 }

nascheme · 2022-06-02T23:33:38Z

My guess is that the _Py_CAST_impl needs to have an overload to handle nullptr_t as the argument. Perhaps then _Py_NULL should not be needed. We would like that API users can still use 0 and NULL as args to these macros.

nascheme · 2022-06-03T01:21:30Z

It looks like _Py_CAST_impl came about as a result of #91959. I wonder though if the cure is worse than the disease. I.e. previously we had warnings when you used -Wold-style-cast. Now you have C++ extensions having compile errors if you don't use nullptr instead of the old style NULL or 0. Can we just go back to:

#define _PyObject_CAST(op) ((PyObject *)(op))

and similar macros? Maybe the _Py_CAST_impl template magic can be opt-in in some way, for people who want to use -Wold-style-cast and not have errors.

serge-sans-paille · 2022-06-03T13:41:28Z

The following overloads seem to be needed to cover all cases

#include <cstdlib>
typedef struct PyObject { int x ; } PyObject;

extern "C++" {
    namespace {
        template <typename type>
        inline type _Py_CAST_impl(long int ptr) {
            return reinterpret_cast<type>(ptr);
        }
        template <typename type>
        inline type _Py_CAST_impl(int ptr) {
            return reinterpret_cast<type>(ptr);
        }
        template <typename type>
        inline type _Py_CAST_impl(std::nullptr_t) {
            return static_cast<type>(nullptr);
        }

        template <typename type, typename expr_type>
            inline type _Py_CAST_impl(expr_type *expr) {
                return reinterpret_cast<type>(expr);
            }

        template <typename type, typename expr_type>
            inline type _Py_CAST_impl(expr_type const *expr) {
                return reinterpret_cast<type>(const_cast<expr_type *>(expr));
            }

        template <typename type, typename expr_type>
            inline type _Py_CAST_impl(expr_type &expr) {
                return static_cast<type>(expr);
            }

        template <typename type, typename expr_type>
            inline type _Py_CAST_impl(expr_type const &expr) {
                return static_cast<type>(
                    const_cast<expr_type &>(expr)
                );
            }
    }
}
#  define _Py_CAST(type, expr) _Py_CAST_impl<type>(expr)

int main()
{
    PyObject *obj = _Py_CAST(PyObject*, nullptr);
    PyObject *obj2 = _Py_CAST(PyObject*, 0);
    PyObject *obj3 = _Py_CAST(PyObject*, NULL);
    return (obj == nullptr);
}

nascheme · 2022-06-03T21:48:28Z

I can confirm this change makes kiwi compile without having to make source code changes. 🎉

corona10 · 2022-06-04T07:19:55Z

@serge-sans-paille would you like to submit the PR for this issue? cc @nascheme @vstinner

Add C++ overloads for _Py_CAST_impl() to handle 0/NULL. This will allow C++ extensions that pass 0 or NULL to macros using _Py_CAST() to continue to compile. Without this, you get an error like: invalid ‘static_cast’ from type ‘int’ to type ‘_object*’ The modern way to use a NULL value in C++ is to use nullptr. However, we want to not break extensions that do things the old way. Co-authored-by: serge-sans-paille

…nGH-93500) Add C++ overloads for _Py_CAST_impl() to handle 0/NULL. This will allow C++ extensions that pass 0 or NULL to macros using _Py_CAST() to continue to compile. Without this, you get an error like: invalid ‘static_cast’ from type ‘int’ to type ‘_object*’ The modern way to use a NULL value in C++ is to use nullptr. However, we want to not break extensions that do things the old way. Co-authored-by: serge-sans-paille (cherry picked from commit 8bcc3fa) Co-authored-by: Neil Schemenauer <nas-github@arctrix.com>

…h-93507) Add C++ overloads for _Py_CAST_impl() to handle 0/NULL. This will allow C++ extensions that pass 0 or NULL to macros using _Py_CAST() to continue to compile. Without this, you get an error like: invalid ‘static_cast’ from type ‘int’ to type ‘_object*’ The modern way to use a NULL value in C++ is to use nullptr. However, we want to not break extensions that do things the old way. Co-authored-by: serge-sans-paille (cherry picked from commit 8bcc3fa) Co-authored-by: Neil Schemenauer <nas-github@arctrix.com> Co-authored-by: Neil Schemenauer <nas-github@arctrix.com>

(cherry picked from commit 713eb18) Co-authored-by: Neil Schemenauer <nas-github@arctrix.com>

(cherry picked from commit 713eb18) Co-authored-by: Neil Schemenauer <nas-github@arctrix.com> Co-authored-by: Neil Schemenauer <nas-github@arctrix.com>

Fix python#93442

nascheme added the type-bug label Jun 2, 2022

nascheme assigned vstinner Jun 2, 2022

nascheme mentioned this issue Jun 2, 2022

pip install fails on Python 3.11b3 nucleic/kiwi#141

Open

AA-Turner added 3.11 expert-C-API 3.12 labels Jun 2, 2022

nascheme linked a pull request Jun 4, 2022 that will close this issue

gh-93442: Make C++ version of _Py_CAST work with 0/NULL. #93500

Merged

nascheme closed this as completed in #93500 Jun 5, 2022

corona10 pushed a commit that referenced this issue Jun 5, 2022

gh-93442: Add test for _Py_CAST(nullptr). (gh-93505)

713eb18

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 5, 2022

pythongh-93442: Add test for _Py_CAST(nullptr). (pythongh-93505)

e6ec695

(cherry picked from commit 713eb18) Co-authored-by: Neil Schemenauer <nas-github@arctrix.com>

corona10 pushed a commit that referenced this issue Jun 5, 2022

gh-93442: Add test for _Py_CAST(nullptr). (gh-93505) (gh-93509)

c21f2bd

(cherry picked from commit 713eb18) Co-authored-by: Neil Schemenauer <nas-github@arctrix.com> Co-authored-by: Neil Schemenauer <nas-github@arctrix.com>

serge-sans-paille added a commit to serge-sans-paille/cpython that referenced this issue Jun 5, 2022

Add support for casting nullptr in _Py_CAST_impl

5cf29ce

Fix python#93442

serge-sans-paille mentioned this issue Jun 5, 2022

gh-93442: Add support for casting nullptr in _Py_CAST_impl #93515

Closed

PyTuple_SET_ITEM fails to compile in C++ source #93442

PyTuple_SET_ITEM fails to compile in C++ source #93442

nascheme commented Jun 2, 2022

vstinner commented Jun 2, 2022

nascheme commented Jun 2, 2022 •

edited

vstinner commented Jun 2, 2022

vstinner commented Jun 2, 2022

nascheme commented Jun 2, 2022

nascheme commented Jun 3, 2022

serge-sans-paille commented Jun 3, 2022

nascheme commented Jun 3, 2022

corona10 commented Jun 4, 2022

PyTuple_SET_ITEM fails to compile in C++ source #93442

PyTuple_SET_ITEM fails to compile in C++ source #93442

Comments

nascheme commented Jun 2, 2022

vstinner commented Jun 2, 2022

nascheme commented Jun 2, 2022 • edited

vstinner commented Jun 2, 2022

vstinner commented Jun 2, 2022

nascheme commented Jun 2, 2022

nascheme commented Jun 3, 2022

serge-sans-paille commented Jun 3, 2022

nascheme commented Jun 3, 2022

corona10 commented Jun 4, 2022

nascheme commented Jun 2, 2022 •

edited