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

gh-94731: Revert to C-style casts for _Py_CAST #94782

Merged
merged 8 commits into from
Jul 14, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 3 additions & 51 deletions Include/pyport.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,62 +14,14 @@
#endif


// Macro to use C++ static_cast<>, reinterpret_cast<> and const_cast<>
// in the Python C API.
//
// In C++, _Py_CAST(type, expr) converts a constant expression to a
// non constant type using const_cast<type>. For example,
// _Py_CAST(PyObject*, op) can convert a "const PyObject*" to
// "PyObject*".
//
// The type argument must not be a constant type.
// Macro to use C++ static_cast<> in the Python C API.
#ifdef __cplusplus
#include <cstddef>
# define _Py_STATIC_CAST(type, expr) static_cast<type>(expr)
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);
}
#if __cplusplus >= 201103
template <typename type>
inline type _Py_CAST_impl(std::nullptr_t) {
return static_cast<type>(nullptr);
}
#endif

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)

#else
# define _Py_STATIC_CAST(type, expr) ((type)(expr))
# define _Py_CAST(type, expr) ((type)(expr))
#endif
// Macro to use the more powerful/dangerous C-style cast even in C++.
#define _Py_CAST(type, expr) ((type)(expr))

// 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
Expand Down
76 changes: 73 additions & 3 deletions Lib/test/_testcppext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
# define NAME _testcpp03ext
#endif

#define _STR(NAME) #NAME
#define STR(NAME) _STR(NAME)

PyDoc_STRVAR(_testcppext_add_doc,
"add(x, y)\n"
"\n"
Expand Down Expand Up @@ -123,11 +126,77 @@ test_unicode(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
Py_RETURN_NONE;
}

/* Test a `new`-allocated object with a virtual method.
* (https://github.com/python/cpython/issues/94731) */

class VirtualPyObject : public PyObject {
public:
VirtualPyObject();
virtual ~VirtualPyObject() {
delete [] internal_data;
--instance_count;
}
virtual void set_internal_data() {
internal_data[0] = 1;
}
static void dealloc(PyObject* o) {
delete static_cast<VirtualPyObject*>(o);
}

// Number of "living" instances
static int instance_count;
private:
// buffer that can get corrupted
int* internal_data;
};

int VirtualPyObject::instance_count = 0;

PyType_Slot VirtualPyObject_Slots[] = {
{Py_tp_free, (void*)VirtualPyObject::dealloc},
{0, _Py_NULL},
};

PyType_Spec VirtualPyObject_Spec = {
/* .name */ STR(NAME) ".VirtualPyObject",
/* .basicsize */ sizeof(VirtualPyObject),
/* .itemsize */ 0,
/* .flags */ Py_TPFLAGS_DEFAULT,
/* .slots */ VirtualPyObject_Slots,
};

VirtualPyObject::VirtualPyObject() {
// Create a temporary type (just so we don't need to store it)
PyObject *ipotype = PyType_FromSpec(&VirtualPyObject_Spec);
// no good way to signal failure from a C++ constructor, so use assert
// for error handling
assert(ipotype);
assert(PyObject_Init(this, (PyTypeObject *)ipotype));
Py_DECREF(ipotype);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PyObject *ipotype = PyType_FromSpec(&VirtualPyObject_Spec);
// no good way to signal failure from a C++ constructor, so use assert
// for error handling
assert(ipotype);
assert(PyObject_Init(this, (PyTypeObject *)ipotype));
Py_DECREF(ipotype);
PyObject *vpotype = PyType_FromSpec(&VirtualPyObject_Spec);
// no good way to signal failure from a C++ constructor, so use assert
// for error handling
assert(vpotype);
assert(PyObject_Init(this, (PyTypeObject *)vpotype));
Py_DECREF(vpotype);

Just suggesting changing the name slightly here to match the name you used from the class. I think the i from ipotype was a hold-over from the name I used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It's a local variable now so I'll just name it type :)

internal_data = new int[50];
++instance_count;
}

static PyObject *
test_virtual_object(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
{
VirtualPyObject* obj = new VirtualPyObject();
obj->set_internal_data();
Py_DECREF(obj);
if (VirtualPyObject::instance_count != 0) {
return PyErr_Format(
PyExc_AssertionError,
"instance_count should be 0, got %d",
VirtualPyObject::instance_count);
}
Py_RETURN_NONE;
}

static PyMethodDef _testcppext_methods[] = {
{"add", _testcppext_add, METH_VARARGS, _testcppext_add_doc},
{"test_api_casts", test_api_casts, METH_NOARGS, _Py_NULL},
{"test_unicode", test_unicode, METH_NOARGS, _Py_NULL},
{"test_virtual_object", test_virtual_object, METH_NOARGS, _Py_NULL},
// Note: _testcppext_exec currently runs all test functions directly.
// When adding a new one, add a call there.

Expand All @@ -152,6 +221,10 @@ _testcppext_exec(PyObject *module)
if (!result) return -1;
Py_DECREF(result);

result = PyObject_CallMethod(module, "test_virtual_object", "");
if (!result) return -1;
Py_DECREF(result);

return 0;
}

Expand All @@ -163,9 +236,6 @@ static PyModuleDef_Slot _testcppext_slots[] = {

PyDoc_STRVAR(_testcppext_doc, "C++ test extension.");

#define _STR(NAME) #NAME
#define STR(NAME) _STR(NAME)

static struct PyModuleDef _testcppext_module = {
PyModuleDef_HEAD_INIT, // m_base
STR(NAME), // m_name
Expand Down
5 changes: 0 additions & 5 deletions Lib/test/setup_testcppext.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@
# a C++ extension using the Python C API does not emit C++ compiler
# warnings
'-Werror',
# Warn on old-style cast (C cast) like: (PyObject*)op
'-Wold-style-cast',
]
else:
# Don't pass any compiler flag to MSVC
Expand All @@ -37,9 +35,6 @@ def main():
name = '_testcpp11ext'

cppflags = [*CPPFLAGS, f'-std={std}']
if std == 'c++11':
# Warn when using NULL rather than _Py_NULL in static inline functions
cppflags.append('-Wzero-as-null-pointer-constant')

cpp_ext = Extension(
name,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Python again uses C-style casts for most casting operations when compiled
with C++. This may trigger compiler warnings, if they are enabled with e.g.
``-Wold-style-cast `` or ``-Wzero-as-null-pointer-constant`` options for ``g++``.