From 3ac690b88be00a44c1988afc209a395d87a57051 Mon Sep 17 00:00:00 2001 From: Yichen Date: Thu, 27 May 2021 23:00:18 +0800 Subject: [PATCH] Explicitly export exception types. (#2999) * Set visibility of exceptions to default. Co-authored-by: XZiar * add test * update docs * Skip failed test. --- docs/advanced/exceptions.rst | 4 ++++ include/pybind11/detail/common.h | 11 +++++++++-- include/pybind11/detail/internals.h | 2 ++ include/pybind11/pytypes.h | 9 ++++++++- tests/pybind11_cross_module_tests.cpp | 8 ++++++++ tests/test_exceptions.cpp | 2 ++ tests/test_exceptions.h | 12 ++++++++++++ tests/test_exceptions.py | 14 ++++++++++++++ 8 files changed, 59 insertions(+), 3 deletions(-) create mode 100644 tests/test_exceptions.h diff --git a/docs/advanced/exceptions.rst b/docs/advanced/exceptions.rst index 7a4d6cbc6e..f70c202fd5 100644 --- a/docs/advanced/exceptions.rst +++ b/docs/advanced/exceptions.rst @@ -164,6 +164,10 @@ section. may be explicitly (re-)thrown to delegate it to the other, previously-declared existing exception translators. + Note that ``libc++`` and ``libstdc++`` `behave differently `_ + with ``-fvisibility=hidden``. Therefore exceptions that are used across ABI boundaries need to be explicitly exported, as exercised in ``tests/test_exceptions.h``. + See also: "Problems with C++ exceptions" under `GCC Wiki `_. + .. _handling_python_exceptions_cpp: Handling exceptions from Python in C++ diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 5560198a05..987f0badf9 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -723,16 +723,23 @@ using expand_side_effects = bool[]; PYBIND11_NAMESPACE_END(detail) +#if defined(_MSC_VER) +# pragma warning(push) +# pragma warning(disable: 4275) // warning C4275: An exported class was derived from a class that wasn't exported. Can be ignored when derived from a STL class. +#endif /// C++ bindings of builtin Python exceptions -class builtin_exception : public std::runtime_error { +class PYBIND11_EXPORT builtin_exception : public std::runtime_error { public: using std::runtime_error::runtime_error; /// Set the error using the Python C API virtual void set_error() const = 0; }; +#if defined(_MSC_VER) +# pragma warning(pop) +#endif #define PYBIND11_RUNTIME_EXCEPTION(name, type) \ - class name : public builtin_exception { public: \ + class PYBIND11_EXPORT name : public builtin_exception { public: \ using builtin_exception::builtin_exception; \ name() : name("") { } \ void set_error() const override { PyErr_SetString(type, what()); } \ diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 75fcd3c208..5578c65160 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -276,6 +276,8 @@ PYBIND11_NOINLINE inline internals &get_internals() { // initial exception translator, below, so add another for our local exception classes. // // libstdc++ doesn't require this (types there are identified only by name) + // libc++ with CPython doesn't require this (types are explicitly exported) + // libc++ with PyPy still need it, awaiting further investigation #if !defined(__GLIBCXX__) (*internals_pp)->registered_exception_translators.push_front(&translate_local_exception); #endif diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 6f0e6067ef..6a1d8356a5 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -319,11 +319,15 @@ PYBIND11_NAMESPACE_BEGIN(detail) inline std::string error_string(); PYBIND11_NAMESPACE_END(detail) +#if defined(_MSC_VER) +# pragma warning(push) +# pragma warning(disable: 4275 4251) // warning C4275: An exported class was derived from a class that wasn't exported. Can be ignored when derived from a STL class. +#endif /// Fetch and hold an error which was already set in Python. An instance of this is typically /// thrown to propagate python-side errors back through C++ which can either be caught manually or /// else falls back to the function dispatcher (which then raises the captured error back to /// python). -class error_already_set : public std::runtime_error { +class PYBIND11_EXPORT error_already_set : public std::runtime_error { public: /// Constructs a new exception from the current Python error indicator, if any. The current /// Python error indicator will be cleared. @@ -371,6 +375,9 @@ class error_already_set : public std::runtime_error { private: object m_type, m_value, m_trace; }; +#if defined(_MSC_VER) +# pragma warning(pop) +#endif /** \defgroup python_builtins _ Unless stated otherwise, the following C++ functions behave the same diff --git a/tests/pybind11_cross_module_tests.cpp b/tests/pybind11_cross_module_tests.cpp index f705e31061..944d085597 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -9,6 +9,7 @@ #include "pybind11_tests.h" #include "local_bindings.h" +#include "test_exceptions.h" #include #include @@ -30,6 +31,13 @@ PYBIND11_MODULE(pybind11_cross_module_tests, m) { m.def("throw_pybind_value_error", []() { throw py::value_error("pybind11 value error"); }); m.def("throw_pybind_type_error", []() { throw py::type_error("pybind11 type error"); }); m.def("throw_stop_iteration", []() { throw py::stop_iteration(); }); + py::register_exception_translator([](std::exception_ptr p) { + try { + if (p) std::rethrow_exception(p); + } catch (const shared_exception &e) { + PyErr_SetString(PyExc_KeyError, e.what()); + } + }); // test_local_bindings.py // Local to both: diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index e27c16dfef..4c5e10dbe0 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -7,6 +7,7 @@ BSD-style license that can be found in the LICENSE file. */ +#include "test_exceptions.h" #include "pybind11_tests.h" // A type that should be raised as an exception in Python @@ -228,4 +229,5 @@ TEST_SUBMODULE(exceptions, m) { // Test repr that cannot be displayed m.def("simple_bool_passthrough", [](bool x) {return x;}); + m.def("throw_should_be_translated_to_key_error", []() { throw shared_exception(); }); } diff --git a/tests/test_exceptions.h b/tests/test_exceptions.h new file mode 100644 index 0000000000..5d02d1b35b --- /dev/null +++ b/tests/test_exceptions.h @@ -0,0 +1,12 @@ +#pragma once +#include "pybind11_tests.h" +#include + +// shared exceptions for cross_module_tests + +class PYBIND11_EXPORT shared_exception : public pybind11::builtin_exception { +public: + using builtin_exception::builtin_exception; + explicit shared_exception() : shared_exception("") {} + void set_error() const override { PyErr_SetString(PyExc_RuntimeError, what()); } +}; diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index c6cb65299e..b9fc269381 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -3,6 +3,8 @@ import pytest +import env # noqa: F401 + from pybind11_tests import exceptions as m import pybind11_cross_module_tests as cm @@ -44,6 +46,18 @@ def test_cross_module_exceptions(): cm.throw_stop_iteration() +# TODO: FIXME +@pytest.mark.xfail( + "env.PYPY and env.MACOS", + raises=RuntimeError, + reason="Expected failure with PyPy and libc++ (Issue #2847 & PR #2999)", +) +def test_cross_module_exception_translator(): + with pytest.raises(KeyError): + # translator registered in cross_module_tests + m.throw_should_be_translated_to_key_error() + + def test_python_call_in_catch(): d = {} assert m.python_call_in_destructor(d) is True