GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-92135: Fix _Py_reinterpret_cast() for const by vstinner · Pull Request #92138 · python/cpython

Conversation

vstinner commented May 2, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Fix C++ compiler warnings on cast macros, like _PyObject_CAST(), when
casting a constant expression to a non constant type: use
const_cast<> in C++.

In C++, Py_SAFE_DOWNCAST() now uses static_cast<> rather than
reinterpret_cast<>.
Add tests to the _testcppext C++ extension.
test_cppext no longer captures stdout in verbose mode.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added the awaiting core review label

May 2, 2022

vstinner commented May 2, 2022

Copy link

Copy Markdown

Member Author

@tacaswell @serge-sans-paille: Does my _Py_reinterpret_const_cast() macro looks like a monster, or does it look correct?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner added the skip news label

May 2, 2022

vstinner commented May 2, 2022

Copy link

Copy Markdown

Member Author

I merged my PR #32175 adding C++ tests to Python stdlib. So I rebased this PR on top of it and I added tests for this PR.

Currently, tests are not executed: only compiler warnings are checked by test_cppext. It is enough for checking that issue #92135 is fixed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland commented May 2, 2022

Copy link

Copy Markdown

Contributor

FTR, in another project, I recently had to use two casts in order to avoid G++ warnings:

void inner_set(void *);

// original code (generated warnings)
void set_func(const char *str) {
    inner_set(str);
}

// new set func, still generated warnings
void improved_set_func(const char *str) {
    inner_set(const_cast<void *>(str));
}

// final set func, no warnings
void set_func_without_warning(const char *str) {
    const void *vptr = static_cast<const void *>(str);
    inner_set(const_cast<void *>(vptr));
}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          gh-92135: Fix _Py_reinterpret_cast() for const

31826b0

Fix C++ compiler warnings on cast macros, like _PyObject_CAST(), when
casting a constant expression to a non constant type: use
const_cast<> in C++.

* In C++, Py_SAFE_DOWNCAST() now uses static_cast<> rather than
  reinterpret_cast<>.
* Add tests to the _testcppext C++ extension.
* test_cppext no longer captures stdout in verbose mode.

vstinner changed the title ~~gh-92135: Add _Py_reinterpret_const_cast() macro~~ gh-92135: Fix _Py_reinterpret_cast() for const

May 2, 2022

vstinner commented May 2, 2022

Copy link

Copy Markdown

Member Author

FTR, in another project, I recently had to use two casts in order to avoid G++ warnings

I understand that this approach (updated _Py_reinterpret_cast()) is correct ;-)

First, I renamed the macro _Py_reinterpret_const_cast(), but @serge-sans-paille didn't like this name, and me neither (it's too long!). So I kept the name _Py_reinterpret_cast().

erlend-aasland reacted with hooray emoji

All reactions

🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland approved these changes

May 2, 2022

View reviewed changes

vstinner merged commit 0313970 into python:main

May 2, 2022

vstinner deleted the const_cast branch

May 2, 2022 15:07

bedevere-bot removed the awaiting core review label

May 2, 2022

vstinner mentioned this pull request

May 2, 2022

In C++, Python 3.11 C API introduces new compiler warnings/errors #92135

Closed

vstinner commented May 2, 2022

Copy link

Copy Markdown

Member Author

@tacaswell: This issue should now be fixed. Please try greenlet with on the main branch of Python.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tacaswell commented May 2, 2022

Copy link

Copy Markdown

Contributor

The const issues is fixed the other casting issues still remains

✔ 13:12:04 $ gcc -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/home/tcaswell/.virtualenvs/bleeding/include -I/home/tcaswell/.pybuild/bleeding/include/python3.11 -c src/greenlet/greenlet.cpp -o build/temp.linux-x86_64-3.11/src/greenlet/greenlet.o
 
In file included from /home/tcaswell/.pybuild/bleeding/include/python3.11/Python.h:38,
                 from src/greenlet/greenlet.cpp:14:
src/greenlet/greenlet_refs.hpp: In member function ‘void greenlet::refs::PyErrPieces::normalize()’:
/home/tcaswell/.pybuild/bleeding/include/python3.11/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))
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/object.h:107:28: note: in expansion of macro ‘_Py_reinterpret_cast’
  107 | #define _PyObject_CAST(op) _Py_reinterpret_cast(PyObject*, (op))
      |                            ^~~~~~~~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/object.h:780:41: note: in expansion of macro ‘_PyObject_CAST’
  780 | #  define PyType_Check(op) PyType_Check(_PyObject_CAST(op))
      |                                         ^~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/pyerrors.h:57:6: note: in expansion of macro ‘PyType_Check’
   57 |     (PyType_Check((x)) &&                                               \
      |      ^~~~~~~~~~~~
src/greenlet/greenlet_refs.hpp:993:17: note: in expansion of macro ‘PyExceptionClass_Check’
  993 |             if (PyExceptionClass_Check(type)) {
      |                 ^~~~~~~~~~~~~~~~~~~~~~
In file included from /home/tcaswell/.pybuild/bleeding/include/python3.11/Python.h:44,
                 from src/greenlet/greenlet.cpp:14:
/home/tcaswell/.pybuild/bleeding/include/python3.11/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))
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/object.h:774:59: note: in definition of macro ‘PyType_FastSubclass’
  774 | #define PyType_FastSubclass(type, flag) PyType_HasFeature(type, flag)
      |                                                           ^~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/object.h:107:28: note: in expansion of macro ‘_Py_reinterpret_cast’
  107 | #define _PyObject_CAST(op) _Py_reinterpret_cast(PyObject*, (op))
      |                            ^~~~~~~~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/object.h:136:31: note: in expansion of macro ‘_PyObject_CAST’
  136 | #  define Py_TYPE(ob) Py_TYPE(_PyObject_CAST(ob))
      |                               ^~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/pyerrors.h:61:25: note: in expansion of macro ‘Py_TYPE’
   61 |     PyType_FastSubclass(Py_TYPE(x), Py_TPFLAGS_BASE_EXC_SUBCLASS)
      |                         ^~~~~~~
src/greenlet/greenlet_refs.hpp:1003:22: note: in expansion of macro ‘PyExceptionInstance_Check’
 1003 |             else if (PyExceptionInstance_Check(type)) {
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/tcaswell/.pybuild/bleeding/include/python3.11/Python.h:38,
                 from src/greenlet/greenlet.cpp:14:
src/greenlet/greenlet_thread_state.hpp: In destructor ‘greenlet::ThreadState::~ThreadState()’:
/home/tcaswell/.pybuild/bleeding/include/python3.11/pyport.h:30:25: error: invalid cast from type ‘greenlet::refs::BorrowedObject’ {aka ‘greenlet::refs::BorrowedReference<_object, greenlet::refs::NoOpChecker>’} to type ‘const PyObject*’ {aka ‘const _object*’}
   30 |        const_cast<type>(reinterpret_cast<const type>(expr))
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/object.h:107:28: note: in expansion of macro ‘_Py_reinterpret_cast’
  107 | #define _PyObject_CAST(op) _Py_reinterpret_cast(PyObject*, (op))
      |                            ^~~~~~~~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/object.h:265:59: note: in expansion of macro ‘_PyObject_CAST’
  265 | #  define PyObject_TypeCheck(ob, type) PyObject_TypeCheck(_PyObject_CAST(ob), type)
      |                                                           ^~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/methodobject.h:17:31: note: in expansion of macro ‘PyObject_TypeCheck’
   17 | #define PyCFunction_Check(op) PyObject_TypeCheck(op, &PyCFunction_Type)
      |                               ^~~~~~~~~~~~~~~~~~
src/greenlet/greenlet_thread_state.hpp:400:33: note: in expansion of macro ‘PyCFunction_Check’
  400 |                              && PyCFunction_Check(refs.at(0))
      |                                 ^~~~~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/pyport.h:30:25: error: invalid cast from type ‘greenlet::refs::BorrowedObject’ {aka ‘greenlet::refs::BorrowedReference<_object, greenlet::refs::NoOpChecker>’} to type ‘const PyObject*’ {aka ‘const _object*’}
   30 |        const_cast<type>(reinterpret_cast<const type>(expr))
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/object.h:107:28: note: in expansion of macro ‘_Py_reinterpret_cast’
  107 | #define _PyObject_CAST(op) _Py_reinterpret_cast(PyObject*, (op))
      |                            ^~~~~~~~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/object.h:127:35: note: in expansion of macro ‘_PyObject_CAST’
  127 | #  define Py_REFCNT(ob) Py_REFCNT(_PyObject_CAST(ob))
      |                                   ^~~~~~~~~~~~~~
src/greenlet/greenlet_thread_state.hpp:401:33: note: in expansion of macro ‘Py_REFCNT’
  401 |                              && Py_REFCNT(refs.at(0)) == 2) {
      |                                 ^~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/pyport.h:30:25: error: invalid cast from type ‘greenlet::refs::BorrowedObject’ {aka ‘greenlet::refs::BorrowedReference<_object, greenlet::refs::NoOpChecker>’} to type ‘const PyObject*’ {aka ‘const _object*’}
   30 |        const_cast<type>(reinterpret_cast<const type>(expr))
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/object.h:107:28: note: in expansion of macro ‘_Py_reinterpret_cast’
  107 | #define _PyObject_CAST(op) _Py_reinterpret_cast(PyObject*, (op))
      |                            ^~~~~~~~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/object.h:580:29: note: in expansion of macro ‘_PyObject_CAST’
  580 |         PyObject *_py_tmp = _PyObject_CAST(op); \
      |                             ^~~~~~~~~~~~~~
src/greenlet/greenlet_thread_state.hpp:422:33: note: in expansion of macro ‘Py_CLEAR’
  422 |                                 Py_CLEAR(function_w);
      |                                 ^~~~~~~~
src/greenlet/greenlet.cpp: In function ‘PyObject* green_repr(greenlet::refs::BorrowedGreenlet)’:
/home/tcaswell/.pybuild/bleeding/include/python3.11/pyport.h:30:25: error: invalid cast from type ‘greenlet::refs::BorrowedGreenlet’ {aka ‘greenlet::refs::_BorrowedGreenlet<_greenlet, greenlet::refs::GreenletChecker>’} to type ‘const PyObject*’ {aka ‘const _object*’}
   30 |        const_cast<type>(reinterpret_cast<const type>(expr))
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/object.h:107:28: note: in expansion of macro ‘_Py_reinterpret_cast’
  107 | #define _PyObject_CAST(op) _Py_reinterpret_cast(PyObject*, (op))
      |                            ^~~~~~~~~~~~~~~~~~~~
/home/tcaswell/.pybuild/bleeding/include/python3.11/object.h:136:31: note: in expansion of macro ‘_PyObject_CAST’
  136 | #  define Py_TYPE(ob) Py_TYPE(_PyObject_CAST(ob))
      |                               ^~~~~~~~~~~~~~
src/greenlet/greenlet.cpp:2395:33: note: in expansion of macro ‘Py_TYPE’
 2395 |     const char* const tp_name = Py_TYPE(self)->tp_name;
      |                                 ^~~~~~~

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tacaswell mentioned this pull request

May 2, 2022

Clarify that const is being cast away python-greenlet/greenlet#302

Closed

vstinner commented May 3, 2022

Copy link

Copy Markdown

Member Author

/home/tcaswell/.pybuild/bleeding/include/python3.11/pyport.h:30:25: error: invalid cast from type ‘greenlet::refs::OwnedErrPiece’ to type ‘const PyObject*’ {aka ‘const _object*’}

I have no idea how to fix this cast. Do you have an idea? Does greenlet::refs::OwnedErrPiece inherit somehow from PyObject? Is it a pointer?

Maybe greenlet should be modified to cast to PyObject*.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tacaswell commented May 3, 2022

Copy link

Copy Markdown

Contributor

I have no idea how to fix this cast. Do you have an idea?

I think we need an actual C++ expert to tell us if what greenlets is doing is fully kosher or undefined behavior that just happened to work before.

Maybe greenlet should be modified to cast to PyObject*.

python-greenlet/greenlet@50f8786 is one way that works.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serge-sans-paille commented May 4, 2022

Copy link

Copy Markdown

Contributor

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:

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

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));}

(see https://godbolt.org/z/9vdzj4fcn)

erlend-aasland reacted with eyes emoji

All reactions

👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tacaswell mentioned this pull request

May 14, 2022

PyUnicode_READ() fails to build on C++: _Py_CAST() used with a constant type (const void*) #92800

Closed

methane mentioned this pull request

May 15, 2022

speed.python.org has no new results since May 7 python/pyperformance#198

Closed

tacaswell mentioned this pull request

May 17, 2022

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

Closed

Reviewers

erlend-aasland

erlend-aasland approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

vstinner commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented May 2, 2022

Uh oh!

vstinner commented May 2, 2022

Uh oh!

erlend-aasland commented May 2, 2022

Uh oh!

vstinner commented May 2, 2022

Uh oh!

vstinner commented May 2, 2022

Uh oh!

tacaswell commented May 2, 2022

Uh oh!

vstinner commented May 3, 2022

Uh oh!

tacaswell commented May 3, 2022

Uh oh!

serge-sans-paille commented May 4, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vstinner commented May 2, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.