From b60fd233fa423ff56579513efd90820509efb2cc Mon Sep 17 00:00:00 2001 From: Saran Tunyasuvunakool Date: Mon, 15 Jul 2019 15:47:02 +0100 Subject: [PATCH] Make sure `detail::get_internals` acquires the GIL before making Python calls. (#1836) This is only necessary if `get_internals` is called for the first time in a given module when the running thread is in a GIL-released state. Fixes #1364 --- include/pybind11/detail/internals.h | 8 ++++ tests/CMakeLists.txt | 12 +++++ tests/cross_module_gil_utils.cpp | 73 +++++++++++++++++++++++++++++ tests/test_gil_scoped.cpp | 8 ++++ tests/test_gil_scoped.py | 5 ++ 5 files changed, 106 insertions(+) create mode 100644 tests/cross_module_gil_utils.cpp diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index f1dd38764..952f80173 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -171,6 +171,14 @@ PYBIND11_NOINLINE inline internals &get_internals() { if (internals_pp && *internals_pp) return **internals_pp; + // Ensure that the GIL is held since we will need to make Python calls. + // Cannot use py::gil_scoped_acquire here since that constructor calls get_internals. + struct gil_scoped_acquire { + gil_scoped_acquire() : state (PyGILState_Ensure()) {} + ~gil_scoped_acquire() { PyGILState_Release(state); } + const PyGILState_STATE state; + } gil; + constexpr auto *id = PYBIND11_INTERNALS_ID; auto builtins = handle(PyEval_GetBuiltins()); if (builtins.contains(id) && isinstance(builtins[id])) { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 9a701108c..fb6776f2a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -83,6 +83,10 @@ set(PYBIND11_CROSS_MODULE_TESTS test_stl_binders.py ) +set(PYBIND11_CROSS_MODULE_GIL_TESTS + test_gil_scoped.py +) + # Check if Eigen is available; if not, remove from PYBIND11_TEST_FILES (but # keep it in PYBIND11_PYTEST_FILES, so that we get the "eigen is not installed" # skip message). @@ -150,6 +154,14 @@ foreach(t ${PYBIND11_CROSS_MODULE_TESTS}) endif() endforeach() +foreach(t ${PYBIND11_CROSS_MODULE_GIL_TESTS}) + list(FIND PYBIND11_PYTEST_FILES ${t} i) + if (i GREATER -1) + list(APPEND test_targets cross_module_gil_utils) + break() + endif() +endforeach() + set(testdir ${CMAKE_CURRENT_SOURCE_DIR}) foreach(target ${test_targets}) set(test_files ${PYBIND11_TEST_FILES}) diff --git a/tests/cross_module_gil_utils.cpp b/tests/cross_module_gil_utils.cpp new file mode 100644 index 000000000..07db9f6e4 --- /dev/null +++ b/tests/cross_module_gil_utils.cpp @@ -0,0 +1,73 @@ +/* + tests/cross_module_gil_utils.cpp -- tools for acquiring GIL from a different module + + Copyright (c) 2019 Google LLC + + All rights reserved. Use of this source code is governed by a + BSD-style license that can be found in the LICENSE file. +*/ +#include +#include + +// This file mimics a DSO that makes pybind11 calls but does not define a +// PYBIND11_MODULE. The purpose is to test that such a DSO can create a +// py::gil_scoped_acquire when the running thread is in a GIL-released state. +// +// Note that we define a Python module here for convenience, but in general +// this need not be the case. The typical scenario would be a DSO that implements +// shared logic used internally by multiple pybind11 modules. + +namespace { + +namespace py = pybind11; +void gil_acquire() { py::gil_scoped_acquire gil; } + +constexpr char kModuleName[] = "cross_module_gil_utils"; + +#if PY_MAJOR_VERSION >= 3 +struct PyModuleDef moduledef = { + PyModuleDef_HEAD_INIT, + kModuleName, + NULL, + 0, + NULL, + NULL, + NULL, + NULL, + NULL +}; +#else +PyMethodDef module_methods[] = { + {NULL, NULL, 0, NULL} +}; +#endif + +} // namespace + +extern "C" PYBIND11_EXPORT +#if PY_MAJOR_VERSION >= 3 +PyObject* PyInit_cross_module_gil_utils() +#else +void initcross_module_gil_utils() +#endif +{ + + PyObject* m = +#if PY_MAJOR_VERSION >= 3 + PyModule_Create(&moduledef); +#else + Py_InitModule(kModuleName, module_methods); +#endif + + if (m != NULL) { + static_assert( + sizeof(&gil_acquire) == sizeof(void*), + "Function pointer must have the same size as void*"); + PyModule_AddObject(m, "gil_acquire_funcaddr", + PyLong_FromVoidPtr(reinterpret_cast(&gil_acquire))); + } + +#if PY_MAJOR_VERSION >= 3 + return m; +#endif +} diff --git a/tests/test_gil_scoped.cpp b/tests/test_gil_scoped.cpp index cb0010ee6..76c17fdc7 100644 --- a/tests/test_gil_scoped.cpp +++ b/tests/test_gil_scoped.cpp @@ -41,4 +41,12 @@ TEST_SUBMODULE(gil_scoped, m) { [](VirtClass &virt) { virt.virtual_func(); }); m.def("test_callback_pure_virtual_func", [](VirtClass &virt) { virt.pure_virtual_func(); }); + m.def("test_cross_module_gil", + []() { + auto cm = py::module::import("cross_module_gil_utils"); + auto gil_acquire = reinterpret_cast( + PyLong_AsVoidPtr(cm.attr("gil_acquire_funcaddr").ptr())); + py::gil_scoped_release gil_release; + gil_acquire(); + }); } diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index 2c72fc6d6..1548337cc 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -78,3 +78,8 @@ def test_python_to_cpp_to_python_from_process(): This test is for completion, but it was never an issue. """ assert _run_in_process(_python_to_cpp_to_python) == 0 + + +def test_cross_module_gil(): + """Makes sure that the GIL can be acquired by another module from a GIL-released state.""" + m.test_cross_module_gil() # Should not raise a SIGSEGV