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

changes to PyObject casting macros causing build failures with python-greenlet #92898

Closed
tacaswell opened this issue May 17, 2022 · 5 comments · Fixed by #92951
Closed

changes to PyObject casting macros causing build failures with python-greenlet #92898

tacaswell opened this issue May 17, 2022 · 5 comments · Fixed by #92951
Assignees
Labels
3.11 only security fixes type-bug An unexpected behavior, bug, or error

Comments

@tacaswell
Copy link
Contributor

tacaswell commented May 17, 2022

Bug report

There have been changes to how the macros for casting PyObject pointers per #91959 (comment) by @vstinner any new compiler warnings are a regression.

There were two types of errors in greenlet, one related to const correctness which has been fixed via #92138) and one related to casting wrapper classes (that has not been fixed).

Per @serge-sans-paille in #92138 (comment)

That's an interesting situation: greenlets are written in C++, and the object that raises issues is of type greenlet::refs::OwnedErrPiece that has a conversion operator to PyObject* (namely: https://github.com/python-greenlet/greenlet/blob/be41e1a24925326b72a02ef5cb6d1ed9643eb062/src/greenlet/greenlet_refs.hpp#L924 ) Using C-style cast or static_cast directly uses that operator so that's fine. But a reinterpret_cast won't work.

and suggests a patch to CPython.

python-greenlet/greenlet#302 is a fix on the greenlet side.

The key line of the error is

src/greenlet/greenlet_refs.hpp: In member function ‘void greenlet::refs::PyErrPieces::normalize()’:
/home/tcaswell/.pybuild/bleeding/include/python3.12/pyport.h:30:25: error: invalid cast from type ‘greenlet::refs::OwnedErrPiece’ to type ‘const PyObject*’ {aka ‘const _object*’}
   30 |        const_cast<type>(reinterpret_cast<const type>(expr))
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This is also affecting python/pyperformance#198

A bunch of links: #92800 (comment)

Your environment

  • CPython versions tested on: main branch
  • Operating system and architecture: linux and x86_64
@tacaswell tacaswell added the type-bug An unexpected behavior, bug, or error label May 17, 2022
@methane methane changed the title changes to PyQbject casting macros causing build failures with python-greenlet changes to PyObject casting macros causing build failures with python-greenlet May 18, 2022
@vstinner
Copy link
Member

@corona10 corona10 assigned vstinner 4 hours ago

Well, you can assign the issue to me, but I'm not sure that I have the C++ skills to fix this specific issue.

@corona10
Copy link
Member

Well, you can assign the issue to me, but I'm not sure that I have the C++ skills to fix this specific issue.

Okay, I will also take a look at this issue too.

@corona10 corona10 added the 3.11 only security fixes label May 18, 2022
@corona10
Copy link
Member

corona10 commented May 19, 2022

That's an interesting situation: greenlets are written in C++, and the object that raises issues is of type greenlet::refs::OwnedErrPiece that has a conversion operator to PyObject* (namely: https://github.com/python-greenlet/greenlet/blob/be41e1a24925326b72a02ef5cb6d1ed9643eb062/src/greenlet/greenlet_refs.hpp#L924 ) Using C-style cast or static_cast directly uses that operator so that's fine. But a reinterpret_cast won't work.

According to me, this means we need something along these lines:

@serge-sans-paille cc @tacaswell

IMHO implementing the conversion operator for PyObject* might be a common usecases in the C++ world,
if there are no more other use cases with _Py_reinterpret_cast, it would be good to support this case.
Would you like to submit the PR?

@vstinner
Copy link
Member

I wrote a simple C++ program to test casts of many types to PyObject*: py_cast.cpp.txt

@serge-sans-paille's approach using templates works in all cases:

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

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

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

template<typename type, typename expr_type>
type _Py_reinterpret_cast_impl(expr_type const & expr)  { return static_cast<type>(const_cast<expr_type &>(expr));}

#define _Py_CAST(type, expr) _Py_reinterpret_cast_impl<type>(expr)

@vstinner
Copy link
Member

PEP 670 converted more macros to static inline functions. While doing this work, I tried to fix C++ compiler warnings on "old-style cast" by adding _Py_CAST() and _Py_STATIC_CAST(). I tried to minimize the quantity of C++ code in the Python C API header files: in short, it's mostly 2 lines which use static_cast<>, reinterpret_cast<> and const_cast<>.

The problem is that my implementation doesn't cover all cases: it doesn't cover greenlet case which implements a class which has a cast operator to PyObject* on a class which does not inherit from PyObject. The class only contains contains a private pointer to an object.

serge-sans-paille added a commit to serge-sans-paille/cpython that referenced this issue May 19, 2022
Use the adequate kind of cast based on the macro argument: const ref, cont
pointer, ref or pointer.

This should be compatible with C++ overloads.

Fix python#92898
corona10 pushed a commit to corona10/cpython that referenced this issue May 21, 2022
… operator (pythongh-92951)

(cherry picked from commit 5b71b51)

Co-authored-by: serge-sans-paille <[email protected]>
vstinner added a commit that referenced this issue May 26, 2022
* Add StrongRef class.
* Rename and reformat functions of the _Py_CAST() implementation.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 26, 2022
…GH-93111)

* Add StrongRef class.
* Rename and reformat functions of the _Py_CAST() implementation.
(cherry picked from commit 20d30ba)

Co-authored-by: Victor Stinner <[email protected]>
miss-islington added a commit that referenced this issue May 26, 2022
* Add StrongRef class.
* Rename and reformat functions of the _Py_CAST() implementation.
(cherry picked from commit 20d30ba)

Co-authored-by: Victor Stinner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants