From cefa9cda6465235451f861808104426f15dd7428 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 17 Jul 2022 22:32:00 -0700 Subject: [PATCH 1/6] For PyPy only, re-enable old behavior (likely to mask bugs), to avoid segfault with unknown root cause. Change prompted by https://github.com/pybind/pybind11/issues/4075 --- include/pybind11/pytypes.h | 6 ++++++ tests/test_exceptions.py | 32 +++++++++++++++++++++----------- 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 339b0961eb..93146ee274 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -456,6 +456,11 @@ struct error_fetch_and_normalize { + " failed to obtain the name " "of the normalized active exception type."); } +#if defined(PYPY_VERSION) + // This behavior masks errors in the error handling, but avoids a PyPy segfault (root + // cause unknown). See https://github.com/pybind/pybind11/issues/4075 for background. + m_lazy_error_string = exc_type_name_norm; +#else if (exc_type_name_norm != m_lazy_error_string) { std::string msg = std::string(called) + ": MISMATCH of original and normalized " @@ -467,6 +472,7 @@ struct error_fetch_and_normalize { msg += ": " + format_value_and_trace(); pybind11_fail(msg); } +#endif } error_fetch_and_normalize(const error_fetch_and_normalize &) = delete; diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index a5984a142f..f39cf946de 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -303,18 +303,28 @@ def test_error_already_set_what_with_happy_exceptions( assert what == expected_what -@pytest.mark.skipif("env.PYPY", reason="PyErr_NormalizeException Segmentation fault") def test_flaky_exception_failure_point_init(): - with pytest.raises(RuntimeError) as excinfo: - m.error_already_set_what(FlakyException, ("failure_point_init",)) - lines = str(excinfo.value).splitlines() - # PyErr_NormalizeException replaces the original FlakyException with ValueError: - assert lines[:3] == [ - "pybind11::error_already_set: MISMATCH of original and normalized active exception types:" - " ORIGINAL FlakyException REPLACED BY ValueError: triggered_failure_point_init", - "", - "At:", - ] + if env.PYPY: + # This behavior masks errors in the error handling, but avoids a PyPy segfault (root + # cause unknown). See https://github.com/pybind/pybind11/issues/4075 for background. + what, py_err_set_after_what = m.error_already_set_what( + FlakyException, ("failure_point_init",) + ) + assert not py_err_set_after_what + lines = what.splitlines() + # PyErr_NormalizeException replaces the original FlakyException with ValueError: + assert lines[:3] == ["ValueError: triggered_failure_point_init", "", "At:"] + else: + with pytest.raises(RuntimeError) as excinfo: + m.error_already_set_what(FlakyException, ("failure_point_init",)) + lines = str(excinfo.value).splitlines() + # PyErr_NormalizeException replaces the original FlakyException with ValueError: + assert lines[:3] == [ + "pybind11::error_already_set: MISMATCH of original and normalized active exception types:" + " ORIGINAL FlakyException REPLACED BY ValueError: triggered_failure_point_init", + "", + "At:", + ] # Checking the first two lines of the traceback as formatted in error_string(): assert "test_exceptions.py(" in lines[3] assert lines[3].endswith("): __init__") From 4140129908864cbf91536934812816e54463046f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 17 Jul 2022 23:03:31 -0700 Subject: [PATCH 2/6] Undo the change in tests/test_exceptions.py I turns out (I forgot) that PyPy segfaults in `test_flaky_exception_failure_point_init` already before the `MISMATCH` code path is reached: https://github.com/pybind/pybind11/runs/7383663596 ``` RPython traceback: test_exceptions.py .......X.........Error in cpyext, CPython compatibility layer: File "pypy_module_cpyext.c", line 14052, in wrapper_second_level__star_3_1 File "pypy_module_cpyext_1.c", line 35750, in not_supposed_to_fail Fatal Python error: Segmentation fault Stack (most recent call first, approximate line numbers): File "/home/runner/work/pybind11/pybind11/tests/test_exceptions.py", line 306 in test_flaky_exception_failure_point_init The function PyErr_NormalizeException was not supposed to fail File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/_pytest/python.py", line 185 in pytest_pyfunc_call File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pluggy/_callers.py", line 9 in _multicall File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pluggy/_manager.py", line 77 in _hookexec File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pluggy/_hooks.py", line 244 in __call__ File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/_pytest/python.py", line 1716 in runtest File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/_pytest/runner.py", line 159 in pytest_runtest_call File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pluggy/_callers.py", line 9 in _multicall File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pluggy/_manager.py", line 77 in _hookexec Fatal error in cpyext, CPython compatibility layer, calling PyErr_NormalizeException Either report a bug or consider not using this particular extension File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pluggy/_hooks.py", line 244 in __call__ File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/_pytest/runner.py", line 261 in File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/_pytest/runner.py", line 317 in from_call File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/_pytest/runner.py", line 246 in call_runtest_hook File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/_pytest/runner.py", line 218 in call_and_report File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/_pytest/runner.py", line 118 in runtestprotocol File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/_pytest/runner.py", line 110 in pytest_runtest_protocol File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pluggy/_callers.py", line 9 in _multicall File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pluggy/_manager.py", line 77 in _hookexec File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pluggy/_hooks.py", line 244 in __call__ File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/_pytest/main.py", line 335 in pytest_runtestloop File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pluggy/_callers.py", line 9 in _multicall File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pluggy/_manager.py", line 77 in _hookexec File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pluggy/_hooks.py", line 244 in __call__ File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/_pytest/main.py", line 318 in _main File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/_pytest/main.py", line 255 in wrap_session File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/_pytest/main.py", line 314 in pytest_cmdline_main File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pluggy/_callers.py", line 9 in _multicall File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pluggy/_manager.py", line 77 in _hookexec File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pluggy/_hooks.py", line 244 in __call__ File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/_pytest/config/__init__.py", line 133 in main File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/_pytest/config/__init__.py", line 181 in console_main File "/opt/hostedtoolcache/PyPy/3.7.13/x64/site-packages/pytest/__main__.py", line 1 in File "/opt/hostedtoolcache/PyPy/3.7.13/x64/lib-python/3/runpy.py", line 62 in _run_code File "/opt/hostedtoolcache/PyPy/3.7.13/x64/lib-python/3/runpy.py", line 170 in _run_module_as_main File "/app_main.py", line 109 in run_toplevel File "/app_main.py", line 652 in run_command_line File "/app_main.py", line 996 in entry_point Segmentation fault (core dumped) ``` --- tests/test_exceptions.py | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index f39cf946de..a5984a142f 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -303,28 +303,18 @@ def test_error_already_set_what_with_happy_exceptions( assert what == expected_what +@pytest.mark.skipif("env.PYPY", reason="PyErr_NormalizeException Segmentation fault") def test_flaky_exception_failure_point_init(): - if env.PYPY: - # This behavior masks errors in the error handling, but avoids a PyPy segfault (root - # cause unknown). See https://github.com/pybind/pybind11/issues/4075 for background. - what, py_err_set_after_what = m.error_already_set_what( - FlakyException, ("failure_point_init",) - ) - assert not py_err_set_after_what - lines = what.splitlines() - # PyErr_NormalizeException replaces the original FlakyException with ValueError: - assert lines[:3] == ["ValueError: triggered_failure_point_init", "", "At:"] - else: - with pytest.raises(RuntimeError) as excinfo: - m.error_already_set_what(FlakyException, ("failure_point_init",)) - lines = str(excinfo.value).splitlines() - # PyErr_NormalizeException replaces the original FlakyException with ValueError: - assert lines[:3] == [ - "pybind11::error_already_set: MISMATCH of original and normalized active exception types:" - " ORIGINAL FlakyException REPLACED BY ValueError: triggered_failure_point_init", - "", - "At:", - ] + with pytest.raises(RuntimeError) as excinfo: + m.error_already_set_what(FlakyException, ("failure_point_init",)) + lines = str(excinfo.value).splitlines() + # PyErr_NormalizeException replaces the original FlakyException with ValueError: + assert lines[:3] == [ + "pybind11::error_already_set: MISMATCH of original and normalized active exception types:" + " ORIGINAL FlakyException REPLACED BY ValueError: triggered_failure_point_init", + "", + "At:", + ] # Checking the first two lines of the traceback as formatted in error_string(): assert "test_exceptions.py(" in lines[3] assert lines[3].endswith("): __init__") From 1420cf4993b67f6732cf78a11de984f67d98ae37 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 18 Jul 2022 00:14:39 -0700 Subject: [PATCH 3/6] Add test_pypy_oserror_normalization --- tests/test_exceptions.cpp | 10 ++++++++++ tests/test_exceptions.py | 6 ++++++ 2 files changed, 16 insertions(+) diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index 3ec999d1dc..3583f22a50 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -334,4 +334,14 @@ TEST_SUBMODULE(exceptions, m) { e.restore(); } }); + + // https://github.com/pybind/pybind11/issues/4075 + m.def("test_pypy_oserror_normalization", []() { + try { + py::module_::import("io").attr("open")("this_filename_must_not_exist", "r"); + } catch (const py::error_already_set &e) { + return py::str(e.what()); // str must be built before e goes out of scope. + } + return py::str("UNEXPECTED"); + }); } diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index a5984a142f..5e3beeedbc 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -360,3 +360,9 @@ def test_error_already_set_double_restore(): "Internal error: pybind11::detail::error_fetch_and_normalize::restore()" " called a second time. ORIGINAL ERROR: ValueError: Random error." ) + + +def test_pypy_oserror_normalization(): + # https://github.com/pybind/pybind11/issues/4075 + what = m.test_pypy_oserror_normalization() + assert "this_filename_must_not_exist" in what From 774472f14568eaea5a46d9d5aa1ba01151c26d3e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 18 Jul 2022 00:54:25 -0700 Subject: [PATCH 4/6] Disable new `PYPY_VERSION` `#if`, to verify that the new test actually fails. --- include/pybind11/pytypes.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 93146ee274..a0625ddfb1 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -456,7 +456,7 @@ struct error_fetch_and_normalize { + " failed to obtain the name " "of the normalized active exception type."); } -#if defined(PYPY_VERSION) +#if defined(YPYP_EVSROIN) // This behavior masks errors in the error handling, but avoids a PyPy segfault (root // cause unknown). See https://github.com/pybind/pybind11/issues/4075 for background. m_lazy_error_string = exc_type_name_norm; From 3f30a98cef74bd3d073606b8ea7ff0af5c1547a3 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 19 Jul 2022 13:21:55 -0700 Subject: [PATCH 5/6] Restore PYPY_VERSION workaround and update comment to reflect what was learned. --- include/pybind11/pytypes.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index a0625ddfb1..57a8da8bf1 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -456,9 +456,10 @@ struct error_fetch_and_normalize { + " failed to obtain the name " "of the normalized active exception type."); } -#if defined(YPYP_EVSROIN) - // This behavior masks errors in the error handling, but avoids a PyPy segfault (root - // cause unknown). See https://github.com/pybind/pybind11/issues/4075 for background. +#if defined(PYPY_VERSION) + // This behavior runs the risk of masking errors in the error handling, but avoids a + // conflict with PyPy, which relies on the normalization here to change OSError to + // FileNotFoundError (https://github.com/pybind/pybind11/issues/4075 for background). m_lazy_error_string = exc_type_name_norm; #else if (exc_type_name_norm != m_lazy_error_string) { From 15a3be32d394e0ea3fb53c2156d4eb457c19c440 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 19 Jul 2022 14:31:10 -0700 Subject: [PATCH 6/6] [ci skip] Fix trivial oversight in comment. --- include/pybind11/pytypes.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 57a8da8bf1..3fdfd0689a 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -459,7 +459,7 @@ struct error_fetch_and_normalize { #if defined(PYPY_VERSION) // This behavior runs the risk of masking errors in the error handling, but avoids a // conflict with PyPy, which relies on the normalization here to change OSError to - // FileNotFoundError (https://github.com/pybind/pybind11/issues/4075 for background). + // FileNotFoundError (https://github.com/pybind/pybind11/issues/4075). m_lazy_error_string = exc_type_name_norm; #else if (exc_type_name_norm != m_lazy_error_string) {