Test smart_holder in virtual inheritance #5796
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
After you push the clang-tidy fix (remove the redundant |
|
From https://chatgpt.com/share/689ee2ee-514c-8008-9d6b-2566dec08a6e "the reproducer only triggers under MSVC x64" @MannixYang Did you already try to get a backtrace? That'll probably help enormously in homing in on the root cause. (ChatGPT has a few guesses.) |
|
@rwgk Sorry for replying to you so late. |
|
I had a C++ backtrace in mind (not a Python backtrace). I haven't used a C++ debugger on Windows for ... forever. Not sure when I'll get to it. If anyone seeing this is set up for Windows debugging: Could you please help? |
Merged
|
One step further (but not a fix yet): This I how I built: This is crashing as expected: I managed to get a C++ traceback, with a lot of help from ChatGPT 5 Pro: At the Full output (long): |
…shared_ptr(): new test passes, but test_shared_ptr_alias_nonpython breaks, test_iostream.py has failures. ``` ..\..\tests\test_iostream.py:255: AssertionError ================================================================ test session starts ================================================================ platform win32 -- Python 3.13.7, pytest-8.4.1, pluggy-1.6.0 installed packages of interest: build==1.3.0 numpy==2.2.6 C++ Info: MSVC 194435211 C++17 __pybind11_internals_v11_msvc_md_mscver19_debug__ PYBIND11_SIMPLE_GIL_MANAGEMENT=False rootdir: C:\Users\rgrossekunst\forked\pybind11\tests configfile: pytest.ini plugins: timeout-2.4.0 collected 1281 items ..\..\tests\test_animal_cat_tiger.py . [ 0%] ..\..\tests\test_async.py .. [ 0%] ... <cut> ... ============================================================== short test summary info ============================================================== SKIPPED [1] ..\..\tests\test_buffers.py:56: cpp_name=`long double`: `long double` and `double` have same size. SKIPPED [1] ..\..\tests\test_buffers.py:56: cpp_name=`std::complex<long double>`: `long double` and `double` have same size. SKIPPED [24] ..\..\tests\test_chrono.py:80: TZ environment variable only supported on POSIX SKIPPED [1] ..\..\tests\test_eigen_matrix.py:754: could not import 'scipy': No module named 'scipy' SKIPPED [1] ..\..\tests\test_eigen_matrix.py:764: could not import 'scipy': No module named 'scipy' SKIPPED [1] ..\..\tests\test_multiple_interpreters.py:127: Requires 3.14.0b3+ SKIPPED [1] ..\..\tests\test_pytypes.py:1044: C++20 non-type template args feature not available. SKIPPED [1] ..\..\tests\test_pytypes.py:1088: C++20 non-type template args feature not available. SKIPPED [3] ..\..\tests\test_pytypes.py:1103: <ranges> not available. SKIPPED [3] ..\..\tests\test_pytypes.py:1116: <ranges> not available. SKIPPED [3] ..\..\tests\test_pytypes.py:1128: <ranges> not available. SKIPPED [1] ..\..\tests\test_scoped_critical_section.py:15: no <barrier> SKIPPED [1] ..\..\tests\test_scoped_critical_section.py:21: no <barrier> SKIPPED [1] ..\..\tests\test_scoped_critical_section.py:27: no <barrier> SKIPPED [1] ..\..\tests\test_stl.py:168: no <experimental/optional> SKIPPED [1] ..\..\tests\test_stl.py:200: no <boost/optional> FAILED ..\..\tests\test_iostream.py::test_not_captured - AssertionError: assert '' == 'Something th...how up in log' FAILED ..\..\tests\test_iostream.py::test_err - AssertionError: assert '' == 'Something th...how up in log' FAILED ..\..\tests\test_iostream.py::test_multi_captured - AssertionError: assert '' == 'bd' FAILED ..\..\tests\test_iostream.py::test_redirect - AssertionError: assert '' == 'Should not be in log!' FAILED ..\..\tests\test_iostream.py::test_redirect_err - AssertionError: assert '' == 'StdOut' ==================================================== 5 failed, 1231 passed, 45 skipped in 27.68s ==================================================== Batch file failed at line 3 with errorcode 1 FAILED: [code=1] tests/CMakeFiles/pytest C:/Users/rgrossekunst/forked/pybind11/build/tests/CMakeFiles/pytest tests\CMakeFiles\pytest-e16405e.bat 69c50e2372e6acf3 ninja: build stopped: subcommand failed. ```
|
After several hours of debugging: commit 41c37a3 pin-points a definite problem. See commit message for more information. I just triggered the CI to see if the And to see what still works in general. Worth noting:
This is because MSVC handles multiple inheritance very differently than other compilers. Concretely: The The problem is in the pybind11 |
…2@v2 cannot be run twice anymore): https://github.com/pybind/pybind11/actions/runs/17394902023/job/49417376616?pr=5796 ``` Run msys2/setup-msys2@v2 with: msystem: mingw64 install: mingw-w64-x86_64-python-numpy mingw-w64-x86_64-python-scipy mingw-w64-x86_64-eigen3 path-type: minimal update: false pacboy: false release: true location: RUNNER_TEMP platform-check-severity: fatal cache: true env: PYTHONDEVMODE: 1 PIP_BREAK_SYSTEM_PACKAGES: 1 PIP_ONLY_BINARY: numpy FORCE_COLOR: 3 PYTEST_TIMEOUT: 300 VERBOSE: 1 CMAKE_COLOR_DIAGNOSTICS: 1 MSYSTEM: MINGW64 Error: Trying to install MSYS2 to D:\a\_temp\msys64 but that already exists, cannot continue. ```
…IND11_RUN_TESTING_WITH_SMART_HOLDER_AS_DEFAULT_BUT_NEVER_USE_IN_PRODUCTION_PLEASE
|
Quick comment: The lines touched by are there due to a misunderstanding of mine, probably from work very early on (~2021). It's quite amazing that these didn't trigger any test failures before. I still need to figure out proper fixes, that don't break the two existing tests. (It could take me another couple weeks before I find the time.) |
Thanks a lot for the quick follow-up and clarification! Really appreciate your time and effort. No worries about the timeline — totally understand it may take some time to sort out. |
…tup-msys2@v2 cannot be run twice anymore):" This reverts commit 44272c9.
…holder() Extracted from https://github.com/rwgk/pybind11/commits/animal_cat_tiger_dbgwip/ rwgk@b187437 with minor modifications.
|
Leaving some breadcrumbs for myself, to get up to speed faster when I come back here later:
|
|
Tracking an observation: Local diff against diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h
index 23d94073..a18256fe 100644
--- a/include/pybind11/detail/type_caster_base.h
+++ b/include/pybind11/detail/type_caster_base.h
@@ -604,7 +604,9 @@ handle smart_holder_from_unique_ptr(std::unique_ptr<T, D> &&src,
auto *inst_raw_ptr = reinterpret_cast<instance *>(inst.ptr());
inst_raw_ptr->owned = true;
void *&valueptr = values_and_holders(inst_raw_ptr).begin()->value_ptr();
- valueptr = src_raw_void_ptr;
+fflush(stderr); fprintf(stdout, "\nLOOOK smart_holder_from_unique_ptr\n"); fflush(stdout);
+ if (valueptr) {
+ }
if (static_cast<void *>(src.get()) == src_raw_void_ptr) {
// This is a multiple-inheritance situation that is incompatible with the current
@@ -671,7 +673,9 @@ handle smart_holder_from_shared_ptr(const std::shared_ptr<T> &src,
auto *inst_raw_ptr = reinterpret_cast<instance *>(inst.ptr());
inst_raw_ptr->owned = true;
void *&valueptr = values_and_holders(inst_raw_ptr).begin()->value_ptr();
- valueptr = src_raw_void_ptr;
+fflush(stderr); fprintf(stdout, "\nLOOOK smart_holder_from_shared_ptr\n"); fflush(stdout);
+ if (valueptr) {
+ }
auto smhldr
= smart_holder::from_shared_ptr(std::shared_ptr<void>(src, const_cast<void *>(st.first)));Those code paths are actually exercised by Sanity check: Do we still get the Access Violation after putting back the Next: Figure out why |
…version master again)
|
All tests pass with this proper bug fix: commit fada0cb Very obvious in hindsight. Not sure when/why I made that mistake; but I don't want to spend time digging deeper. It was correct in This kind of bug is what I was hoping to catch with I think I'll open a new PR to transfer the bug fix from here, and to try adding the animal-cat-tiger test into @MannixYang If you get a chance to cherry-pick the commit to try it our in your application, that'd be great. |
rwgk
added a commit
to rwgk/pybind11
that referenced
this pull request
Sep 14, 2025
|
Closing this in favor of #5836 Thanks a lot for the initial reproducer! |
rwgk
added a commit
that referenced
this pull request
Oct 1, 2025
…and `unique_ptr` to-Python conversions (#5836) * ChatGPT-generated diamond virtual-inheritance test case. * Report "virtual base at offset 0" but don't skip test. * Remove Left/Right virtual default dtors, to resolve clang-tidy errors: ``` /__w/pybind11/pybind11/tests/test_class_sh_mi_thunks.cpp:44:13: error: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override,-warnings-as-errors] 44 | virtual ~Left() = default; | ~~~~~~~ ^ | override /__w/pybind11/pybind11/tests/test_class_sh_mi_thunks.cpp:48:13: error: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override,-warnings-as-errors] 48 | virtual ~Right() = default; | ~~~~~~~ ^ | override ``` * Add assert(ptr) in register_instance_impl, deregister_instance_impl * Proper bug fix * Also exercise smart_holder_from_unique_ptr * [skip ci] ChatGPT-generated bug fix: smart_holder::from_unique_ptr() * Exception-safe ownership transfer from unique_ptr to shared_ptr ChatGPT: * shared_ptr’s ctor can throw (control-block alloc). Using get() keeps unique_ptr owning the memory if that happens, so no leak. * Only after the shared_ptr is successfully constructed do you release(), transferring ownership exactly once. * [skip ci] Rename alias_ptr to mi_subobject_ptr to distinguish from trampoline code (which often uses the term "alias", too) * [skip ci] Also exercise smart_holder::from_raw_ptr_take_ownership * [skip ci] Add st.first comments (generated by ChatGPT) * [skip ci] Copy and extend (raw_ptr, unique_ptr) reproducer from PR #5796 * Some polishing: comments, add back Left/Right dtors for consistency within test_class_sh_mi_thunks.cpp * explicitly default copy/move for VBase to silence -Wdeprecated-copy-with-dtor * Resolve clang-tidy error: ``` /__w/pybind11/pybind11/tests/test_class_sh_mi_thunks.cpp:67:5: error: 'auto ptr' can be declared as 'auto *ptr' [readability-qualified-auto,-warnings-as-errors] 67 | auto ptr = new Diamond; | ^~~~ | auto * ``` * Expand comment in `smart_holder::from_unique_ptr()` * Better Left/Right padding to make it more likely that we avoid "all at offset 0". Clarify comment. * Give up on `alignas(16)` to resolve MSVC warning: ``` "D:\a\pybind11\pybind11\build\ALL_BUILD.vcxproj" (default target) (1) -> "D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj" (default target) (13) -> (ClCompile target) -> D:\a\pybind11\pybind11\tests\test_class_sh_mi_thunks.cpp(70,17): warning C4316: 'test_class_sh_mi_thunks::Diamond': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] D:\a\pybind11\pybind11\tests\test_class_sh_mi_thunks.cpp(80,43): warning C4316: 'test_class_sh_mi_thunks::Diamond': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: 'std::_Ref_count_obj2<_Ty>': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: with [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: [ [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: _Ty=test_class_sh_mi_thunks::Diamond [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\memory(2913,46): warning C4316: ] [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] D:\a\pybind11\pybind11\include\pybind11\detail\init.h(77,21): warning C4316: 'test_class_sh_mi_thunks::Diamond': object allocated on the heap may not be aligned 16 [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] ``` The warning came from alignas(16) making Diamond over-aligned, while regular new/make_shared aren’t guaranteed to return 16-byte aligned memory on MSVC (hence C4316). I’ve removed the explicit alignment and switched to asymmetric payload sizes (char[4] vs char[24]), which still nudges MI layout without relying on over-alignment. This keeps the test goal and eliminates the warning across all MSVC builds. If we ever want to stress over-alignment explicitly, we can add aligned operator new/delete under __cpp_aligned_new, but that’s more than we need here. * Rename test_virtual_base_at_offset_0() → test_virtual_base_not_at_offset_0() and replace pytest.skip() with assert. Add helpful comment for future maintainers.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is a Test PR.
Related issues #5786 .
The error occurred at Visual Studio 17 2022 X64.
Suggested changelog entry: