For PyPy only, re-enable old behavior (runs the risk of masking bugs) (#4079)

rwgk · web-flow · commit cb35a3c1432a · 2022-07-21T06:38:00.000-07:00
* For PyPy only, re-enable old behavior (likely to mask bugs), to avoid segfault with unknown root cause. Change prompted by #4075 * 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 <SystemError object at 0x7fcc8cea6868> 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 <lambda> 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 <module> 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 "<builtin>/app_main.py", line 109 in run_toplevel File "<builtin>/app_main.py", line 652 in run_command_line File "<builtin>/app_main.py", line 996 in entry_point Segmentation fault (core dumped) ``` * Add test_pypy_oserror_normalization * Disable new `PYPY_VERSION` `#if`, to verify that the new test actually fails. * Restore PYPY_VERSION workaround and update comment to reflect what was learned. * [ci skip] Fix trivial oversight in comment.
diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h
@@ -473,6 +473,12 @@ struct error_fetch_and_normalize {
                           + " failed to obtain the name "
                             "of the normalized active exception type.");
         }
+#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).
+        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 "
@@ -484,6 +490,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.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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -473,6 +473,12 @@ struct error_fetch_and_normalize {`
`473`	`473`	`+ " failed to obtain the name "`
`474`	`474`	`"of the normalized active exception type.");`
`475`	`475`	`}`
	`476`	`+#if defined(PYPY_VERSION)`
	`477`	`+ // This behavior runs the risk of masking errors in the error handling, but avoids a`
	`478`	`+ // conflict with PyPy, which relies on the normalization here to change OSError to`
	`479`	`+ // FileNotFoundError (https://github.com/pybind/pybind11/issues/4075).`
	`480`	`+ m_lazy_error_string = exc_type_name_norm;`
	`481`	`+#else`
`476`	`482`	`if (exc_type_name_norm != m_lazy_error_string) {`
`477`	`483`	`std::string msg = std::string(called)`
`478`	`484`	`+ ": MISMATCH of original and normalized "`
`@@ -484,6 +490,7 @@ struct error_fetch_and_normalize {`
`484`	`490`	`msg += ": " + format_value_and_trace();`
`485`	`491`	`pybind11_fail(msg);`
`486`	`492`	`}`
	`493`	`+#endif`
`487`	`494`	`}`
`488`	`495`
`489`	`496`	`error_fetch_and_normalize(const error_fetch_and_normalize &) = delete;`