From cb35a3c1432a5e0441aab4a62593fed82664abed Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 21 Jul 2022 06:38:00 -0700 Subject: [PATCH 1/2] For PyPy only, re-enable old behavior (runs the risk of masking bugs) (#4079) * 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 * 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) ``` * 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. --- include/pybind11/pytypes.h | 7 +++++++ tests/test_exceptions.cpp | 10 ++++++++++ tests/test_exceptions.py | 6 ++++++ 3 files changed, 23 insertions(+) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 7a731e22a..a837fb9fa 100644 --- 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 index 3ec999d1d..3583f22a5 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 a5984a142..5e3beeedb 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 9a2963734de309a6d3f4ffb3e43bb56713a7ccae Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 21 Jul 2022 06:40:34 -0700 Subject: [PATCH 2/2] More systematic gcc & clang coverage (#4083) * More systematic gcc coverage, based on https://github.com/pybind/pybind11/pull/4074#issuecomment-1188385580 * Fix complete fail. * Resolve GCC 11 & 12 "redundant move in return statement" warnings. * Also add clang 11, 12, 13 (to gather info for warning suppressions). * Add & use `PYBIND11_DETECTED_CLANG_WITH_MISLEADING_CALL_STD_MOVE_EXPLICITLY_WARNING` --- .github/workflows/ci.yml | 20 +++++++++++++------- include/pybind11/detail/common.h | 18 ++++++++++++++++++ include/pybind11/numpy.h | 11 +++++++++-- tests/test_kwargs_and_defaults.cpp | 11 ++++++++++- 4 files changed, 50 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 412282a4f..5a24ad6ab 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -281,6 +281,12 @@ jobs: std: 20 - clang: 10 std: 17 + - clang: 11 + std: 20 + - clang: 12 + std: 20 + - clang: 13 + std: 20 - clang: 14 std: 20 @@ -437,14 +443,14 @@ jobs: strategy: fail-fast: false matrix: - gcc: - - 7 - - latest - std: - - 11 include: - - gcc: 10 - std: 20 + - { gcc: 7, std: 11 } + - { gcc: 7, std: 17 } + - { gcc: 8, std: 14 } + - { gcc: 8, std: 17 } + - { gcc: 10, std: 17 } + - { gcc: 11, std: 20 } + - { gcc: 12, std: 20 } name: "🐍 3 • GCC ${{ matrix.gcc }} • C++${{ matrix.std }}• x64" container: "gcc:${{ matrix.gcc }}" diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 05370108f..9e6947daa 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -1158,6 +1158,24 @@ constexpr inline bool silence_msvc_c4127(bool cond) { return cond; } # define PYBIND11_SILENCE_MSVC_C4127(...) __VA_ARGS__ #endif +#if defined(__clang__) \ + && (defined(__apple_build_version__) /* AppleClang 13.0.0.13000029 was the only data point \ + available. */ \ + || (__clang_major__ >= 7 \ + && __clang_major__ <= 12) /* Clang 3, 5, 13, 14, 15 do not generate the warning. */ \ + ) +# define PYBIND11_DETECTED_CLANG_WITH_MISLEADING_CALL_STD_MOVE_EXPLICITLY_WARNING +// Example: +// tests/test_kwargs_and_defaults.cpp:46:68: error: local variable 'args' will be copied despite +// being returned by name [-Werror,-Wreturn-std-move] +// m.def("args_function", [](py::args args) -> py::tuple { return args; }); +// ^~~~ +// test_kwargs_and_defaults.cpp:46:68: note: call 'std::move' explicitly to avoid copying +// m.def("args_function", [](py::args args) -> py::tuple { return args; }); +// ^~~~ +// std::move(args) +#endif + // Pybind offers detailed error messages by default for all builts that are debug (through the // negation of ndebug). This can also be manually enabled by users, for any builds, through // defining PYBIND11_DETAILED_ERROR_MESSAGES. diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 4f82f1ec3..56e5edc5a 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -1865,9 +1865,13 @@ private: } auto result = returned_array::create(trivial, shape); +#ifdef PYBIND11_DETECTED_CLANG_WITH_MISLEADING_CALL_STD_MOVE_EXPLICITLY_WARNING +# pragma clang diagnostic push +# pragma clang diagnostic ignored "-Wreturn-std-move" +#endif if (size == 0) { - return std::move(result); + return result; } /* Call the function */ @@ -1878,7 +1882,10 @@ private: apply_trivial(buffers, params, mutable_data, size, i_seq, vi_seq, bi_seq); } - return std::move(result); + return result; +#ifdef PYBIND11_DETECTED_CLANG_WITH_MISLEADING_CALL_STD_MOVE_EXPLICITLY_WARNING +# pragma clang diagnostic pop +#endif } template diff --git a/tests/test_kwargs_and_defaults.cpp b/tests/test_kwargs_and_defaults.cpp index 7418afefb..2f3cabaf0 100644 --- a/tests/test_kwargs_and_defaults.cpp +++ b/tests/test_kwargs_and_defaults.cpp @@ -43,7 +43,16 @@ TEST_SUBMODULE(kwargs_and_defaults, m) { m.def("kw_func_udl_z", kw_func, "x"_a, "y"_a = 0); // test_args_and_kwargs - m.def("args_function", [](py::args args) -> py::tuple { return std::move(args); }); + m.def("args_function", [](py::args args) -> py::tuple { +#ifdef PYBIND11_DETECTED_CLANG_WITH_MISLEADING_CALL_STD_MOVE_EXPLICITLY_WARNING +# pragma clang diagnostic push +# pragma clang diagnostic ignored "-Wreturn-std-move" +#endif + return args; +#ifdef PYBIND11_DETECTED_CLANG_WITH_MISLEADING_CALL_STD_MOVE_EXPLICITLY_WARNING +# pragma clang diagnostic pop +#endif + }); m.def("args_kwargs_function", [](const py::args &args, const py::kwargs &kwargs) { return py::make_tuple(args, kwargs); });