From 56cd516eb9f714bd8b1937cd2502604f235aca11 Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Wed, 19 Oct 2022 04:46:26 +0000 Subject: [PATCH 01/31] Illustrate bug in functional.h --- include/pybind11/functional.h | 2 ++ tests/test_callbacks.cpp | 15 +++++++++++++++ tests/test_callbacks.py | 5 +++++ 3 files changed, 22 insertions(+) diff --git a/include/pybind11/functional.h b/include/pybind11/functional.h index 102d1a938b..8a41592d2c 100644 --- a/include/pybind11/functional.h +++ b/include/pybind11/functional.h @@ -46,6 +46,7 @@ struct type_caster> { stateless (i.e. function pointer or lambda function without captured variables), in which case the roundtrip can be avoided. */ + #ifndef BAD if (auto cfunc = func.cpp_function()) { auto *cfunc_self = PyCFunction_GET_SELF(cfunc.ptr()); if (isinstance(cfunc_self)) { @@ -69,6 +70,7 @@ struct type_caster> { // Raising an fail exception here works to prevent the segfault, but only on gcc. // See PR #1413 for full details } + #endif // ensure GIL is held during functor destruction struct func_handle { diff --git a/tests/test_callbacks.cpp b/tests/test_callbacks.cpp index 92b8053de4..7b5e9a3409 100644 --- a/tests/test_callbacks.cpp +++ b/tests/test_callbacks.cpp @@ -16,6 +16,8 @@ int dummy_function(int i) { return i + 1; } +static PyMethodDef def; + TEST_SUBMODULE(callbacks, m) { // test_callbacks, test_function_signatures m.def("test_callback1", [](const py::object &func) { return func(); }); @@ -240,4 +242,17 @@ TEST_SUBMODULE(callbacks, m) { f(); } }); + + def.ml_name = "example_name"; + def.ml_doc = "Example doc"; + def.ml_meth = [](PyObject *, PyObject *) -> PyObject* { + auto result = py::cast(20); + return result.release().ptr(); + }; + def.ml_flags = METH_VARARGS; + + py::capsule rec_capsule(malloc(1), [](void *data) { free(data); }); + py::handle m_ptr = + PyCFunction_New(&def, rec_capsule.ptr()); + m.add_object("custom_function", m_ptr); } diff --git a/tests/test_callbacks.py b/tests/test_callbacks.py index 0b1047bbf2..5c1a109387 100644 --- a/tests/test_callbacks.py +++ b/tests/test_callbacks.py @@ -193,3 +193,8 @@ def test_callback_num_times(): if len(rates) > 1: print("Min Mean Max") print(f"{min(rates):6.3f} {sum(rates) / len(rates):6.3f} {max(rates):6.3f}") + +def test_custom_func(): + func = m.custom_function + assert func() == 20 + val = m.roundtrip(func) \ No newline at end of file From ba640fd5d08fb696bb09687afc4b41cee1a15978 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 19 Oct 2022 04:54:45 +0000 Subject: [PATCH 02/31] style: pre-commit fixes --- include/pybind11/functional.h | 20 ++++++++++---------- tests/test_callbacks.cpp | 5 ++--- tests/test_callbacks.py | 3 ++- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/include/pybind11/functional.h b/include/pybind11/functional.h index 8a41592d2c..d9be351095 100644 --- a/include/pybind11/functional.h +++ b/include/pybind11/functional.h @@ -38,15 +38,15 @@ struct type_caster> { auto func = reinterpret_borrow(src); - /* - When passing a C++ function as an argument to another C++ - function via Python, every function call would normally involve - a full C++ -> Python -> C++ roundtrip, which can be prohibitive. - Here, we try to at least detect the case where the function is - stateless (i.e. function pointer or lambda function without - captured variables), in which case the roundtrip can be avoided. - */ - #ifndef BAD +/* + When passing a C++ function as an argument to another C++ + function via Python, every function call would normally involve + a full C++ -> Python -> C++ roundtrip, which can be prohibitive. + Here, we try to at least detect the case where the function is + stateless (i.e. function pointer or lambda function without + captured variables), in which case the roundtrip can be avoided. + */ +#ifndef BAD if (auto cfunc = func.cpp_function()) { auto *cfunc_self = PyCFunction_GET_SELF(cfunc.ptr()); if (isinstance(cfunc_self)) { @@ -70,7 +70,7 @@ struct type_caster> { // Raising an fail exception here works to prevent the segfault, but only on gcc. // See PR #1413 for full details } - #endif +#endif // ensure GIL is held during functor destruction struct func_handle { diff --git a/tests/test_callbacks.cpp b/tests/test_callbacks.cpp index 7b5e9a3409..e52e7ef3c4 100644 --- a/tests/test_callbacks.cpp +++ b/tests/test_callbacks.cpp @@ -245,14 +245,13 @@ TEST_SUBMODULE(callbacks, m) { def.ml_name = "example_name"; def.ml_doc = "Example doc"; - def.ml_meth = [](PyObject *, PyObject *) -> PyObject* { + def.ml_meth = [](PyObject *, PyObject *) -> PyObject * { auto result = py::cast(20); return result.release().ptr(); }; def.ml_flags = METH_VARARGS; py::capsule rec_capsule(malloc(1), [](void *data) { free(data); }); - py::handle m_ptr = - PyCFunction_New(&def, rec_capsule.ptr()); + py::handle m_ptr = PyCFunction_New(&def, rec_capsule.ptr()); m.add_object("custom_function", m_ptr); } diff --git a/tests/test_callbacks.py b/tests/test_callbacks.py index 5c1a109387..2ee0f49290 100644 --- a/tests/test_callbacks.py +++ b/tests/test_callbacks.py @@ -194,7 +194,8 @@ def test_callback_num_times(): print("Min Mean Max") print(f"{min(rates):6.3f} {sum(rates) / len(rates):6.3f} {max(rates):6.3f}") + def test_custom_func(): func = m.custom_function assert func() == 20 - val = m.roundtrip(func) \ No newline at end of file + val = m.roundtrip(func) From e85ed9b135c2f25858ccdafefb65fc4e7a4e40f3 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Wed, 19 Oct 2022 13:38:15 -0400 Subject: [PATCH 03/31] Make functional casting more robust / add workaround --- include/pybind11/functional.h | 14 ++++++++++++-- include/pybind11/pytypes.h | 4 ++-- tests/test_callbacks.cpp | 5 ++++- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/include/pybind11/functional.h b/include/pybind11/functional.h index d9be351095..557e7e6b3b 100644 --- a/include/pybind11/functional.h +++ b/include/pybind11/functional.h @@ -49,9 +49,19 @@ struct type_caster> { #ifndef BAD if (auto cfunc = func.cpp_function()) { auto *cfunc_self = PyCFunction_GET_SELF(cfunc.ptr()); - if (isinstance(cfunc_self)) { + if (cfunc_self == nullptr) { + PyErr_Clear(); + } else if (isinstance(cfunc_self)) { auto c = reinterpret_borrow(cfunc_self); - auto *rec = (function_record *) c; + + function_record *rec = nullptr; + // THIS IS THE PROBLEM: UNSAFE NO WAY TO FIGURE OUT IF CAPSULE IS FOREIGN + // We would need to refactor to store a special string such as + // pybind11_function_record Doing so is almost certainly an ABI break though Best + // we can do without an ABI break is ignore named capsules + if (c.name() == nullptr) { + rec = static_cast(c); + } while (rec != nullptr) { if (rec->is_stateless diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 80b49ec397..4ec45060de 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -1842,8 +1842,8 @@ class capsule : public object { } } - capsule(const void *value, void (*destructor)(void *)) { - m_ptr = PyCapsule_New(const_cast(value), nullptr, [](PyObject *o) { + capsule(const void *value, void (*destructor)(void *), const char *name = nullptr) { + m_ptr = PyCapsule_New(const_cast(value), name, [](PyObject *o) { // guard if destructor called while err indicator is set error_scope error_guard; auto destructor = reinterpret_cast(PyCapsule_GetContext(o)); diff --git a/tests/test_callbacks.cpp b/tests/test_callbacks.cpp index e52e7ef3c4..e45a9be830 100644 --- a/tests/test_callbacks.cpp +++ b/tests/test_callbacks.cpp @@ -251,7 +251,10 @@ TEST_SUBMODULE(callbacks, m) { }; def.ml_flags = METH_VARARGS; - py::capsule rec_capsule(malloc(1), [](void *data) { free(data); }); + // rec_capsule with custom name + constexpr const char *rec_capsule_name = "CUSTOM_REC_CAPSULE"; + py::capsule rec_capsule( + malloc(1), [](void *data) { free(data); }, rec_capsule_name); py::handle m_ptr = PyCFunction_New(&def, rec_capsule.ptr()); m.add_object("custom_function", m_ptr); } From 7f3ff9bbcec982155890f491ac981370f06430c6 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Wed, 19 Oct 2022 14:14:04 -0400 Subject: [PATCH 04/31] Make function_record* casting even more robust --- include/pybind11/pybind11.h | 30 +++++++++++++++++++++--------- tests/test_callbacks.py | 2 +- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index e662236d01..e047a188a4 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -468,13 +468,20 @@ class cpp_function : public function { if (rec->sibling) { if (PyCFunction_Check(rec->sibling.ptr())) { auto *self = PyCFunction_GET_SELF(rec->sibling.ptr()); - capsule rec_capsule = isinstance(self) ? reinterpret_borrow(self) - : capsule(self); - chain = (detail::function_record *) rec_capsule; - /* Never append a method to an overload chain of a parent class; - instead, hide the parent's overloads in this case */ - if (!chain->scope.is(rec->scope)) { + if (!isinstance(self)) { chain = nullptr; + } else { + auto rec_capsule = reinterpret_borrow(self); + if (rec_capsule.name() == nullptr) { + chain = static_cast(rec_capsule); + /* Never append a method to an overload chain of a parent class; + instead, hide the parent's overloads in this case */ + if (!chain->scope.is(rec->scope)) { + chain = nullptr; + } + } else { + chain = nullptr; + } } } // Don't trigger for things like the default __init__, which are wrapper_descriptors @@ -1871,9 +1878,14 @@ class class_ : public detail::generic_type { static detail::function_record *get_function_record(handle h) { h = detail::get_function(h); - return h ? (detail::function_record *) reinterpret_borrow( - PyCFunction_GET_SELF(h.ptr())) - : nullptr; + if (!h) { + return nullptr; + } + auto cap = reinterpret_borrow(PyCFunction_GET_SELF(h.ptr())); + if (cap.name() != nullptr) { + return nullptr; + } + return static_cast(cap); } }; diff --git a/tests/test_callbacks.py b/tests/test_callbacks.py index 2ee0f49290..6625085dcb 100644 --- a/tests/test_callbacks.py +++ b/tests/test_callbacks.py @@ -198,4 +198,4 @@ def test_callback_num_times(): def test_custom_func(): func = m.custom_function assert func() == 20 - val = m.roundtrip(func) + _ = m.roundtrip(func) From fbdf4aa6cd31f5952becdb137d3559a636183a62 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Wed, 19 Oct 2022 14:16:47 -0400 Subject: [PATCH 05/31] See if this fixes PyPy issue --- tests/test_callbacks.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_callbacks.py b/tests/test_callbacks.py index 6625085dcb..c30f795523 100644 --- a/tests/test_callbacks.py +++ b/tests/test_callbacks.py @@ -131,10 +131,10 @@ def test_movable_object(): assert m.callback_with_movable(lambda _: None) is True -@pytest.mark.skipif( - "env.PYPY", - reason="PyPy segfaults on here. See discussion on #1413.", -) +# @pytest.mark.skipif( +# "env.PYPY", +# reason="PyPy segfaults on here. See discussion on #1413.", +# ) def test_python_builtins(): """Test if python builtins like sum() can be used as callbacks""" assert m.test_sum_builtin(sum, [1, 2, 3]) == 6 From 9deba241fc100b69f5f590198a1041ecc34afd26 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Wed, 19 Oct 2022 14:23:25 -0400 Subject: [PATCH 06/31] It still fails on PyPy sadly --- tests/test_callbacks.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_callbacks.py b/tests/test_callbacks.py index c30f795523..6625085dcb 100644 --- a/tests/test_callbacks.py +++ b/tests/test_callbacks.py @@ -131,10 +131,10 @@ def test_movable_object(): assert m.callback_with_movable(lambda _: None) is True -# @pytest.mark.skipif( -# "env.PYPY", -# reason="PyPy segfaults on here. See discussion on #1413.", -# ) +@pytest.mark.skipif( + "env.PYPY", + reason="PyPy segfaults on here. See discussion on #1413.", +) def test_python_builtins(): """Test if python builtins like sum() can be used as callbacks""" assert m.test_sum_builtin(sum, [1, 2, 3]) == 6 From 4c6c768898f70e2b1e2c9924b05b074d6dc2962d Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Thu, 20 Oct 2022 09:53:14 -0400 Subject: [PATCH 07/31] Do not make new CTOR just yet --- include/pybind11/pytypes.h | 4 ++-- tests/test_callbacks.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 4ec45060de..80b49ec397 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -1842,8 +1842,8 @@ class capsule : public object { } } - capsule(const void *value, void (*destructor)(void *), const char *name = nullptr) { - m_ptr = PyCapsule_New(const_cast(value), name, [](PyObject *o) { + capsule(const void *value, void (*destructor)(void *)) { + m_ptr = PyCapsule_New(const_cast(value), nullptr, [](PyObject *o) { // guard if destructor called while err indicator is set error_scope error_guard; auto destructor = reinterpret_cast(PyCapsule_GetContext(o)); diff --git a/tests/test_callbacks.cpp b/tests/test_callbacks.cpp index e45a9be830..f4199b7851 100644 --- a/tests/test_callbacks.cpp +++ b/tests/test_callbacks.cpp @@ -253,8 +253,8 @@ TEST_SUBMODULE(callbacks, m) { // rec_capsule with custom name constexpr const char *rec_capsule_name = "CUSTOM_REC_CAPSULE"; - py::capsule rec_capsule( - malloc(1), [](void *data) { free(data); }, rec_capsule_name); + py::capsule rec_capsule(malloc(1), [](void *data) { free(data); }); + rec_capsule.set_name(rec_capsule_name); py::handle m_ptr = PyCFunction_New(&def, rec_capsule.ptr()); m.add_object("custom_function", m_ptr); } From c36f3a8906542902cad988f440c16dcc51518256 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Thu, 20 Oct 2022 10:19:02 -0400 Subject: [PATCH 08/31] Fix test --- tests/test_callbacks.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_callbacks.cpp b/tests/test_callbacks.cpp index f4199b7851..9c21a37b83 100644 --- a/tests/test_callbacks.cpp +++ b/tests/test_callbacks.cpp @@ -253,7 +253,7 @@ TEST_SUBMODULE(callbacks, m) { // rec_capsule with custom name constexpr const char *rec_capsule_name = "CUSTOM_REC_CAPSULE"; - py::capsule rec_capsule(malloc(1), [](void *data) { free(data); }); + py::capsule rec_capsule(std::malloc(1), [](void *data) { std::free(data); }); rec_capsule.set_name(rec_capsule_name); py::handle m_ptr = PyCFunction_New(&def, rec_capsule.ptr()); m.add_object("custom_function", m_ptr); From 1aaf98ba1d18765cc023b4ac81cec351b198c566 Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Thu, 20 Oct 2022 14:53:07 +0000 Subject: [PATCH 09/31] Add name to ensure correctness --- include/pybind11/functional.h | 2 +- include/pybind11/pybind11.h | 13 ++++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/include/pybind11/functional.h b/include/pybind11/functional.h index 557e7e6b3b..8cb8defc00 100644 --- a/include/pybind11/functional.h +++ b/include/pybind11/functional.h @@ -59,7 +59,7 @@ struct type_caster> { // We would need to refactor to store a special string such as // pybind11_function_record Doing so is almost certainly an ABI break though Best // we can do without an ABI break is ignore named capsules - if (c.name() == nullptr) { + if (c.name() == function_capsule_name()) { rec = static_cast(c); } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index e047a188a4..659cb01409 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -69,6 +69,12 @@ inline bool apply_exception_translators(std::forward_list & return false; } +// Need to use a wrapper function to ensure 1 address +inline const char* function_capsule_name() { + static const char* name = "pybind11_function_capsule"; + return name; +} + #if defined(_MSC_VER) # define PYBIND11_COMPAT_STRDUP _strdup #else @@ -472,7 +478,7 @@ class cpp_function : public function { chain = nullptr; } else { auto rec_capsule = reinterpret_borrow(self); - if (rec_capsule.name() == nullptr) { + if (rec_capsule.name() == detail::function_capsule_name()) { chain = static_cast(rec_capsule); /* Never append a method to an overload chain of a parent class; instead, hide the parent's overloads in this case */ @@ -503,6 +509,7 @@ class cpp_function : public function { capsule rec_capsule(unique_rec.release(), [](void *ptr) { destruct((detail::function_record *) ptr); }); + rec_capsule.set_name(detail::function_capsule_name()); guarded_strdup.release(); object scope_module; @@ -670,7 +677,7 @@ class cpp_function : public function { using namespace detail; /* Iterator over the list of potentially admissible overloads */ - const function_record *overloads = (function_record *) PyCapsule_GetPointer(self, nullptr), + const function_record *overloads = (function_record *) PyCapsule_GetPointer(self, function_capsule_name()), *it = overloads; /* Need to know how many arguments + keyword arguments there are to pick the right @@ -1882,7 +1889,7 @@ class class_ : public detail::generic_type { return nullptr; } auto cap = reinterpret_borrow(PyCFunction_GET_SELF(h.ptr())); - if (cap.name() != nullptr) { + if (cap.name() != detail::function_capsule_name()) { return nullptr; } return static_cast(cap); From 871273c97a7ed9a89c200ac4065a322cacb03562 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 20 Oct 2022 14:59:12 +0000 Subject: [PATCH 10/31] style: pre-commit fixes --- include/pybind11/pybind11.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 659cb01409..aae76235ae 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -70,8 +70,8 @@ inline bool apply_exception_translators(std::forward_list & } // Need to use a wrapper function to ensure 1 address -inline const char* function_capsule_name() { - static const char* name = "pybind11_function_capsule"; +inline const char *function_capsule_name() { + static const char *name = "pybind11_function_capsule"; return name; } @@ -677,8 +677,9 @@ class cpp_function : public function { using namespace detail; /* Iterator over the list of potentially admissible overloads */ - const function_record *overloads = (function_record *) PyCapsule_GetPointer(self, function_capsule_name()), - *it = overloads; + const function_record *overloads + = (function_record *) PyCapsule_GetPointer(self, function_capsule_name()), + *it = overloads; /* Need to know how many arguments + keyword arguments there are to pick the right overload */ From 4962a5d42ac8bb088a2fa63d75edcee99def59b8 Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Thu, 20 Oct 2022 15:41:36 +0000 Subject: [PATCH 11/31] Clean up tests + remove ifdef guards --- include/pybind11/functional.h | 18 ++++++++---------- tests/test_callbacks.cpp | 18 ++++++++++++++---- tests/test_callbacks.py | 8 +++++--- 3 files changed, 27 insertions(+), 17 deletions(-) diff --git a/include/pybind11/functional.h b/include/pybind11/functional.h index 8cb8defc00..47478c404b 100644 --- a/include/pybind11/functional.h +++ b/include/pybind11/functional.h @@ -38,15 +38,14 @@ struct type_caster> { auto func = reinterpret_borrow(src); -/* - When passing a C++ function as an argument to another C++ - function via Python, every function call would normally involve - a full C++ -> Python -> C++ roundtrip, which can be prohibitive. - Here, we try to at least detect the case where the function is - stateless (i.e. function pointer or lambda function without - captured variables), in which case the roundtrip can be avoided. - */ -#ifndef BAD + /* + When passing a C++ function as an argument to another C++ + function via Python, every function call would normally involve + a full C++ -> Python -> C++ roundtrip, which can be prohibitive. + Here, we try to at least detect the case where the function is + stateless (i.e. function pointer or lambda function without + captured variables), in which case the roundtrip can be avoided. + */ if (auto cfunc = func.cpp_function()) { auto *cfunc_self = PyCFunction_GET_SELF(cfunc.ptr()); if (cfunc_self == nullptr) { @@ -80,7 +79,6 @@ struct type_caster> { // Raising an fail exception here works to prevent the segfault, but only on gcc. // See PR #1413 for full details } -#endif // ensure GIL is held during functor destruction struct func_handle { diff --git a/tests/test_callbacks.cpp b/tests/test_callbacks.cpp index 9c21a37b83..d10bfcca76 100644 --- a/tests/test_callbacks.cpp +++ b/tests/test_callbacks.cpp @@ -245,8 +245,15 @@ TEST_SUBMODULE(callbacks, m) { def.ml_name = "example_name"; def.ml_doc = "Example doc"; - def.ml_meth = [](PyObject *, PyObject *) -> PyObject * { - auto result = py::cast(20); + def.ml_meth = [](PyObject *, PyObject *args) -> PyObject * { + if (PyTuple_Size(args) != 1) { + throw std::runtime_error("Invalid number of arguments for example_name"); + } + PyObject *first = PyTuple_GetItem(args, 0); + if (!PyLong_Check(first)) { + throw std::runtime_error("Invalid argument to example_name"); + } + auto result = py::cast(PyLong_AsLong(first) * 9); return result.release().ptr(); }; def.ml_flags = METH_VARARGS; @@ -255,6 +262,9 @@ TEST_SUBMODULE(callbacks, m) { constexpr const char *rec_capsule_name = "CUSTOM_REC_CAPSULE"; py::capsule rec_capsule(std::malloc(1), [](void *data) { std::free(data); }); rec_capsule.set_name(rec_capsule_name); - py::handle m_ptr = PyCFunction_New(&def, rec_capsule.ptr()); - m.add_object("custom_function", m_ptr); + m.add_object("custom_function", PyCFunction_New(&def, rec_capsule.ptr())); + + // rec_capsule with nullptr name + py::capsule rec_capsule2(std::malloc(1), [](void *data) { std::free(data); }); + m.add_object("custom_function2", PyCFunction_New(&def, rec_capsule2.ptr())); } diff --git a/tests/test_callbacks.py b/tests/test_callbacks.py index 6625085dcb..a942d72ae1 100644 --- a/tests/test_callbacks.py +++ b/tests/test_callbacks.py @@ -196,6 +196,8 @@ def test_callback_num_times(): def test_custom_func(): - func = m.custom_function - assert func() == 20 - _ = m.roundtrip(func) + assert m.custom_function(4) == 36 + assert m.roundtrip(m.custom_function)(4) == 36 + + assert m.custom_function2(3) == 27 + assert m.roundtrip(m.custom_function2)(3) == 27 From 3a8f0f32569829e9646d6c15fd0d17141172f214 Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Thu, 20 Oct 2022 22:09:01 +0000 Subject: [PATCH 12/31] Add comments --- include/pybind11/functional.h | 4 +++- include/pybind11/pybind11.h | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/include/pybind11/functional.h b/include/pybind11/functional.h index 47478c404b..2249f4e9c5 100644 --- a/include/pybind11/functional.h +++ b/include/pybind11/functional.h @@ -57,7 +57,9 @@ struct type_caster> { // THIS IS THE PROBLEM: UNSAFE NO WAY TO FIGURE OUT IF CAPSULE IS FOREIGN // We would need to refactor to store a special string such as // pybind11_function_record Doing so is almost certainly an ABI break though Best - // we can do without an ABI break is ignore named capsules + // we can do without an ABI break is ignore capsules with the wrong name + // + // Note that we compare pointers, not values to ensure each extension is unique if (c.name() == function_capsule_name()) { rec = static_cast(c); } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index aae76235ae..79eb2609ed 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -478,6 +478,7 @@ class cpp_function : public function { chain = nullptr; } else { auto rec_capsule = reinterpret_borrow(self); + // Compare the pointers, not the values to ensure that each extension is unique if (rec_capsule.name() == detail::function_capsule_name()) { chain = static_cast(rec_capsule); /* Never append a method to an overload chain of a parent class; @@ -1890,6 +1891,8 @@ class class_ : public detail::generic_type { return nullptr; } auto cap = reinterpret_borrow(PyCFunction_GET_SELF(h.ptr())); + + // Compare the pointers, not the values to ensure that each extension is unique if (cap.name() != detail::function_capsule_name()) { return nullptr; } From 8dce8b34f8536286c982b833f8139514e1f04e9f Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Fri, 21 Oct 2022 11:19:57 -0400 Subject: [PATCH 13/31] Improve comments, error handling, and safety --- include/pybind11/functional.h | 6 +----- include/pybind11/pybind11.h | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/include/pybind11/functional.h b/include/pybind11/functional.h index 2249f4e9c5..0dfa78f675 100644 --- a/include/pybind11/functional.h +++ b/include/pybind11/functional.h @@ -54,11 +54,7 @@ struct type_caster> { auto c = reinterpret_borrow(cfunc_self); function_record *rec = nullptr; - // THIS IS THE PROBLEM: UNSAFE NO WAY TO FIGURE OUT IF CAPSULE IS FOREIGN - // We would need to refactor to store a special string such as - // pybind11_function_record Doing so is almost certainly an ABI break though Best - // we can do without an ABI break is ignore capsules with the wrong name - // + // Check that we can safely reinterpret the capsule into a function_record // Note that we compare pointers, not values to ensure each extension is unique if (c.name() == function_capsule_name()) { rec = static_cast(c); diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 79eb2609ed..6d23697fae 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -678,9 +678,10 @@ class cpp_function : public function { using namespace detail; /* Iterator over the list of potentially admissible overloads */ - const function_record *overloads - = (function_record *) PyCapsule_GetPointer(self, function_capsule_name()), - *it = overloads; + const function_record *overloads = reinterpret_cast( + PyCapsule_GetPointer(self, function_capsule_name())), + *it = overloads; + assert(overloads != nullptr); /* Need to know how many arguments + keyword arguments there are to pick the right overload */ @@ -1890,8 +1891,15 @@ class class_ : public detail::generic_type { if (!h) { return nullptr; } - auto cap = reinterpret_borrow(PyCFunction_GET_SELF(h.ptr())); + handle self = PyCFunction_GET_SELF(h.ptr()); + if (!self) { + throw error_already_set(); + } + if (!isinstance(self)) { + return nullptr; + } + auto cap = reinterpret_borrow(self); // Compare the pointers, not the values to ensure that each extension is unique if (cap.name() != detail::function_capsule_name()) { return nullptr; From eca0cbf7f0a528d945a0823bb47e8e9afc5a9e30 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Fri, 21 Oct 2022 12:21:16 -0400 Subject: [PATCH 14/31] Fix compile error --- include/pybind11/pybind11.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 6d23697fae..a4d36edf86 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1892,14 +1892,14 @@ class class_ : public detail::generic_type { return nullptr; } - handle self = PyCFunction_GET_SELF(h.ptr()); - if (!self) { + handle func_self = PyCFunction_GET_SELF(h.ptr()); + if (!func_self) { throw error_already_set(); } - if (!isinstance(self)) { + if (!isinstance(func_self)) { return nullptr; } - auto cap = reinterpret_borrow(self); + auto cap = reinterpret_borrow(func_self); // Compare the pointers, not the values to ensure that each extension is unique if (cap.name() != detail::function_capsule_name()) { return nullptr; From a0d5be39a365540992dd6ad11c00c4efb0ee415b Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Fri, 21 Oct 2022 16:22:16 +0000 Subject: [PATCH 15/31] Fix magic logic --- include/pybind11/pybind11.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index a4d36edf86..88053e7269 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -71,7 +71,7 @@ inline bool apply_exception_translators(std::forward_list & // Need to use a wrapper function to ensure 1 address inline const char *function_capsule_name() { - static const char *name = "pybind11_function_capsule"; + static char name[] = "pybind11_function_capsule"; return name; } From 184534e140696c12073eea17627cfae316105dd1 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Fri, 21 Oct 2022 12:36:15 -0400 Subject: [PATCH 16/31] Extract helper function --- include/pybind11/functional.h | 3 +-- include/pybind11/pybind11.h | 11 +++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/include/pybind11/functional.h b/include/pybind11/functional.h index 0dfa78f675..72a88e6170 100644 --- a/include/pybind11/functional.h +++ b/include/pybind11/functional.h @@ -55,8 +55,7 @@ struct type_caster> { function_record *rec = nullptr; // Check that we can safely reinterpret the capsule into a function_record - // Note that we compare pointers, not values to ensure each extension is unique - if (c.name() == function_capsule_name()) { + if (detail::is_function_record_capsule(c)) { rec = static_cast(c); } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 88053e7269..548b46d4b5 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -75,6 +75,11 @@ inline const char *function_capsule_name() { return name; } +inline bool is_function_record_capsule(capsule &cap) { + // Compare the pointers, not the values to ensure that each extension is unique + return cap.name() == function_capsule_name(); +} + #if defined(_MSC_VER) # define PYBIND11_COMPAT_STRDUP _strdup #else @@ -478,8 +483,7 @@ class cpp_function : public function { chain = nullptr; } else { auto rec_capsule = reinterpret_borrow(self); - // Compare the pointers, not the values to ensure that each extension is unique - if (rec_capsule.name() == detail::function_capsule_name()) { + if (detail::is_function_record_capsule(rec_capsule)) { chain = static_cast(rec_capsule); /* Never append a method to an overload chain of a parent class; instead, hide the parent's overloads in this case */ @@ -1900,8 +1904,7 @@ class class_ : public detail::generic_type { return nullptr; } auto cap = reinterpret_borrow(func_self); - // Compare the pointers, not the values to ensure that each extension is unique - if (cap.name() != detail::function_capsule_name()) { + if (!detail::is_function_record_capsule(cap)) { return nullptr; } return static_cast(cap); From f92cb03f22807c5ae3d1145094996ca77cf79705 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Fri, 21 Oct 2022 12:41:57 -0400 Subject: [PATCH 17/31] Fix func signature --- include/pybind11/pybind11.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 548b46d4b5..65f1713da7 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -75,7 +75,7 @@ inline const char *function_capsule_name() { return name; } -inline bool is_function_record_capsule(capsule &cap) { +inline bool is_function_record_capsule(const capsule &cap) { // Compare the pointers, not the values to ensure that each extension is unique return cap.name() == function_capsule_name(); } From 17b1f9139c950fda2cb624827d8b2eee8a9df7ee Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Fri, 21 Oct 2022 16:52:55 +0000 Subject: [PATCH 18/31] move to local internals --- include/pybind11/detail/internals.h | 17 ++++++++++++++--- include/pybind11/pybind11.h | 12 +++--------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 7de7794344..2d314bb144 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -34,7 +34,7 @@ /// further ABI-incompatible changes may be made before the ABI is officially /// changed to the new version. #ifndef PYBIND11_INTERNALS_VERSION -# define PYBIND11_INTERNALS_VERSION 4 +# define PYBIND11_INTERNALS_VERSION 5 #endif PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) @@ -491,7 +491,7 @@ PYBIND11_NOINLINE internals &get_internals() { struct local_internals { type_map registered_types_cpp; std::forward_list registered_exception_translators; -#if defined(WITH_THREAD) && PYBIND11_INTERNALS_VERSION == 4 +#if defined(WITH_THREAD) && PYBIND11_INTERNALS_VERSION >= 4 // For ABI compatibility, we can't store the loader_life_support TLS key in // the `internals` struct directly. Instead, we store it in `shared_data` and @@ -523,7 +523,10 @@ struct local_internals { loader_life_support_tls_key = static_cast(ptr)->loader_life_support_tls_key; } -#endif // defined(WITH_THREAD) && PYBIND11_INTERNALS_VERSION == 4 +#endif // defined(WITH_THREAD) && PYBIND11_INTERNALS_VERSION >= 4 +#if PYBIND11_INTERNALS_VERSION == 5 + const char* function_capsule_name = strdup("pybind11_function_capsule"); +#endif }; /// Works like `get_internals`, but for things which are locally registered. @@ -537,6 +540,14 @@ inline local_internals &get_local_internals() { return *locals; } +inline const char* get_function_capsule_name() { +#if PYBIND11_INTERNALS_VERSION == 5 + return get_local_internals().function_capsule_name; +#else + return nullptr; +#endif +} + /// Constructs a std::string with the given arguments, stores it in `internals`, and returns its /// `c_str()`. Such strings objects have a long storage duration -- the internal strings are only /// cleared when the program exits or after interpreter shutdown (when embedding), and so are diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 65f1713da7..9f45dcca5b 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -69,15 +69,9 @@ inline bool apply_exception_translators(std::forward_list & return false; } -// Need to use a wrapper function to ensure 1 address -inline const char *function_capsule_name() { - static char name[] = "pybind11_function_capsule"; - return name; -} - inline bool is_function_record_capsule(const capsule &cap) { // Compare the pointers, not the values to ensure that each extension is unique - return cap.name() == function_capsule_name(); + return cap.name() == get_function_capsule_name(); } #if defined(_MSC_VER) @@ -514,7 +508,7 @@ class cpp_function : public function { capsule rec_capsule(unique_rec.release(), [](void *ptr) { destruct((detail::function_record *) ptr); }); - rec_capsule.set_name(detail::function_capsule_name()); + rec_capsule.set_name(detail::get_function_capsule_name()); guarded_strdup.release(); object scope_module; @@ -683,7 +677,7 @@ class cpp_function : public function { /* Iterator over the list of potentially admissible overloads */ const function_record *overloads = reinterpret_cast( - PyCapsule_GetPointer(self, function_capsule_name())), + PyCapsule_GetPointer(self, get_function_capsule_name())), *it = overloads; assert(overloads != nullptr); From 3e04c076384a2ff5eac0e9fc3f15ae29e3590649 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 21 Oct 2022 16:59:09 +0000 Subject: [PATCH 19/31] style: pre-commit fixes --- include/pybind11/detail/internals.h | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 2d314bb144..b98a23fbea 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -34,7 +34,11 @@ /// further ABI-incompatible changes may be made before the ABI is officially /// changed to the new version. #ifndef PYBIND11_INTERNALS_VERSION -# define PYBIND11_INTERNALS_VERSION 5 +# define PYBIND11_INTERNALS_VERSION 4 +#endif + +#ifndef PYBIND11_LOCAL_INTERNALS_VERSION +# define PYBIND11_LOCAL_INTERNALS_VERSION 1 #endif PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) @@ -491,7 +495,7 @@ PYBIND11_NOINLINE internals &get_internals() { struct local_internals { type_map registered_types_cpp; std::forward_list registered_exception_translators; -#if defined(WITH_THREAD) && PYBIND11_INTERNALS_VERSION >= 4 +#if defined(WITH_THREAD) && PYBIND11_INTERNALS_VERSION == 4 // For ABI compatibility, we can't store the loader_life_support TLS key in // the `internals` struct directly. Instead, we store it in `shared_data` and @@ -523,9 +527,10 @@ struct local_internals { loader_life_support_tls_key = static_cast(ptr)->loader_life_support_tls_key; } -#endif // defined(WITH_THREAD) && PYBIND11_INTERNALS_VERSION >= 4 -#if PYBIND11_INTERNALS_VERSION == 5 - const char* function_capsule_name = strdup("pybind11_function_capsule"); +#endif // defined(WITH_THREAD) && PYBIND11_INTERNALS_VERSION == 4 + +#if PYBIND11_LOCAL_INTERNALS_VERSION >= 1 + const char *function_capsule_name = strdup("pybind11_function_capsule"); #endif }; @@ -540,8 +545,8 @@ inline local_internals &get_local_internals() { return *locals; } -inline const char* get_function_capsule_name() { -#if PYBIND11_INTERNALS_VERSION == 5 +inline const char *get_function_capsule_name() { +#if PYBIND11_LOCAL_INTERNALS_VERSION >= 1 return get_local_internals().function_capsule_name; #else return nullptr; From 13b5fea0de20a900e4c3a5641581696693481514 Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Fri, 21 Oct 2022 17:14:56 +0000 Subject: [PATCH 20/31] Switch to simpler design --- include/pybind11/detail/internals.h | 16 ---------------- include/pybind11/pybind11.h | 11 ++++++++--- 2 files changed, 8 insertions(+), 19 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index b98a23fbea..7de7794344 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -37,10 +37,6 @@ # define PYBIND11_INTERNALS_VERSION 4 #endif -#ifndef PYBIND11_LOCAL_INTERNALS_VERSION -# define PYBIND11_LOCAL_INTERNALS_VERSION 1 -#endif - PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) using ExceptionTranslator = void (*)(std::exception_ptr); @@ -528,10 +524,6 @@ struct local_internals { = static_cast(ptr)->loader_life_support_tls_key; } #endif // defined(WITH_THREAD) && PYBIND11_INTERNALS_VERSION == 4 - -#if PYBIND11_LOCAL_INTERNALS_VERSION >= 1 - const char *function_capsule_name = strdup("pybind11_function_capsule"); -#endif }; /// Works like `get_internals`, but for things which are locally registered. @@ -545,14 +537,6 @@ inline local_internals &get_local_internals() { return *locals; } -inline const char *get_function_capsule_name() { -#if PYBIND11_LOCAL_INTERNALS_VERSION >= 1 - return get_local_internals().function_capsule_name; -#else - return nullptr; -#endif -} - /// Constructs a std::string with the given arguments, stores it in `internals`, and returns its /// `c_str()`. Such strings objects have a long storage duration -- the internal strings are only /// cleared when the program exits or after interpreter shutdown (when embedding), and so are diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 9f45dcca5b..960ab28745 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -69,9 +69,14 @@ inline bool apply_exception_translators(std::forward_list & return false; } +constexpr const char* function_capsule_name = "pybind11_funciton_capsule_v1"; + inline bool is_function_record_capsule(const capsule &cap) { // Compare the pointers, not the values to ensure that each extension is unique - return cap.name() == get_function_capsule_name(); + if (cap.name() == nullptr) { + return false; + } + return (std::strcmp(cap.name(), function_capsule_name) == 0); } #if defined(_MSC_VER) @@ -508,7 +513,7 @@ class cpp_function : public function { capsule rec_capsule(unique_rec.release(), [](void *ptr) { destruct((detail::function_record *) ptr); }); - rec_capsule.set_name(detail::get_function_capsule_name()); + rec_capsule.set_name(detail::function_capsule_name); guarded_strdup.release(); object scope_module; @@ -677,7 +682,7 @@ class cpp_function : public function { /* Iterator over the list of potentially admissible overloads */ const function_record *overloads = reinterpret_cast( - PyCapsule_GetPointer(self, get_function_capsule_name())), + PyCapsule_GetPointer(self, function_capsule_name)), *it = overloads; assert(overloads != nullptr); From 71f97d17c204fce376085033c5114dd9f3027d4e Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 21 Oct 2022 17:20:54 +0000 Subject: [PATCH 21/31] style: pre-commit fixes --- include/pybind11/pybind11.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 960ab28745..d53a419d54 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -69,7 +69,7 @@ inline bool apply_exception_translators(std::forward_list & return false; } -constexpr const char* function_capsule_name = "pybind11_funciton_capsule_v1"; +constexpr const char *function_capsule_name = "pybind11_funciton_capsule_v1"; inline bool is_function_record_capsule(const capsule &cap) { // Compare the pointers, not the values to ensure that each extension is unique From 5c07f91c0366e4363e59704b389c8737f94d1994 Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Fri, 21 Oct 2022 18:34:50 +0000 Subject: [PATCH 22/31] Move to function_record --- include/pybind11/attr.h | 5 +++++ include/pybind11/pybind11.h | 9 +++------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index db7cd8efff..e8585c53e2 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -185,7 +185,12 @@ struct argument_record { /// Internal data structure which holds metadata about a bound function (signature, overloads, /// etc.) +// Note that function_capsule_name must be updated whenever this struct is changed struct function_record { + + // Update the following whenever this struct is changed + static constexpr const char* function_capsule_name = "pybind11_function_capsule_v1"; + function_record() : is_constructor(false), is_new_style_constructor(false), is_stateless(false), is_operator(false), is_method(false), has_args(false), has_kwargs(false), diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index d53a419d54..664f81e452 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -69,14 +69,11 @@ inline bool apply_exception_translators(std::forward_list & return false; } -constexpr const char *function_capsule_name = "pybind11_funciton_capsule_v1"; - inline bool is_function_record_capsule(const capsule &cap) { - // Compare the pointers, not the values to ensure that each extension is unique if (cap.name() == nullptr) { return false; } - return (std::strcmp(cap.name(), function_capsule_name) == 0); + return (std::strcmp(cap.name(), function_record::function_capsule_name) == 0); } #if defined(_MSC_VER) @@ -513,7 +510,7 @@ class cpp_function : public function { capsule rec_capsule(unique_rec.release(), [](void *ptr) { destruct((detail::function_record *) ptr); }); - rec_capsule.set_name(detail::function_capsule_name); + rec_capsule.set_name(detail::function_record::function_capsule_name); guarded_strdup.release(); object scope_module; @@ -682,7 +679,7 @@ class cpp_function : public function { /* Iterator over the list of potentially admissible overloads */ const function_record *overloads = reinterpret_cast( - PyCapsule_GetPointer(self, function_capsule_name)), + PyCapsule_GetPointer(self, function_record::function_capsule_name)), *it = overloads; assert(overloads != nullptr); From aae5968791a1f0cc44c2d2899b0839e40c65630b Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 21 Oct 2022 18:41:24 +0000 Subject: [PATCH 23/31] style: pre-commit fixes --- include/pybind11/attr.h | 2 +- include/pybind11/pybind11.h | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index e8585c53e2..a1618a44d1 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -189,7 +189,7 @@ struct argument_record { struct function_record { // Update the following whenever this struct is changed - static constexpr const char* function_capsule_name = "pybind11_function_capsule_v1"; + static constexpr const char *function_capsule_name = "pybind11_function_capsule_v1"; function_record() : is_constructor(false), is_new_style_constructor(false), is_stateless(false), diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 664f81e452..6096601549 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -678,9 +678,10 @@ class cpp_function : public function { using namespace detail; /* Iterator over the list of potentially admissible overloads */ - const function_record *overloads = reinterpret_cast( - PyCapsule_GetPointer(self, function_record::function_capsule_name)), - *it = overloads; + const function_record *overloads + = reinterpret_cast( + PyCapsule_GetPointer(self, function_record::function_capsule_name)), + *it = overloads; assert(overloads != nullptr); /* Need to know how many arguments + keyword arguments there are to pick the right From 8edaae713243433318980c4d552d4e4f095f7cc9 Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Sat, 22 Oct 2022 21:58:58 +0000 Subject: [PATCH 24/31] Switch to internals, update tests and docs --- include/pybind11/attr.h | 4 --- include/pybind11/detail/internals.h | 24 +++++++++++++++ include/pybind11/pybind11.h | 16 +++------- tests/test_callbacks.cpp | 45 +++++++++++++++++------------ 4 files changed, 54 insertions(+), 35 deletions(-) diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index a1618a44d1..2284cd8a3d 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -185,12 +185,8 @@ struct argument_record { /// Internal data structure which holds metadata about a bound function (signature, overloads, /// etc.) -// Note that function_capsule_name must be updated whenever this struct is changed struct function_record { - // Update the following whenever this struct is changed - static constexpr const char *function_capsule_name = "pybind11_function_capsule_v1"; - function_record() : is_constructor(false), is_new_style_constructor(false), is_stateless(false), is_operator(false), is_method(false), has_args(false), has_kwargs(false), diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 7de7794344..5a599a3695 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -197,6 +197,11 @@ struct internals { PYBIND11_TLS_FREE(tstate); } #endif +#if PYBIND11_INTERNALS_VERSION >= 5 + // Note that we have to invoke strdup to allocate memory to ensure a unique address + // We want unique addresses since we use pointer equality to compare function records + const char *function_record_capsule_name = strdup("pybind11_function_record_capsule"); +#endif }; /// Additional type information which does not fit into the PyTypeObject. @@ -548,6 +553,25 @@ const char *c_str(Args &&...args) { return strings.front().c_str(); } +inline const char *get_function_record_capsule_name() { +#if PYBIND11_INTERNALS_VERSION >= 5 + return get_internals().function_record_capsule_name; +#else + return nullptr; +#endif +} + +// Determine whether or not the following capsule contains a pybind11 function record. +// Note that we use internals to make sure that only ABI compatible records are touched. +// +// This check is currently used in two places: +// - An important optimization in functional.h to avoid overhead in C++ -> Python -> C++ +// - The sibling feature of cpp_function to allow overloads +inline bool is_function_record_capsule(const capsule &cap) { + // Pointer equality as we rely + return cap.name() == get_function_record_capsule_name(); +} + PYBIND11_NAMESPACE_END(detail) /// Returns a named pointer that is shared among all extension modules (using the same diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 6096601549..79e0a598ff 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -69,13 +69,6 @@ inline bool apply_exception_translators(std::forward_list & return false; } -inline bool is_function_record_capsule(const capsule &cap) { - if (cap.name() == nullptr) { - return false; - } - return (std::strcmp(cap.name(), function_record::function_capsule_name) == 0); -} - #if defined(_MSC_VER) # define PYBIND11_COMPAT_STRDUP _strdup #else @@ -510,7 +503,7 @@ class cpp_function : public function { capsule rec_capsule(unique_rec.release(), [](void *ptr) { destruct((detail::function_record *) ptr); }); - rec_capsule.set_name(detail::function_record::function_capsule_name); + rec_capsule.set_name(detail::get_function_record_capsule_name()); guarded_strdup.release(); object scope_module; @@ -678,10 +671,9 @@ class cpp_function : public function { using namespace detail; /* Iterator over the list of potentially admissible overloads */ - const function_record *overloads - = reinterpret_cast( - PyCapsule_GetPointer(self, function_record::function_capsule_name)), - *it = overloads; + const function_record *overloads = reinterpret_cast( + PyCapsule_GetPointer(self, get_function_record_capsule_name())), + *it = overloads; assert(overloads != nullptr); /* Need to know how many arguments + keyword arguments there are to pick the right diff --git a/tests/test_callbacks.cpp b/tests/test_callbacks.cpp index d10bfcca76..61830947f1 100644 --- a/tests/test_callbacks.cpp +++ b/tests/test_callbacks.cpp @@ -16,8 +16,6 @@ int dummy_function(int i) { return i + 1; } -static PyMethodDef def; - TEST_SUBMODULE(callbacks, m) { // test_callbacks, test_function_signatures m.def("test_callback1", [](const py::object &func) { return func(); }); @@ -243,28 +241,37 @@ TEST_SUBMODULE(callbacks, m) { } }); - def.ml_name = "example_name"; - def.ml_doc = "Example doc"; - def.ml_meth = [](PyObject *, PyObject *args) -> PyObject * { - if (PyTuple_Size(args) != 1) { - throw std::runtime_error("Invalid number of arguments for example_name"); - } - PyObject *first = PyTuple_GetItem(args, 0); - if (!PyLong_Check(first)) { - throw std::runtime_error("Invalid argument to example_name"); - } - auto result = py::cast(PyLong_AsLong(first) * 9); - return result.release().ptr(); - }; - def.ml_flags = METH_VARARGS; + auto custom_def = []() { + static PyMethodDef def; + def.ml_name = "example_name"; + def.ml_doc = "Example doc"; + def.ml_meth = [](PyObject *, PyObject *args) -> PyObject * { + if (PyTuple_Size(args) != 1) { + throw std::runtime_error("Invalid number of arguments for example_name"); + } + PyObject *first = PyTuple_GetItem(args, 0); + if (!PyLong_Check(first)) { + throw std::runtime_error("Invalid argument to example_name"); + } + auto result = py::cast(PyLong_AsLong(first) * 9); + return result.release().ptr(); + }; + def.ml_flags = METH_VARARGS; + return &def; + }(); // rec_capsule with custom name - constexpr const char *rec_capsule_name = "CUSTOM_REC_CAPSULE"; + constexpr const char *rec_capsule_name = "pybind11_function_record_capsule"; py::capsule rec_capsule(std::malloc(1), [](void *data) { std::free(data); }); rec_capsule.set_name(rec_capsule_name); - m.add_object("custom_function", PyCFunction_New(&def, rec_capsule.ptr())); + m.add_object("custom_function", PyCFunction_New(custom_def, rec_capsule.ptr())); + // This test requires a new ABI version to pass +#if PYBIND11_INTERNALS_VERSION >= 5 // rec_capsule with nullptr name py::capsule rec_capsule2(std::malloc(1), [](void *data) { std::free(data); }); - m.add_object("custom_function2", PyCFunction_New(&def, rec_capsule2.ptr())); + m.add_object("custom_function2", PyCFunction_New(custom_def, rec_capsule2.ptr())); +#else + m.add_object("custom_function2", PyCFunction_New(custom_def, rec_capsule.ptr())); +#endif } From 8f8a873bfd2655670ce4b14e1137700aba71c27e Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Sat, 22 Oct 2022 23:59:20 +0000 Subject: [PATCH 25/31] Fix lint --- include/pybind11/attr.h | 1 - tests/test_callbacks.cpp | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index 2284cd8a3d..db7cd8efff 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -186,7 +186,6 @@ struct argument_record { /// Internal data structure which holds metadata about a bound function (signature, overloads, /// etc.) struct function_record { - function_record() : is_constructor(false), is_new_style_constructor(false), is_stateless(false), is_operator(false), is_method(false), has_args(false), has_kwargs(false), diff --git a/tests/test_callbacks.cpp b/tests/test_callbacks.cpp index 61830947f1..e0da0026b1 100644 --- a/tests/test_callbacks.cpp +++ b/tests/test_callbacks.cpp @@ -241,7 +241,7 @@ TEST_SUBMODULE(callbacks, m) { } }); - auto custom_def = []() { + auto *custom_def = []() { static PyMethodDef def; def.ml_name = "example_name"; def.ml_doc = "Example doc"; From 2d054d3950e96a290f215e3fca238842d71fd5c5 Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Sun, 23 Oct 2022 00:03:06 +0000 Subject: [PATCH 26/31] Oops, forgot to resolve last comment --- include/pybind11/functional.h | 2 +- include/pybind11/pybind11.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/functional.h b/include/pybind11/functional.h index 72a88e6170..87ec4d10cb 100644 --- a/include/pybind11/functional.h +++ b/include/pybind11/functional.h @@ -56,7 +56,7 @@ struct type_caster> { function_record *rec = nullptr; // Check that we can safely reinterpret the capsule into a function_record if (detail::is_function_record_capsule(c)) { - rec = static_cast(c); + rec = c.get_pointer(); } while (rec != nullptr) { diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 79e0a598ff..5eeedf85ab 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1896,7 +1896,7 @@ class class_ : public detail::generic_type { if (!detail::is_function_record_capsule(cap)) { return nullptr; } - return static_cast(cap); + return cap.get_pointer(); } }; From 8e4cece000591da98ca2835c30e890539bf4c6fa Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Sun, 23 Oct 2022 00:07:07 +0000 Subject: [PATCH 27/31] Fix typo --- include/pybind11/detail/internals.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 5a599a3695..dc2a2fbbd2 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -568,7 +568,7 @@ inline const char *get_function_record_capsule_name() { // - An important optimization in functional.h to avoid overhead in C++ -> Python -> C++ // - The sibling feature of cpp_function to allow overloads inline bool is_function_record_capsule(const capsule &cap) { - // Pointer equality as we rely + // Pointer equality as we rely on internals() to ensure unique pointers return cap.name() == get_function_record_capsule_name(); } From aa89200509ae0a953dca3a46893a55e6bb36788d Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Sat, 29 Oct 2022 19:07:43 +0000 Subject: [PATCH 28/31] Update in response to comments --- include/pybind11/detail/internals.h | 21 +++++++++++++-------- include/pybind11/pybind11.h | 2 +- tests/test_callbacks.cpp | 4 ++-- tests/test_callbacks.py | 5 +++++ 4 files changed, 21 insertions(+), 11 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index dc2a2fbbd2..26fe266f05 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -182,6 +182,16 @@ struct internals { # endif // PYBIND11_INTERNALS_VERSION > 4 // Unused if PYBIND11_SIMPLE_GIL_MANAGEMENT is defined: PyInterpreterState *istate = nullptr; + +# if PYBIND11_INTERNALS_VERSION > 4 + // Note that we have to invoke strdup to allocate memory to ensure a unique address + // We want unique addresses since we use pointer equality to compare function records + std::string function_record_capsule_name = "pybind11_function_record_capsule"; +# endif + + internals() = default; + internals(const internals &other) = delete; + internals &operator=(const internals &other) = delete; ~internals() { # if PYBIND11_INTERNALS_VERSION > 4 PYBIND11_TLS_FREE(loader_life_support_tls_key); @@ -197,11 +207,6 @@ struct internals { PYBIND11_TLS_FREE(tstate); } #endif -#if PYBIND11_INTERNALS_VERSION >= 5 - // Note that we have to invoke strdup to allocate memory to ensure a unique address - // We want unique addresses since we use pointer equality to compare function records - const char *function_record_capsule_name = strdup("pybind11_function_record_capsule"); -#endif }; /// Additional type information which does not fit into the PyTypeObject. @@ -554,15 +559,15 @@ const char *c_str(Args &&...args) { } inline const char *get_function_record_capsule_name() { -#if PYBIND11_INTERNALS_VERSION >= 5 - return get_internals().function_record_capsule_name; +#if PYBIND11_INTERNALS_VERSION > 4 + return get_internals().function_record_capsule_name.c_str(); #else return nullptr; #endif } // Determine whether or not the following capsule contains a pybind11 function record. -// Note that we use internals to make sure that only ABI compatible records are touched. +// Note that we use `internals` to make sure that only ABI compatible records are touched. // // This check is currently used in two places: // - An important optimization in functional.h to avoid overhead in C++ -> Python -> C++ diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 5eeedf85ab..915e27da20 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -473,7 +473,7 @@ class cpp_function : public function { } else { auto rec_capsule = reinterpret_borrow(self); if (detail::is_function_record_capsule(rec_capsule)) { - chain = static_cast(rec_capsule); + chain = rec_capsule.get_pointer(); /* Never append a method to an overload chain of a parent class; instead, hide the parent's overloads in this case */ if (!chain->scope.is(rec->scope)) { diff --git a/tests/test_callbacks.cpp b/tests/test_callbacks.cpp index e0da0026b1..e3c23e79c4 100644 --- a/tests/test_callbacks.cpp +++ b/tests/test_callbacks.cpp @@ -267,11 +267,11 @@ TEST_SUBMODULE(callbacks, m) { m.add_object("custom_function", PyCFunction_New(custom_def, rec_capsule.ptr())); // This test requires a new ABI version to pass -#if PYBIND11_INTERNALS_VERSION >= 5 +#if PYBIND11_INTERNALS_VERSION > 4 // rec_capsule with nullptr name py::capsule rec_capsule2(std::malloc(1), [](void *data) { std::free(data); }); m.add_object("custom_function2", PyCFunction_New(custom_def, rec_capsule2.ptr())); #else - m.add_object("custom_function2", PyCFunction_New(custom_def, rec_capsule.ptr())); + m.add_object("custom_function2", py::none()); #endif } diff --git a/tests/test_callbacks.py b/tests/test_callbacks.py index a942d72ae1..57b6599880 100644 --- a/tests/test_callbacks.py +++ b/tests/test_callbacks.py @@ -199,5 +199,10 @@ def test_custom_func(): assert m.custom_function(4) == 36 assert m.roundtrip(m.custom_function)(4) == 36 + +@pytest.mark.skipif( + m.custom_function2 is None, reason="Current PYBIND11_INTERNALS_VERSION too low" +) +def test_custom_func2(): assert m.custom_function2(3) == 27 assert m.roundtrip(m.custom_function2)(3) == 27 From f184c54121929c77b7edc81f9c2dcb7ffe4d8681 Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Tue, 1 Nov 2022 15:17:36 +0000 Subject: [PATCH 29/31] Implement suggestion to improve test --- include/pybind11/detail/internals.h | 5 ++++- tests/test_callbacks.cpp | 7 +++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 26fe266f05..2f778c06f5 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -43,6 +43,9 @@ using ExceptionTranslator = void (*)(std::exception_ptr); PYBIND11_NAMESPACE_BEGIN(detail) +static constexpr const char *internals_function_record_capsule_name + = "pybind11_function_record_capsule"; + // Forward declarations inline PyTypeObject *make_static_property_type(); inline PyTypeObject *make_default_metaclass(); @@ -186,7 +189,7 @@ struct internals { # if PYBIND11_INTERNALS_VERSION > 4 // Note that we have to invoke strdup to allocate memory to ensure a unique address // We want unique addresses since we use pointer equality to compare function records - std::string function_record_capsule_name = "pybind11_function_record_capsule"; + std::string function_record_capsule_name = internals_function_record_capsule_name; # endif internals() = default; diff --git a/tests/test_callbacks.cpp b/tests/test_callbacks.cpp index e3c23e79c4..2fd05dec72 100644 --- a/tests/test_callbacks.cpp +++ b/tests/test_callbacks.cpp @@ -260,8 +260,11 @@ TEST_SUBMODULE(callbacks, m) { return &def; }(); - // rec_capsule with custom name - constexpr const char *rec_capsule_name = "pybind11_function_record_capsule"; + // rec_capsule with name that has the same value (but not pointer) as our internal one + // This capsule should be detected by our code as foreign and not inspected as the pointers + // shouldn't match + constexpr const char *rec_capsule_name + = pybind11::detail::internals_function_record_capsule_name; py::capsule rec_capsule(std::malloc(1), [](void *data) { std::free(data); }); rec_capsule.set_name(rec_capsule_name); m.add_object("custom_function", PyCFunction_New(custom_def, rec_capsule.ptr())); From 14b7e335306907f2ebd2b84d635f035076689622 Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Tue, 1 Nov 2022 16:00:19 +0000 Subject: [PATCH 30/31] Update comment --- include/pybind11/detail/internals.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 2f778c06f5..222944fe22 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -187,7 +187,7 @@ struct internals { PyInterpreterState *istate = nullptr; # if PYBIND11_INTERNALS_VERSION > 4 - // Note that we have to invoke strdup to allocate memory to ensure a unique address + // Note that we have to use a std::string to allocate memory to ensure a unique address // We want unique addresses since we use pointer equality to compare function records std::string function_record_capsule_name = internals_function_record_capsule_name; # endif From d54fa89b26074eb940491d870185602bfe73c3f8 Mon Sep 17 00:00:00 2001 From: Ethan Steinberg Date: Tue, 1 Nov 2022 17:47:07 +0000 Subject: [PATCH 31/31] Simple fixes --- include/pybind11/detail/internals.h | 3 +-- include/pybind11/pybind11.h | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 222944fe22..6fd61098c4 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -43,8 +43,7 @@ using ExceptionTranslator = void (*)(std::exception_ptr); PYBIND11_NAMESPACE_BEGIN(detail) -static constexpr const char *internals_function_record_capsule_name - = "pybind11_function_record_capsule"; +constexpr const char *internals_function_record_capsule_name = "pybind11_function_record_capsule"; // Forward declarations inline PyTypeObject *make_static_property_type(); diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 915e27da20..76d6eadcae 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -669,6 +669,7 @@ class cpp_function : public function { /// Main dispatch logic for calls to functions bound using pybind11 static PyObject *dispatcher(PyObject *self, PyObject *args_in, PyObject *kwargs_in) { using namespace detail; + assert(isinstance(self)); /* Iterator over the list of potentially admissible overloads */ const function_record *overloads = reinterpret_cast(