From 42b01fb60e88335824980d3a7905ce22290a92bb Mon Sep 17 00:00:00 2001 From: Yichen Yan Date: Tue, 11 May 2021 18:01:03 +0800 Subject: [PATCH 1/9] Set visibility of exceptions to default. Co-authored-by: XZiar --- include/pybind11/detail/common.h | 11 +++++++++-- include/pybind11/pytypes.h | 9 ++++++++- 2 files changed, 17 insertions(+), 3 deletions(-) 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/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 From 4a08d26e9b8f40960d7a8d4455718cdcf7804b5a Mon Sep 17 00:00:00 2001 From: Yichen Yan Date: Sun, 23 May 2021 22:19:52 +0800 Subject: [PATCH 2/9] add test --- tests/pybind11_cross_module_tests.cpp | 8 ++++++++ tests/test_exceptions.cpp | 2 ++ tests/test_exceptions.h | 12 ++++++++++++ tests/test_exceptions.py | 3 +++ 4 files changed, 25 insertions(+) create mode 100644 tests/test_exceptions.h diff --git a/tests/pybind11_cross_module_tests.cpp b/tests/pybind11_cross_module_tests.cpp index f705e31061..73c7b8c808 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 tmp_e &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..2ebb747a60 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_", []() { throw tmp_e(); }); } diff --git a/tests/test_exceptions.h b/tests/test_exceptions.h new file mode 100644 index 0000000000..27a80292f1 --- /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 tmp_e : public pybind11::builtin_exception { +public: + using builtin_exception::builtin_exception; + explicit tmp_e() : tmp_e("") {} + void set_error() const override { PyErr_SetString(PyExc_RuntimeError, what()); } +}; diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index c6cb65299e..1656a4770e 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -43,6 +43,9 @@ def test_cross_module_exceptions(): with pytest.raises(StopIteration) as excinfo: cm.throw_stop_iteration() + with pytest.raises(KeyError) as excinfo: + m.throw_() + def test_python_call_in_catch(): d = {} From 0987a87d844d66cd420bea2d04720c053edd141e Mon Sep 17 00:00:00 2001 From: Yichen Yan Date: Sun, 23 May 2021 22:37:24 +0800 Subject: [PATCH 3/9] update docs --- docs/advanced/exceptions.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/advanced/exceptions.rst b/docs/advanced/exceptions.rst index 7a4d6cbc6e..eb10f97b0d 100644 --- a/docs/advanced/exceptions.rst +++ b/docs/advanced/exceptions.rst @@ -164,6 +164,9 @@ section. may be explicitly (re-)thrown to delegate it to the other, previously-declared existing exception translators. + Libc++ `behaves differently `_ + with ``-fvisibility=hidden``. So you'll need to explicitly export exceptions who'll be used across ABI boundaries as ``tests/test_exceptions.h``. + .. _handling_python_exceptions_cpp: Handling exceptions from Python in C++ From ee1d13a7402a330088b2f4cab9723f8cb7b63b7f Mon Sep 17 00:00:00 2001 From: Yichen Yan Date: Mon, 24 May 2021 23:00:43 +0800 Subject: [PATCH 4/9] refine --- tests/pybind11_cross_module_tests.cpp | 2 +- tests/test_exceptions.cpp | 2 +- tests/test_exceptions.h | 4 ++-- tests/test_exceptions.py | 3 ++- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/pybind11_cross_module_tests.cpp b/tests/pybind11_cross_module_tests.cpp index 73c7b8c808..944d085597 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -34,7 +34,7 @@ PYBIND11_MODULE(pybind11_cross_module_tests, m) { py::register_exception_translator([](std::exception_ptr p) { try { if (p) std::rethrow_exception(p); - } catch (const tmp_e &e) { + } catch (const shared_exception &e) { PyErr_SetString(PyExc_KeyError, e.what()); } }); diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index 2ebb747a60..4c5e10dbe0 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -229,5 +229,5 @@ TEST_SUBMODULE(exceptions, m) { // Test repr that cannot be displayed m.def("simple_bool_passthrough", [](bool x) {return x;}); - m.def("throw_", []() { throw tmp_e(); }); + m.def("throw_should_be_translated_to_key_error", []() { throw shared_exception(); }); } diff --git a/tests/test_exceptions.h b/tests/test_exceptions.h index 27a80292f1..5d02d1b35b 100644 --- a/tests/test_exceptions.h +++ b/tests/test_exceptions.h @@ -4,9 +4,9 @@ // shared exceptions for cross_module_tests -class PYBIND11_EXPORT tmp_e : public pybind11::builtin_exception { +class PYBIND11_EXPORT shared_exception : public pybind11::builtin_exception { public: using builtin_exception::builtin_exception; - explicit tmp_e() : tmp_e("") {} + 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 1656a4770e..b3e702f806 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -44,7 +44,8 @@ def test_cross_module_exceptions(): cm.throw_stop_iteration() with pytest.raises(KeyError) as excinfo: - m.throw_() + # translator registered in cross_module_tests + m.throw_should_be_translated_to_key_error() def test_python_call_in_catch(): From 97773db1de7204e15becff8b8cb6addbe8c323c9 Mon Sep 17 00:00:00 2001 From: Yichen Yan Date: Wed, 26 May 2021 10:12:28 +0800 Subject: [PATCH 5/9] Skip failed test. --- tests/test_exceptions.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index b3e702f806..f0f83cae0b 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 @@ -43,7 +45,14 @@ def test_cross_module_exceptions(): with pytest.raises(StopIteration) as excinfo: cm.throw_stop_iteration() - with pytest.raises(KeyError) as excinfo: + +# TODO: FIXME +@pytest.mark.skipif( + "env.PYPY and env.MACOS", + reason="Known failure with PyPy and libc++ (Issue #2847 & PR #2999)", +) +def test_cross_module_exception_translator(): + with pytest.raises(KeyError) as _: # translator registered in cross_module_tests m.throw_should_be_translated_to_key_error() From bd3018849dc08c62c89bdda2e503553630cc96ef Mon Sep 17 00:00:00 2001 From: Yichen Yan Date: Wed, 26 May 2021 11:28:03 +0800 Subject: [PATCH 6/9] update docs --- docs/advanced/exceptions.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/advanced/exceptions.rst b/docs/advanced/exceptions.rst index eb10f97b0d..1110dfa5ac 100644 --- a/docs/advanced/exceptions.rst +++ b/docs/advanced/exceptions.rst @@ -166,6 +166,7 @@ section. Libc++ `behaves differently `_ with ``-fvisibility=hidden``. So you'll need to explicitly export exceptions who'll be used across ABI boundaries as ``tests/test_exceptions.h``. + See also section "Problems with C++ exceptions" in `GCC Wiki `_. .. _handling_python_exceptions_cpp: From 07fae167e26a22111c2ff678140a45e2782a9e45 Mon Sep 17 00:00:00 2001 From: Yichen Yan Date: Wed, 26 May 2021 16:00:55 +0800 Subject: [PATCH 7/9] cleanup old workaround. --- include/pybind11/detail/internals.h | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 75fcd3c208..e430bfc848 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -242,16 +242,6 @@ inline void translate_exception(std::exception_ptr p) { } } -#if !defined(__GLIBCXX__) -inline void translate_local_exception(std::exception_ptr p) { - try { - if (p) std::rethrow_exception(p); - } catch (error_already_set &e) { e.restore(); return; - } catch (const builtin_exception &e) { e.set_error(); return; - } -} -#endif - /// Return a reference to the current `internals` data PYBIND11_NOINLINE inline internals &get_internals() { auto **&internals_pp = get_internals_pp(); @@ -270,15 +260,6 @@ PYBIND11_NOINLINE inline internals &get_internals() { auto builtins = handle(PyEval_GetBuiltins()); if (builtins.contains(id) && isinstance(builtins[id])) { internals_pp = static_cast(capsule(builtins[id])); - - // We loaded builtins through python's builtins, which means that our `error_already_set` - // and `builtin_exception` may be different local classes than the ones set up in the - // initial exception translator, below, so add another for our local exception classes. - // - // libstdc++ doesn't require this (types there are identified only by name) -#if !defined(__GLIBCXX__) - (*internals_pp)->registered_exception_translators.push_front(&translate_local_exception); -#endif } else { if (!internals_pp) internals_pp = new internals*(); auto *&internals_ptr = *internals_pp; From 1defec9a6eada10f7800789cba211090c9d56638 Mon Sep 17 00:00:00 2001 From: Yichen Yan Date: Wed, 26 May 2021 23:07:57 +0800 Subject: [PATCH 8/9] update --- include/pybind11/detail/internals.h | 21 +++++++++++++++++++++ tests/test_exceptions.py | 3 ++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index e430bfc848..5578c65160 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -242,6 +242,16 @@ inline void translate_exception(std::exception_ptr p) { } } +#if !defined(__GLIBCXX__) +inline void translate_local_exception(std::exception_ptr p) { + try { + if (p) std::rethrow_exception(p); + } catch (error_already_set &e) { e.restore(); return; + } catch (const builtin_exception &e) { e.set_error(); return; + } +} +#endif + /// Return a reference to the current `internals` data PYBIND11_NOINLINE inline internals &get_internals() { auto **&internals_pp = get_internals_pp(); @@ -260,6 +270,17 @@ PYBIND11_NOINLINE inline internals &get_internals() { auto builtins = handle(PyEval_GetBuiltins()); if (builtins.contains(id) && isinstance(builtins[id])) { internals_pp = static_cast(capsule(builtins[id])); + + // We loaded builtins through python's builtins, which means that our `error_already_set` + // and `builtin_exception` may be different local classes than the ones set up in the + // 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 } else { if (!internals_pp) internals_pp = new internals*(); auto *&internals_ptr = *internals_pp; diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index f0f83cae0b..d2d52423d7 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -47,8 +47,9 @@ def test_cross_module_exceptions(): # TODO: FIXME -@pytest.mark.skipif( +@pytest.mark.xfail( "env.PYPY and env.MACOS", + raises=RuntimeError, reason="Known failure with PyPy and libc++ (Issue #2847 & PR #2999)", ) def test_cross_module_exception_translator(): From d691d1983c446bbba05b4174ba1631ac2cc9b25c Mon Sep 17 00:00:00 2001 From: Yichen Yan Date: Thu, 27 May 2021 07:24:31 +0800 Subject: [PATCH 9/9] update --- docs/advanced/exceptions.rst | 6 +++--- tests/test_exceptions.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/advanced/exceptions.rst b/docs/advanced/exceptions.rst index 1110dfa5ac..f70c202fd5 100644 --- a/docs/advanced/exceptions.rst +++ b/docs/advanced/exceptions.rst @@ -164,9 +164,9 @@ section. may be explicitly (re-)thrown to delegate it to the other, previously-declared existing exception translators. - Libc++ `behaves differently `_ - with ``-fvisibility=hidden``. So you'll need to explicitly export exceptions who'll be used across ABI boundaries as ``tests/test_exceptions.h``. - See also section "Problems with C++ exceptions" in `GCC Wiki `_. + 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: diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index d2d52423d7..b9fc269381 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -50,10 +50,10 @@ def test_cross_module_exceptions(): @pytest.mark.xfail( "env.PYPY and env.MACOS", raises=RuntimeError, - reason="Known failure with PyPy and libc++ (Issue #2847 & PR #2999)", + reason="Expected failure with PyPy and libc++ (Issue #2847 & PR #2999)", ) def test_cross_module_exception_translator(): - with pytest.raises(KeyError) as _: + with pytest.raises(KeyError): # translator registered in cross_module_tests m.throw_should_be_translated_to_key_error()