From 8b48528f7a7d5ea3b5149a1ae642cb6e8d2fc767 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 7 Nov 2024 21:28:44 +0100 Subject: [PATCH] [CPyCppyy] Fix regression in passing buffers through `void *` Credit goes to Wim Lavrijsen (@wlav). See also: https://github.com/wlav/cppyy/issues/272. --- bindings/pyroot/cppyy/CPyCppyy/src/CPyCppyy.h | 4 +- .../pyroot/cppyy/CPyCppyy/src/Converters.cxx | 71 ++++++++++++++ .../cppyy/CPyCppyy/src/LowLevelViews.cxx | 66 ++++++++----- .../pyroot/cppyy/CPyCppyy/src/PyStrings.cxx | 3 + .../pyroot/cppyy/CPyCppyy/src/PyStrings.h | 1 + .../pyroot/cppyy/CPyCppyy/src/Pythonize.cxx | 9 +- .../cppyy/cppyy/test/test_regression.py | 8 +- ...mplicit-conversion-of-any-ctypes-ptr.patch | 93 ------------------- bindings/pyroot/cppyy/sync-upstream | 1 - 9 files changed, 133 insertions(+), 123 deletions(-) delete mode 100644 bindings/pyroot/cppyy/patches/CPyCppyy-Remove-implicit-conversion-of-any-ctypes-ptr.patch diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/CPyCppyy.h b/bindings/pyroot/cppyy/CPyCppyy/src/CPyCppyy.h index 6948099fe194c..e3244bdcfe74b 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/CPyCppyy.h +++ b/bindings/pyroot/cppyy/CPyCppyy/src/CPyCppyy.h @@ -352,7 +352,7 @@ inline PyObject* CPyCppyy_tp_call(PyObject* cb, PyObject* args, size_t, PyObject #endif // weakref forced strong reference -#if PY_VERSION_HEX < 0x30d00f0 +#if PY_VERSION_HEX < 0x30d0000 static inline PyObject* CPyCppyy_GetWeakRef(PyObject* ref) { PyObject* pyobject = PyWeakref_GetObject(ref); if (!pyobject || pyobject == Py_None) @@ -370,7 +370,7 @@ static inline PyObject* CPyCppyy_GetWeakRef(PyObject* ref) { #endif // Py_TYPE as inline function -#if PY_VERSION_HEX < 0x030900A4 && !defined(Py_SET_TYPE) +#if PY_VERSION_HEX < 0x03090000 && !defined(Py_SET_TYPE) static inline void _Py_SET_TYPE(PyObject *ob, PyTypeObject *type) { ob->ob_type = type; } #define Py_SET_TYPE(ob, type) _Py_SET_TYPE((PyObject*)(ob), type) diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx index ea7e1abed246b..f7ccf76488edd 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx @@ -203,6 +203,62 @@ static bool IsPyCArgObject(PyObject* pyobject) return Py_TYPE(pyobject) == pycarg_type; } +#if PY_VERSION_HEX < 0x30d0000 +static bool IsCTypesArrayOrPointer(PyObject* pyobject) +{ + static PyTypeObject* cstgdict_type = nullptr; + if (!cstgdict_type) { + // get any pointer type to initialize the extended dictionary type + PyTypeObject* ct_int = GetCTypesType(ct_c_int); + if (ct_int && ct_int->tp_dict) { + cstgdict_type = Py_TYPE(ct_int->tp_dict); + } + } + + PyTypeObject* pytype = Py_TYPE(pyobject); + if (pytype->tp_dict && Py_TYPE(pytype->tp_dict) == cstgdict_type) + return true; + return false; +} +#else +// the internals of ctypes have been redone, requiring a more complex checking +namespace { + +typedef struct { + PyTypeObject *DictRemover_Type; + PyTypeObject *PyCArg_Type; + PyTypeObject *PyCField_Type; + PyTypeObject *PyCThunk_Type; + PyTypeObject *StructParam_Type; + PyTypeObject *PyCType_Type; + PyTypeObject *PyCStructType_Type; + PyTypeObject *UnionType_Type; + PyTypeObject *PyCPointerType_Type; +// ... unused fields omitted ... +} _cppyy_ctypes_state; + +} // unnamed namespace + +static bool IsCTypesArrayOrPointer(PyObject* pyobject) +{ + static _cppyy_ctypes_state* state = nullptr; + if (!state) { + PyObject* ctmod = PyImport_AddModule("_ctypes"); // the extension module, not the Python one + if (ctmod) + state = (_cppyy_ctypes_state*)PyModule_GetState(ctmod); + } + + // verify for object types that have a C payload + if (state && (PyObject_IsInstance((PyObject*)Py_TYPE(pyobject), (PyObject*)state->PyCType_Type) || + PyObject_IsInstance((PyObject*)Py_TYPE(pyobject), (PyObject*)state->PyCPointerType_Type))) { + return true; + } + + return false; +} +#endif + + //- helper to establish life lines ------------------------------------------- static inline bool SetLifeLine(PyObject* holder, PyObject* target, intptr_t ref) { @@ -1488,6 +1544,16 @@ bool CPyCppyy::VoidArrayConverter::SetArg( return true; } +// allow any other ctypes pointer type + if (IsCTypesArrayOrPointer(pyobject)) { + void** payload = (void**)((CPyCppyy_tagCDataObject*)pyobject)->b_ptr; + if (payload) { + para.fValue.fVoidp = *payload; + para.fTypeCode = 'p'; + return true; + } + } + // final try: attempt to get buffer Py_ssize_t buflen = Utility::GetBuffer(pyobject, '*', 1, para.fValue.fVoidp, false); @@ -1600,6 +1666,11 @@ bool CPyCppyy::name##ArrayConverter::SetArg( \ para.fValue.fVoidp = (void*)((CPyCppyy_tagCDataObject*)pyobject)->b_ptr;\ para.fTypeCode = 'p'; \ convOk = true; \ + } else if (Py_TYPE(pyobject) == GetCTypesType(ct_c_void_p)) { \ + /* special case: pass address of c_void_p buffer to return the address */\ + para.fValue.fVoidp = (void*)((CPyCppyy_tagCDataObject*)pyobject)->b_ptr;\ + para.fTypeCode = 'p'; \ + convOk = true; \ } else if (LowLevelView_Check(pyobject) && \ ((LowLevelView*)pyobject)->fBufInfo.ndim == 2 && \ strchr(((LowLevelView*)pyobject)->fBufInfo.format, code)) { \ diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/LowLevelViews.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/LowLevelViews.cxx index a58e7bed5c47b..0f31cb8bfa58a 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/LowLevelViews.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/LowLevelViews.cxx @@ -817,38 +817,62 @@ static PyObject* ll_reshape(CPyCppyy::LowLevelView* self, PyObject* shape) //--------------------------------------------------------------------------- -static PyObject* ll_array(CPyCppyy::LowLevelView* self, PyObject* args, PyObject* /* kwds */) +static PyObject* ll_array(CPyCppyy::LowLevelView* self, PyObject* args, PyObject* kwds) { // Construct a numpy array from the lowlevelview (w/o copy if possible); this // uses the Python methods to avoid depending on numpy directly // Expect as most a dtype from the arguments; - static PyObject* ctmod = PyImport_ImportModule("numpy"); // ref-count kept - if (!ctmod) + static PyObject* npmod = PyImport_ImportModule("numpy"); // ref-count kept + if (!npmod) return nullptr; -// expect possible dtype from the arguments, otherwie take it from the type code - PyObject* dtype; - if (!args || PyTuple_GET_SIZE(args) != 1) { - PyObject* npdtype = PyObject_GetAttr(ctmod, CPyCppyy::PyStrings::gDType); - PyObject* typecode = ll_typecode(self, nullptr); - dtype = PyObject_CallFunctionObjArgs(npdtype, typecode, nullptr); - Py_DECREF(typecode); - Py_DECREF(npdtype); - } else { - dtype = PyTuple_GET_ITEM(args, 0); - Py_INCREF(dtype); + bool docopy = false; + if (kwds) { + PyObject* pycp = PyObject_GetItem(kwds, CPyCppyy::PyStrings::gCopy); + if (!pycp) { + PyErr_SetString(PyExc_TypeError, "__array__ only supports the \"copy\" keyword"); + return nullptr; + } + + docopy = PyObject_IsTrue(pycp); + Py_DECREF(pycp); } - if (!dtype) - return nullptr; + if (!docopy) { // view requested + // expect possible dtype from the arguments, otherwise take it from the type code + PyObject* dtype; + if (!args || PyTuple_GET_SIZE(args) != 1) { + PyObject* npdtype = PyObject_GetAttr(npmod, CPyCppyy::PyStrings::gDType); + PyObject* typecode = ll_typecode(self, nullptr); + dtype = PyObject_CallFunctionObjArgs(npdtype, typecode, nullptr); + Py_DECREF(typecode); + Py_DECREF(npdtype); + } else { + dtype = PyTuple_GET_ITEM(args, 0); + Py_INCREF(dtype); + } - PyObject* npfrombuf = PyObject_GetAttr(ctmod, CPyCppyy::PyStrings::gFromBuffer); - PyObject* view = PyObject_CallFunctionObjArgs(npfrombuf, (PyObject*)self, dtype, nullptr); - Py_DECREF(dtype); - Py_DECREF(npfrombuf); + if (!dtype) + return nullptr; + + PyObject* npfrombuf = PyObject_GetAttr(npmod, CPyCppyy::PyStrings::gFromBuffer); + PyObject* view = PyObject_CallFunctionObjArgs(npfrombuf, (PyObject*)self, dtype, nullptr); + Py_DECREF(dtype); + Py_DECREF(npfrombuf); + + return view; + + } else { // copy requested + PyObject* npcopy = PyObject_GetAttr(npmod, CPyCppyy::PyStrings::gCopy); + PyObject* newarr = PyObject_CallFunctionObjArgs(npcopy, (PyObject*)self, nullptr); + Py_DECREF(npcopy); - return view; + return newarr; + } + +// never get here + return nullptr; } diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/PyStrings.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/PyStrings.cxx index e918d44fc0d54..abbf16ece0049 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/PyStrings.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/PyStrings.cxx @@ -7,6 +7,7 @@ PyObject* CPyCppyy::PyStrings::gAssign = nullptr; PyObject* CPyCppyy::PyStrings::gBases = nullptr; PyObject* CPyCppyy::PyStrings::gBase = nullptr; +PyObject* CPyCppyy::PyStrings::gCopy = nullptr; PyObject* CPyCppyy::PyStrings::gCppBool = nullptr; PyObject* CPyCppyy::PyStrings::gCppName = nullptr; PyObject* CPyCppyy::PyStrings::gAnnotations = nullptr; @@ -87,6 +88,7 @@ bool CPyCppyy::CreatePyStrings() { CPPYY_INITIALIZE_STRING(gAssign, __assign__); CPPYY_INITIALIZE_STRING(gBases, __bases__); CPPYY_INITIALIZE_STRING(gBase, __base__); + CPPYY_INITIALIZE_STRING(gCopy, copy); #if PY_VERSION_HEX < 0x03000000 CPPYY_INITIALIZE_STRING(gCppBool, __cpp_nonzero__); #else @@ -169,6 +171,7 @@ PyObject* CPyCppyy::DestroyPyStrings() { // Remove all cached python strings. Py_DECREF(PyStrings::gBases); PyStrings::gBases = nullptr; Py_DECREF(PyStrings::gBase); PyStrings::gBase = nullptr; + Py_DECREF(PyStrings::gCopy); PyStrings::gCopy = nullptr; Py_DECREF(PyStrings::gCppBool); PyStrings::gCppBool = nullptr; Py_DECREF(PyStrings::gCppName); PyStrings::gCppName = nullptr; Py_DECREF(PyStrings::gAnnotations); PyStrings::gAnnotations = nullptr; diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/PyStrings.h b/bindings/pyroot/cppyy/CPyCppyy/src/PyStrings.h index 7012b89ce1620..55eaef58273a3 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/PyStrings.h +++ b/bindings/pyroot/cppyy/CPyCppyy/src/PyStrings.h @@ -10,6 +10,7 @@ namespace PyStrings { extern PyObject* gAssign; extern PyObject* gBases; extern PyObject* gBase; + extern PyObject* gCopy; extern PyObject* gCppBool; extern PyObject* gCppName; extern PyObject* gAnnotations; diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/Pythonize.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/Pythonize.cxx index 9666b77e85bfa..8559b2ebfe7ff 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/Pythonize.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/Pythonize.cxx @@ -528,13 +528,14 @@ PyObject* VectorData(PyObject* self, PyObject*) //--------------------------------------------------------------------------- -PyObject* VectorArray(PyObject* self, PyObject* /* args */, PyObject* /* kwargs */) +PyObject* VectorArray(PyObject* self, PyObject* args, PyObject* kwargs) { - // TODO: kwargs are ignored as the only known use is NumPy passing copy=False PyObject* pydata = VectorData(self, nullptr); - PyObject* view = PyObject_CallMethodNoArgs(pydata, PyStrings::gArray); + PyObject* arrcall = PyObject_GetAttr(pydata, PyStrings::gArray); + PyObject* newarr = PyObject_Call(arrcall, args, kwargs); + Py_DECREF(arrcall); Py_DECREF(pydata); - return view; + return newarr; } diff --git a/bindings/pyroot/cppyy/cppyy/test/test_regression.py b/bindings/pyroot/cppyy/cppyy/test/test_regression.py index cf0df68ceb899..6bd0baf387ba4 100644 --- a/bindings/pyroot/cppyy/cppyy/test/test_regression.py +++ b/bindings/pyroot/cppyy/cppyy/test/test_regression.py @@ -9,8 +9,12 @@ class TestREGRESSION: def setup_class(cls): import cppyy - def stringpager(text, cls=cls): - cls.helpout.append(text) + if sys.hexversion < 0x30d0000: + def stringpager(text, cls=cls): + cls.helpout.append(text) + else: + def stringpager(text, title='', cls=cls): + cls.helpout.append(text) import pydoc pydoc.pager = stringpager diff --git a/bindings/pyroot/cppyy/patches/CPyCppyy-Remove-implicit-conversion-of-any-ctypes-ptr.patch b/bindings/pyroot/cppyy/patches/CPyCppyy-Remove-implicit-conversion-of-any-ctypes-ptr.patch deleted file mode 100644 index 966978f7177a6..0000000000000 --- a/bindings/pyroot/cppyy/patches/CPyCppyy-Remove-implicit-conversion-of-any-ctypes-ptr.patch +++ /dev/null @@ -1,93 +0,0 @@ -From 1ec1d647f8a67c22184b4401dd63b672fbde1490 Mon Sep 17 00:00:00 2001 -From: Jonas Rembser -Date: Mon, 4 Nov 2024 21:29:30 +0100 -Subject: [PATCH] [CPyCppyy] Remove implicit conversion of any ctypes ptr type - to `void*` - -The `IsCTypesArrayOrPointer` gives false positives in Python 3.13, -resulting the void pointer converter to take the wrong code path and -crash. See: -https://github.com/wlav/cppyy/issues/272 - -This code path is used for implicit conversion from other `ctypes` -pointer types to `void*`, which is not strictly required. One can -always do an explicit cast: `ctypes.cast(my_ptr, ctypes.c_void_p )`. - -Given that this a niche feature that broke Python 3.13 support for -functions taking `void*`, which is quite common, it can be argued that -it's better to remove this implicit conversion. - -This commit fixes the following tests under Python 3.13: - -``` -roottest-python-basic-datatype -roottest-python-cpp-cpp -``` - -This reverts the following commit from upstream: -https://github.com/wlav/CPyCppyy/commit/80a0205f590394b88a583a296704356a9740606f ---- - .../pyroot/cppyy/CPyCppyy/src/Converters.cxx | 33 ------------------- - 1 file changed, 33 deletions(-) - -diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx -index 0a8bce3692..ea7e1abed2 100644 ---- a/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx -+++ b/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx -@@ -203,24 +203,6 @@ static bool IsPyCArgObject(PyObject* pyobject) - return Py_TYPE(pyobject) == pycarg_type; - } - --static bool IsCTypesArrayOrPointer(PyObject* pyobject) --{ -- static PyTypeObject* cstgdict_type = nullptr; -- if (!cstgdict_type) { -- // get any pointer type to initialize the extended dictionary type -- PyTypeObject* ct_int = GetCTypesType(ct_c_int); -- if (ct_int && ct_int->tp_dict) { -- cstgdict_type = Py_TYPE(ct_int->tp_dict); -- } -- } -- -- PyTypeObject* pytype = Py_TYPE(pyobject); -- if (pytype->tp_dict && Py_TYPE(pytype->tp_dict) == cstgdict_type) -- return true; -- return false; --} -- -- - //- helper to establish life lines ------------------------------------------- - static inline bool SetLifeLine(PyObject* holder, PyObject* target, intptr_t ref) - { -@@ -1506,16 +1488,6 @@ bool CPyCppyy::VoidArrayConverter::SetArg( - return true; - } - --// allow any other ctypes pointer type -- if (IsCTypesArrayOrPointer(pyobject)) { -- void** payload = (void**)((CPyCppyy_tagCDataObject*)pyobject)->b_ptr; -- if (payload) { -- para.fValue.fVoidp = *payload; -- para.fTypeCode = 'p'; -- return true; -- } -- } -- - // final try: attempt to get buffer - Py_ssize_t buflen = Utility::GetBuffer(pyobject, '*', 1, para.fValue.fVoidp, false); - -@@ -1628,11 +1600,6 @@ bool CPyCppyy::name##ArrayConverter::SetArg( \ - para.fValue.fVoidp = (void*)((CPyCppyy_tagCDataObject*)pyobject)->b_ptr;\ - para.fTypeCode = 'p'; \ - convOk = true; \ -- } else if (Py_TYPE(pyobject) == GetCTypesType(ct_c_void_p)) { \ -- /* special case: pass address of c_void_p buffer to return the address */\ -- para.fValue.fVoidp = (void*)((CPyCppyy_tagCDataObject*)pyobject)->b_ptr;\ -- para.fTypeCode = 'p'; \ -- convOk = true; \ - } else if (LowLevelView_Check(pyobject) && \ - ((LowLevelView*)pyobject)->fBufInfo.ndim == 2 && \ - strchr(((LowLevelView*)pyobject)->fBufInfo.format, code)) { \ --- -2.47.0 - diff --git a/bindings/pyroot/cppyy/sync-upstream b/bindings/pyroot/cppyy/sync-upstream index da495b5164b7c..c5d5c15e36851 100755 --- a/bindings/pyroot/cppyy/sync-upstream +++ b/bindings/pyroot/cppyy/sync-upstream @@ -45,7 +45,6 @@ git apply patches/CPyCppyy-Adapt-to-no-std-in-ROOT.patch git apply patches/CPyCppyy-Always-convert-returned-std-string.patch git apply patches/CPyCppyy-Disable-implicit-conversion-to-smart-ptr.patch git apply patches/CPyCppyy-TString_converter.patch -git apply patches/CPyCppyy-Remove-implicit-conversion-of-any-ctypes-ptr.patch git apply patches/CPyCppyy-Prevent-construction-of-agg-init-for-tuple.patch git apply patches/cppyy-No-CppyyLegacy-namespace.patch git apply patches/cppyy-Remove-Windows-workaround.patch