Custom owning pointer

[YannickJadoul]

I find myself unable to create and register a smart pointer that would have the same basic functionality as std::unique_ptr. A returned smart pointer does not seem to be moved, and instead another one is created on the same raw pointer.

A small example:

#include <pybind11/pybind11.h>
#include <iostream>
#include "my_unique_ptr.h"

PYBIND11_DECLARE_HOLDER_TYPE(T, my_unique_ptr<T>);

namespace py = pybind11;

class A
{
public:
	A() { std::cout << "A::A() - " << (void *) this << std::endl; }
	~A() { std::cout << "A::~A() - " << (void *) this << std::endl; }
};

PYBIND11_PLUGIN(test) {
    py::module m("test");

    py::class_<A, my_unique_ptr<A>>(m, "A")
    	.def_static("make", [] () { return my_unique_ptr<A>(new A); });


    return m.ptr();
}

where my_unique_ptr.h is a copy of the GCC std::unique_ptr implementation, just to be sure there is nothing wrong with the actual smart pointer type I need to wrap: https://gist.github.com/YannickJadoul/fe5b318e9cbbd65a6e31ead7bad1599f (Please note that in my actual project, I am wrapping existing code by someone else, and there is not an obvious way of changing the custom smart pointer for a standard std::unique_ptr, although I certainly agree that could be infinitely better.)

As running test.A.make() results in the following output ...

A::A() - 0x28ec190
A::~A() - 0x28ec190
<parselmouth.A object at 0x7f1838361de0>

... you can see how quitting my Python interpreter afterwards results in a beautiful segfault.

So, am I misusing this nice pybind11 project and its smart pointers, or is this an unforeseen feature (aka bug)?

[jagerman]

The output doesn't indicate a problem: the 0x28e... is the C++ A instance, while the Python wrapper around this instance lives at 0x7f1.... Try something like:

    m.def("print_addr", [](A *a) { std::cout << (void *) a << std::endl; });

and then call it from the Python side: you should see the same address.

[YannickJadoul]

Fair point (though I do not want my A object to be copied if I return a pointer, do I?), but I'm afraid that does not shed another light on the case:

>>> a = test.A.make()
A::A() - 0xc24220
A::~A() - 0xc24220
>>> test.print_addr(a)
0xc24220
>>> 
A::~A() - 0xc241c0
Segmentation fault (core dumped)

Besides, I've been printing before what constructors of the smart pointer are being called, and I twice get a constructor invocation with a raw pointer, instead of a move (or copy, e.g. in the legacy auto_ptr case) constructor.

[dean0x7d]

I can reproduce the issue. Looks like a bug in pybind11, specifically with move-only holders that aren't exactly std::unique_ptr. I'll submit a fix in a little bit.

[YannickJadoul]

Awesome!

(For the record, with the old, deprecated std::auto_ptr, which did not have a move-constructor yet, the same thing happens. So it's not for 'move only types', but also for smart pointers that handle things in a copy constructor.)

[dean0x7d]

(For the record, with the old, deprecated std::auto_ptr, which did not have a move-constructor yet, the same thing happens. So it's not for 'move only types', but also for smart pointers that handle things in a copy constructor.)

Are you sure about std::auto_ptr? I just tried it briefly and it seemed to work fine. Not that I think the deprecated std::auto_ptr should be supported, but I'm just curious.

The move-only holder issue should be resolved by PR #607.

[YannickJadoul]

I do seem to be getting the same, yes, both with std::auto_ptr and with a custom TestPtr that inherits std::auto_ptr and has the same constructors:

template <class T>
class TestPtr : public std::auto_ptr<T>
{
public:
	TestPtr(T *t) : std::auto_ptr<T>(t) { std::cout << "TestPtr<T>::TestPtr<T>(T *) - " << (void *) this << std::endl; }
	TestPtr(TestPtr<T> &t) : std::auto_ptr<T>(t) { std::cout << "TestPtr<T>::TestPtr<T>(TestPtr<T> &) - " << (void *) this << std::endl; }
	TestPtr(TestPtr<T> &&t) : std::auto_ptr<T>(t) { std::cout << "TestPtr<T>::TestPtr<T>(TestPtr<T> &&) - " << (void *) this << std::endl; }
	~TestPtr() { std::cout << "TestPtr<T>::~TestPtr<T>() - " << (void *) this << std::endl; }
};

>>> a = test.A.make()
A::A() - 0x2007b80
TestPtr<T>::TestPtr<T>(T *) - 0x7ffc775334e0
TestPtr<T>::TestPtr<T>(T *) - 0x7ff9db7f6da8
TestPtr<T>::~TestPtr<T>() - 0x7ffc775334e0
A::~A() - 0x2007b80
>>> test.print_addr(a)
0x2007b80
>>> <Ctrl-D>
TestPtr<T>::~TestPtr<T>() - 0x7ff9db7f6da8
A::~A() - 0x2007b80

No explicit segfault, this time, but I think that's just lucky undefined behavior, because the destructor is called twice on the same object.

Do note that the 'copy constructor' of std::auto_ptr is not a true one, since it's argument is non-const. So that's maybe yet another reason why you maybe do not want to support it? Either way, I just wanted to give some extra context that might've been useful for a more general solution. But I agree that auto_ptr is pretty useless to support.

[dean0x7d]

You're right. I seem to have accidentally tested std::auto_ptr with the changes in PR #607 instead of the master branch. Indeed, it fails on master because std::is_copy_constructible<auto_ptr<T>> evaluates to false, so it follows the same code path as move-only holders. So it seems that PR #607 accidentally also adds support for std::auto_ptr... I'm not sure that's a good thing.

[YannickJadoul]

Somehow, there is a weird chain of dependencies and consequences there, isn't it? Because it is not copy-constructible, it will be a "move-only holder", but because there is no move constructor, this semi-copy-contructor std::auto_ptr(std::auto_ptr&) is called?
Related to that, what if you would have a smart pointer that can do two things? It can either move the ownership (SmartPtr<T>(SmartPtr<T>&&)), or it can make a (deep) copy of the pointee object (SmartPtr>T>(const SmartPtr<T>&). In the current solution in your branch, pybind11 would prefer the copy-ctr instead of the move-ctr, right? That somehow feels inconsistent with C++'s default way of preferring a move over a copy, and using the copy-ctr as a fall-back option for rvalue references.

[dean0x7d]

I agree that move constructors should be preferred, but there's a bit of a tradeoff there. Due to Python's dynamic typing, pybind11 relies on runtime type information to properly cast stuff back and forth. C++'s static type information (lvalue vs rvalue, const vs mutable) is lost in the process. This information needs to be encoded into runtime data as well. This is already done for the actual objects, but not for the object holders.

Most of the time this doesn't matter since most holders behave somewhat like std::unique_ptr or std::shared_ptr. The latter could benefit from using its move constructor, but copying is also pretty cheap compared to all the other runtime machinery that binds C++ and Python. That's true for most custom holders so I'm not sure optimizing for the corner case would be worth the code complication here.

[YannickJadoul]

Ok, fair enough, I don't have a good view on the pybind11 internals. And even if you're going from C++ to Python (with py::cast), as is the only thing implemented for unique_ptr (if I'm correct), don't still have the full type information to decide whether you can move or have to copy? Otherwise, it's maybe an interesting point to describe in the documentation, as I had to start playing around with examples to know the details of using smart pointers.

Either way, my immediate problem is solved; thank you very much for this quick solution!