[smart_holder] Bug fix: `std::unique_ptr` deleter needs to be copied. #4850

rwgk · 2023-09-19T03:39:05Z

Description

IMPORTANT: Quite possibly follow-on work is needed. See comments for details, in particular #4850 (comment) below.

Root cause for this bug: Firmly held wrong belief that std::unique_ptr does not store a deleter.

But here it is:

https://en.cppreference.com/w/cpp/memory/unique_ptr/get_deleter

Bug detected via a few failing tests with PyCLIF-pybind11 (out of many millions).

Suggested changelog entry:

Using a `union` is complicated: https://en.cppreference.com/w/cpp/language/union > If members of a union are classes with user-defined constructors and destructors, to switch the active member, explicit destructor and placement new are generally needed: Using `std::variant` increases compile-time overhead. It is currently unclear how much these effects matter in practice: optimization left for later.

``` -- The CXX compiler identification is Clang 15.0.7 /usr/bin/cmake -E __run_co_compile --tidy="/usr/bin/clang-tidy;--use-color;--warnings-as-errors=*;--extra-arg-before=--driver-mode=g++" --source=/__w/pybind11/pybind11/tests/test_class_sh_inheritance.cpp -- /usr/bin/c++ -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_EIGEN -Dpybind11_tests_EXPORTS -I/__w/pybind11/pybind11/include -isystem /usr/include/python3.9 -isystem /__w/pybind11/pybind11/build/_deps/eigen-src -Os -DNDEBUG -fPIC -fvisibility=hidden -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -flto=thin -std=c++17 -o CMakeFiles/pybind11_tests.dir/test_class_sh_inheritance.cpp.o -c /__w/pybind11/pybind11/tests/test_class_sh_inheritance.cpp /__w/pybind11/pybind11/tests/pure_cpp/smart_holder_poc_test.cpp:264:30: error: pointer parameter 'raw_ptr' can be pointer to const [readability-non-const-parameter,-warnings-as-errors] new int(19), [](int *raw_ptr) { delete raw_ptr; }); ^ const ```

…esired (the lambda function only captures a reference, which can become dangling).

``` /usr/bin/cmake -E __run_co_compile --tidy="/usr/bin/clang-tidy;--use-color;--warnings-as-errors=*;--extra-arg-before=--driver-mode=g++" --source=/__w/pybind11/pybind11/tests/test_class.cpp -- /usr/bin/c++ -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_EIGEN -Dpybind11_tests_EXPORTS -I/__w/pybind11/pybind11/include -isystem /usr/include/python3.9 -isystem /__w/pybind11/pybind11/build/_deps/eigen-src -Os -DNDEBUG -fPIC -fvisibility=hidden -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -flto=thin -std=c++17 -o CMakeFiles/pybind11_tests.dir/test_class.cpp.o -c /__w/pybind11/pybind11/tests/test_class.cpp /__w/pybind11/pybind11/include/pybind11/detail/smart_holder_poc.h:114:5: error: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor,-warnings-as-errors] custom_deleter(D &&deleter) : deleter{std::move(deleter)} {} ^ explicit /__w/pybind11/pybind11/include/pybind11/detail/smart_holder_poc.h:120:76: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors] return guarded_delete(std::function<void(void *)>(custom_deleter<T, D>(std::move(uqp_del))), ^~~~~~~~~ std::forward<D> ```

@wangxf123456

Reduced from a PyCLIF use case in the wild by @wangxf123456 (internal change cl/565476030).

rwgk · 2023-11-01T07:28:03Z

Hi @iwanders,

I'm replying here to your message under rwgk#26.

When I saw your message I decided it's best to first transfer a simple existing internal-only test to a new test_class_sh_unique_ptr_custom_deleter here, so that you see everything that exists already:

c684f6c

I was thinking that we probably need more tests in tests/pure_cpp/smart_holder_poc_test.cpp, and expand test_class_sh_unique_ptr_custom_deleter. But I'm very uncertain myself what & how exactly.

Things that come to mind as maybe relevant:

It's not obvious, but some code in smart_holder_poc.h turned out to not be usable like that from smart_holder_type_casters.h (when I worked on it originally a couple years ago). It's only used by tests/pure_cpp/smart_holder_poc_test.cpp. One of the things I always wanted to do but never got to: move all code that is not used from smart_holder_type_casters.h to the tests/pure_cpp/ directory, so that only the production code is left in smart_holder_poc.h. The test-only code in smart_holder_poc.h is probably more distracting than helpful for your purposes.

I was thinking of the smart_holder_poc.h code under this PR as ~complete: it works for Google-internal global testing and solved in-the-wild test failures (17 tests, probably 3-5 unique use cases). Therefore I'm surprised that you're adding code in smart_holder_poc.h.

What do you think about this approach to collaborating:

Could you help by reviewing this PR? — With the idea that we want to ensure this PR by itself is healthy, but we don't want to add much.
Then you could expand the test coverage more systematically (I'd have suggestions) in a follow-on PR, also here (main pybind11 repo, not my private one, which only has the small free GitHub Actions quota).

iwanders · 2023-11-01T11:33:21Z

I was thinking of the smart_holder_poc.h code under this PR as ~complete: it works for Google-internal global testing and solved in-the-wild test failures (17 tests, probably 3-5 unique use cases). Therefore I'm surprised that you're adding code in smart_holder_poc.h.

Hmm, I may be missing something, but I was assuming a custom deleter should survive a roundtrip from unique_ptr -> smart_holder -> unique_ptr.

Currently it doesn't:

The from_unique_ptr method moves the deleter from the unique pointer to the smart holder.
The counterpart; as_unique_ptr method does not copy the deleter from the smart holder back into the unique pointer?

Github won't let me comment on the appropriate line, but I feel this current PR only fixes the bug in one direction (going into smart holder), not in the direction of going out of the smart holder. This test shows the problem, it will fail with this branch and unmodified code, which is why I was modifying the smart holder file.

Edit;

Could you help by reviewing this PR? — With the idea that we want to ensure this PR by itself is healthy, but we don't want to add much.

Went through it, overall it looks fine to me, my main concern is the above statement that the deleter is only handled when going into the smart holder, not when being converted back to a unique pointer.

What do you think about this approach to collaborating: [...] Then you could expand the test coverage more systematically (I'd have suggestions) in a follow-on PR

Sure, I can't commit a lot of time, but happy to help over the next week or so.

iwanders · 2023-11-01T11:38:08Z

include/pybind11/detail/smart_holder_poc.h

-}
+struct custom_deleter {
+    D deleter;
+    explicit custom_deleter(D &&deleter) : deleter{std::move(deleter)} {}


This is always tricky, but shouldn't this std::move be a std::forward instead? THe D&& could be a universal reference to an lvalue I think, in which case moving it may not have the intended effect at the call site, while forward will move if it is an rvalue but copy if it is an lvalue?

Edit; so make this like line 122.

Changed, thanks for catching this!

This is always tricky,

Yeah, I wasn't paying enough attention here before.

iwanders · 2023-11-01T11:51:12Z

tests/test_class_sh_unique_ptr_custom_deleter.cpp

+} // namespace class_sh_unique_ptr_custom_deleter
+} // namespace pybind11_tests
+
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_unique_ptr_custom_deleter::Pet)


(Feel free to ignore) This line missing a semicolon trips up the syntax highlighting on github, but it seems to be in line with the other tests. Could add a do {} while(0) at the end of the macro to enforce the ; at the end of the line. But I think that requires a lot of changes in other unit tests as well.

"Too late", unfortunately: I just looked, I'd have to change 35 files around the Google codebase, outside the pybind11 source directory. It'll set me back a whole day or two worth of effort. And there are so many other far more substantial things wishing for those days.

iwanders · 2023-11-01T11:52:39Z

tests/test_class_sh_unique_ptr_custom_deleter.py

+
+def test_create():
+    pet = m.create("abc")
+    assert pet.name == "abc"


This unit test doesn't actually test the deleter is ran, unless ran inside valgrind / lsan, is that intentional?

This unit test doesn't actually test the deleter is ran, unless ran inside valgrind / lsan, is that intentional?

@wangxf123456 was completely focused on reducing a test from the wild, just enough to reproduce the error message we saw.

Copy-pasting what @iwanders wrote in another comment:

my main concern is the above statement that the deleter is only handled when going into the smart holder, not when being converted back to a unique pointer.

I agree. I was also completely focused on the issue in the wild. — Reporting a thought/uncertainty in the back of my mind: Do we actually have to solve this (is that feature actually needed in the wild)? Or would it be sufficient to clearly report an error if someone tried to use the non-existing feature? And then work on it as needed.

I'll add a note about it to the PR description, then merge this PR when I see that the GHA are happy.

Sure, I can't commit a lot of time, but happy to help over the next week or so.

I really appreciate any help! And I'm a big believer in small steps. I want to encourage you to pick out one thing at a time. Solve it. Merge. Next one. (My goal, in general, is to review PRs within hours; highest priority.)

Or would it be sufficient to clearly report an error if someone tried to use the non-existing feature? And then work on it as needed.

Yeah, tricky, it's a balance. I'd at least throw in case there's a non default deleter detected in as_unique_ptr, that way it explicitly fails, instead of implicitly losing the deleter and leaking memory, and that should still be doable while avoiding most of the extra code I had to add to account for it.

Speaking of as_unique_ptr, another random thought; that method could be marked &&, so std::unique_ptr<T, D> as_unique_ptr() &&, that ensures it can only be called on an r-value smart_holder, which makes it clear that the smart_holder object after the method call is in an indeterminate state. In that case it would be illegal to call as_unique_ptr on an lvalue though, so it makes it a bit more cumbersome at the callsite, but it does draw more attention to what it is doing. Not saying this should be done, but just stating it is an option to make it clear we are moving out of the smart holder.

I'll add a note about it to the PR description, then merge this PR when I see that the GHA are happy.

Sounds good.

Ah I see that as_unique_ptr was also wrapped with PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP; 100% agree it's probably not worth worrying about the deleter there then 👍

…e `smart_holder::as_unique_ptr` method from production code.

rwgk · 2023-11-01T15:22:15Z

@iwanders I found a simple trick to make it obvious what is and isn't used in production: 4d9d722

I need to do something else right now, but will come back here to similarly ifdef-out all smart_holder_poc.h code that is not actually used in production code.

rwgk · 2023-11-01T16:19:29Z

Side remark:

Something weird is happening with this test:

..\..\tests\test_type_caster_odr_guard_1.py ..FF                         [ 95%]

It seems to affect only windows-2022 builds. Un/Related? I need to find out.

(There is one test_iostream flake mixed in.)

…` to mark code that is not used from production code. Add comment to explain.

rwgk · 2023-11-01T21:59:09Z

Speaking of as_unique_ptr, another random thought; that method could be marked &&, so std::unique_ptr<T, D> as_unique_ptr() &&, that ensures it can only be called on an r-value smart_holder, which makes it clear that the smart_holder object after the method call is in an indeterminate state.

Hm ... IIUC that's not in line with my thinking behind the code. I wanted to leave the smart_holder in an well-defined state at all times. I believe it has to, because we have to assume it lives in a Python object until that is destroyed.

Your very good round-trip question seems much more important in the context of smart_holder_type_casters.h: is that actually working as intended?

I'm thinking we need something like these:

pybind11/tests/test_class_sh_basic.cpp

Lines 58 to 65 in e955753

    
           struct sddm : std::default_delete<atyp      > {}; 
        
           struct sddc : std::default_delete<atyp const> {}; 
        
           std::unique_ptr<atyp,       sddm> rtrn_udmp() { return std::unique_ptr<atyp,       sddm>(new atyp{"rtrn_udmp"}); } 
        
           std::unique_ptr<atyp const, sddc> rtrn_udcp() { return std::unique_ptr<atyp const, sddc>(new atyp{"rtrn_udcp"}); } 
        
           std::string pass_udmp(std::unique_ptr<atyp,       sddm> obj) { return "pass_udmp:" + obj->mtxt; } 
        
           std::string pass_udcp(std::unique_ptr<atyp const, sddc> obj) { return "pass_udcp:" + obj->mtxt; }

Only more sophisticated to establish firmly that the deleter is copied properly.

rwgk · 2023-11-01T22:31:54Z

I think this PR is in good shape for merging now, but I'm mystified by the persistent type_caster_odr test failures with windows-2022. I need to get to the bottom of those first.

I'll use that time to rerun Google global testing for this PR, although the only change is the std::move / std::forward replacement.

iwanders · 2023-11-01T22:34:48Z

Hm ... IIUC that's not in line with my thinking behind the code. I wanted to leave the smart_holder in an well-defined state at all times. I believe it has to, because we have to assume it lives in a Python object until that is destroyed.

Ah, sorry, I worded this ambiguous. You're right it will be in a well defined state, I was mostly concerned about the semantics, to me the as_unique_ptr consumes the smart holder and it would make sense to me to require an rvalue to allow that. On second look I see is_populated is never set to false, except through the default constructor, it's not set to false by as_unique_ptr, but again, because it's only used by the unit tests lets leave it ;)

Only more sophisticated to establish firmly that the deleter is copied properly.

Hmm, still trying to wrap my head around that file as an outsider xD. Easiest way I'd test it is by making a destructor that has a side effect (sets a boolean on call), then perform the round trip and ensure the deleter is only called after the final value goes out of scope.

Does the `PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP` define have anything to do with it?

rwgk · 2023-11-01T22:46:59Z

Easiest way I'd test it is by making a destructor that has a side effect (sets a boolean on call), then perform the round trip and ensure the deleter is only called after the final value goes out of scope.

That sounds good!

I recommend waiting until this PR is merged, so that we can collaborate more easily. I'll try to get to the bottom of the type_caster_odr_guard failures with highest priority.

rwgk · 2023-11-01T23:58:35Z

Certainly, the windows-2022 failures are due to changes in the environment, unrelated to this PR:

[smart_holder] Another super simple experiment trying to get to the bottom of the type_caster_odr_guard test failures with windows-2022 #4914

That ran with smart_holder head, without any code changes.

6 windows-2022 failures, 5 are type_caster_odr_guard failures, one test_iostream flake (which is very common).

I'll revert my latest commit here, then merge. I'll work on dealing with the type_caster_odr_guard failures separately.

This reverts commit fe59369.

rwgk · 2023-11-02T15:20:59Z

For easy future reference, global testing internal ID: OCL:566559299:BASE:578853944:1698937167027:61af92f8 (passed)

rwgk · 2023-11-02T16:31:16Z

@iwanders FYI I'll merge from master into smart_holder, too. Should be done in an hour or so. Doesn't change much I guess, just so you're not surprised.

rwgk · 2023-11-02T18:33:45Z

FYI I'll merge from master into smart_holder

Done.

iwanders · 2023-11-02T23:08:50Z

@rwgk , thanks, I haven't had time to look at it yet, but wanted to let you know I'll try to make that deleter roundtrip unit test over the weekend. I have to take some time to read up on how the smart holder actually works and how it interacts with unique pointers or deleters first :)

rwgk · 2023-11-02T23:30:44Z

read up on how the smart holder actually works

I'd be happy to meet and go through the code together. If you're interested, please get in touch via email (in git log). (I looked but couldn't find your direct email.)

rwgk added 7 commits September 18, 2023 19:35

Store std::function<void (void *)> del_fun; in guarded_delete

f14846d

Add one test case (more later).

884fe1c

Introduce struct custom_deleter to ensure the deleter is moved as d…

17098eb

…esired (the lambda function only captures a reference, which can become dangling).

Workaround for gcc 4.8.5, clang 3.6

d815d7d

rwgk mentioned this pull request Oct 22, 2023

Attach python lifetime to shared_ptr passed to C++ #2839

Open

rwgk added 2 commits October 31, 2023 23:28

Merge branch 'smart_holder' into unique_ptr_deleter_sh

4dbe657

Transfer reduced test here.

c684f6c

Reduced from a PyCLIF use case in the wild by @wangxf123456 (internal change cl/565476030).

Add missing include (clangd Include Cleaner)

a685d57

iwanders reviewed Nov 1, 2023

View reviewed changes

rwgk added 3 commits November 1, 2023 07:08

Change std::move to std::forward as suggested by @iwanders.

8a973d4

Add missing includes (clangd Include Cleaner)

4fb7d12

Use new PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP to exclud…

4d9d722

…e `smart_holder::as_unique_ptr` method from production code.

Systematically add `PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP…

7669a5b

…` to mark code that is not used from production code. Add comment to explain.

Very simple experiment related to pybind#4850 (comment)

fe59369

Does the `PYBIND11_TESTS_PURE_CPP_SMART_HOLDER_POC_TEST_CPP` define have anything to do with it?

rwgk mentioned this pull request Nov 1, 2023

[smart_holder] Another super simple experiment trying to get to the bottom of the type_caster_odr_guard test failures with windows-2022 #4914

Closed

Revert "Very simple experiment related to pybind#4850 (comment)"

821e13a

This reverts commit fe59369.

rwgk marked this pull request as ready for review November 2, 2023 15:21

rwgk requested a review from henryiii as a code owner November 2, 2023 15:21

rwgk merged commit 55105fb into pybind:smart_holder Nov 2, 2023
147 of 152 checks passed

github-actions bot added the needs changelog Possibly needs a changelog entry label Nov 2, 2023

rwgk removed the needs changelog Possibly needs a changelog entry label Nov 2, 2023

rwgk mentioned this pull request Nov 2, 2023

git merge smart_holder (pybind/pybind11#4917) google/pybind11k#30075

Merged

This was referenced Nov 3, 2023

std::unique_ptr deleter unit tests. rwgk/pybind11#26

Closed

[smart_holder] Unique ptr deleter roundtrip tests and fix #4921

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[smart_holder] Bug fix: `std::unique_ptr` deleter needs to be copied. #4850

[smart_holder] Bug fix: `std::unique_ptr` deleter needs to be copied. #4850

rwgk commented Sep 19, 2023 •

edited

Loading

rwgk commented Nov 1, 2023

iwanders commented Nov 1, 2023 •

edited

Loading

iwanders Nov 1, 2023 •

edited

Loading

rwgk Nov 1, 2023

iwanders Nov 1, 2023

rwgk Nov 1, 2023

iwanders Nov 1, 2023

rwgk Nov 1, 2023

iwanders Nov 1, 2023

iwanders Nov 1, 2023

rwgk commented Nov 1, 2023

rwgk commented Nov 1, 2023

rwgk commented Nov 1, 2023 •

edited

Loading

rwgk commented Nov 1, 2023

iwanders commented Nov 1, 2023

rwgk commented Nov 1, 2023

rwgk commented Nov 1, 2023

rwgk commented Nov 2, 2023

rwgk commented Nov 2, 2023

rwgk commented Nov 2, 2023 •

edited

Loading

iwanders commented Nov 2, 2023

rwgk commented Nov 2, 2023

[smart_holder] Bug fix: std::unique_ptr deleter needs to be copied. #4850

[smart_holder] Bug fix: std::unique_ptr deleter needs to be copied. #4850

Conversation

rwgk commented Sep 19, 2023 • edited Loading

Description

Suggested changelog entry:

rwgk commented Nov 1, 2023

iwanders commented Nov 1, 2023 • edited Loading

iwanders Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

rwgk Nov 1, 2023

Choose a reason for hiding this comment

iwanders Nov 1, 2023

Choose a reason for hiding this comment

rwgk Nov 1, 2023

Choose a reason for hiding this comment

iwanders Nov 1, 2023

Choose a reason for hiding this comment

rwgk Nov 1, 2023

Choose a reason for hiding this comment

iwanders Nov 1, 2023

Choose a reason for hiding this comment

iwanders Nov 1, 2023

Choose a reason for hiding this comment

rwgk commented Nov 1, 2023

rwgk commented Nov 1, 2023

rwgk commented Nov 1, 2023 • edited Loading

rwgk commented Nov 1, 2023

iwanders commented Nov 1, 2023

rwgk commented Nov 1, 2023

rwgk commented Nov 1, 2023

rwgk commented Nov 2, 2023

rwgk commented Nov 2, 2023

rwgk commented Nov 2, 2023 • edited Loading

iwanders commented Nov 2, 2023

rwgk commented Nov 2, 2023

[smart_holder] Bug fix: `std::unique_ptr` deleter needs to be copied. #4850

[smart_holder] Bug fix: `std::unique_ptr` deleter needs to be copied. #4850

rwgk commented Sep 19, 2023 •

edited

Loading

iwanders commented Nov 1, 2023 •

edited

Loading

iwanders Nov 1, 2023 •

edited

Loading

rwgk commented Nov 1, 2023 •

edited

Loading

rwgk commented Nov 2, 2023 •

edited

Loading