Detect and fail if using mismatched holders

[rhaschke]

This is a rebase of #1161 augmented with a fix to handle mismatching holder types in function arguments and derived classes as well. All checks are performed at declaration time only. No runtime costs when calling functions!

[rhaschke]

The two commits e2aa4ec and 502bf24 realize the idea suggested in #1161 (comment) to store holder types locally in a type's record.
The advantage of this approach is that we don't need a global map that accumulates all seen types. I guess, if one loads a module using one holder type, then unloads this module, and finally loads another module using another holder type for the very same C++ type, the global map will still complain - for no reason.
However, the local storage approach has the drawback that we cannot check for holder compatibility before a type was registered, thus requiring the compatibility check at function call time instead of its initialization time.
Would be interesting to get some feedback on these pros and cons. @jagerman
EDIT: I resolved the drawback. See #2644 (comment).

Another shortcoming of the current implementation is the checking of holder compatibility for derived types. A derived class needs to use the same holder type as its base class(es). So far, the check was restricted to the default holder vs. a custom holder type:

pybind11/include/pybind11/attr.h

Line 289 in 06b673a

if (default_holder != base_info->default_holder) {

However, holder compatibility should be ensured in general, also between two custom holder types.
Thus, 7852e7d attempts to replace the simple check (based on the default_holder flag) with a more elaborate one, comparing holder type names. Unfortunately, I wasn't able to handle this via compile-time checks as the holder type info is only available as a std::type_info, not as a template type. For this reason, the check is limited to comparing stringified type names. Furthermore, I had to ignore any type details, e.g. deleters (cde5c11), to correctly handle a unit test changing the visibility of the destructor:

pybind11/tests/test_smart_ptr.cpp

Lines 207 to 212 in 06b673a

	// Object derived but with public destructor and no Deleter in default holder
	class MyObject4b : public MyObject4a {
	public:
	MyObject4b(int i) : MyObject4a(i) { print_created(this); }
	~MyObject4b() override { print_destroyed(this); }
	};

Maybe, a cleaner approach would be to implicitly auto-convert between holder types as suggested in #1161 (comment). However, this would add a lot of runtime overhead again. If desired, this can be handled by explicit wrappers for type conversion.

[rhaschke]

The local storage approach has the drawback that we cannot check for holder compatibility before a type was registered, thus requiring the compatibility check at function call time instead of its initialization time.

My last commit resolves this drawback by introducing the (new) constraint that a function's return type has to be registered before defining the function. As functions with an unregistered return type cannot be called anyway, I think this is a good compromise.

I think this is ready for review. Before merging, I will clean up the commit history (fixups), which I kept for now to ease the reviewing.

[rhaschke]

As pointed out by @YannickJadoul, the costs of runtime checks at function call time are undesirable. The latest commit moves the check to function declaration time.

[rwgk]

(Sorry I posted this under the wrong PR a minute ago.)

Hi @rhaschke , I looked through the changes, and to be totally honest, this PR is currently totally over my head. (I need to learn a lot more than I know at the moment to meaningfully review.)

But I tried to use this PR Google-internal. I didn't get past the initial manual testing stage, first running into this error:

ImportError: Cannot register function using not yet registered type 'proto2::Message'

I changed the corresponding code to move the class_ definition for proto2::Message up. Trying again I got:

ImportError: Detected mismatching holder types when declaring function '__copy__': attempting to use holder type std::__u::unique_ptr<proto2::Message, std::__u::default_delete<proto2::Message> >, but proto2::Message was declared using holder type std::__u::shared_ptr<proto2::Message>

I know these bindings are used in ~100 extensions and pass thousands if not 100s of thousands of tests. Is it plausible that so many tests run successfully with mismatched holders? (Or could there be a problem with the mismatch detection?)

I could dig in deeper, but wanted to check with you first.

(The affected code is open source in theory, but the open source repo (pybind11_protobuf) is behind and doesn't have a working build system.)

[YannickJadoul]

There's no conversion from a std::unique_ptr to std::shared_ptr in holders, right? (Ofc there is in C++, but I think types gets erased before that?)

[kenoslund-google]

Internally we added specialization of move_only_holder_caster for all protocol buffer types which promotes a unique_ptr to a shared_ptr. I guess the check code gets triggered before that, so the conversion doesn't have a chance to happen. This isn't a great solution, and we would definitely like a better one though.

I think a generic mechanism to convert holder types would be super useful. I didn't got through the PR in great detail, so apologies if I'm way off the mark here, but could you make a convert_holder_type class, which uses check_for_holder_mismatch as its default implementation, but can then be specialized for other conversions? This could then be used to automatically promote unique_ptrs to shared_ptrs when the class's holder is shared_ptr.

I'm curious, how does this PR handle the case when implicit conversions of the held value are possible? eg, one of the simplest cases would be the conversion from holder_type<Foo> to holder_type<const Foo>. A convert_holder_type mechanism could potentially handle that too.

[rhaschke]

Sorry for the delayed answer. I didn't receive emails the last days 😢

Trying again I got:

ImportError: Detected mismatching holder types when declaring function '__copy__': attempting to use holder type std::__u::unique_ptr<proto2::Message, std::__u::default_delete<proto2::Message> >, but proto2::Message was declared using holder type std::__u::shared_ptr<proto2::Message>

I know these bindings are used in ~100 extensions and pass thousands if not 100s of thousands of tests. Is it plausible that so many tests run successfully with mismatched holders?

Before this PR, pybind11 was extremely sloppy regarding holder type compatibility. It just reinterpreted the shared_ptr returned from the C++ function as a unique_ptr holder! This kind of works, because shared_ptr and unique_ptr share the same memory layout, i.e. the actual raw pointer is stored in the same memory location.

When I changed the memory layout of the smart pointer returned from C++ in this unit test, I've got a perfect segfault immediately.
So, there is a strong need for this PR.

Internally we added specialization of move_only_holder_caster for all protocol buffer types which promotes a unique_ptr to a shared_ptr. I guess the check code gets triggered before that, so the conversion doesn't have a chance to happen.

To minimize runtime costs, I explicitly moved the check from call time (when the caster would be called as well) to function definition time. To realize the automatic holder type conversion, @rwgk is aiming for, I think one should replace the new compatibility check and try to inject a wrapper function instead that converts between holder types (if necessary).
The key problem, which is the reason that I didn't realize the idea yet, is that the type of the registered custom type is not available to the compiler at function definition time. So, we cannot apply template magic. Rather, the type needs to be looked up at runtime via detail::get_type_info(typeid(base_type))->holder_type.

[rwgk]

So, we cannot apply template magic. Rather, the type needs to be looked up at runtime via detail::get_type_info(typeid(base_type))->holder_type

Do you have an idea how this could be implemented?

At first sight it looks similar to solving this problem:

https://stackoverflow.com/questions/4972795/how-do-i-typecast-with-type-info

I can only imagine something like a lookup table from the holders' std::type_info::name to objects with virtual functions that reinterpret_cast the pybind11::detail::value_and_holder::vh void * holder pointers ... but feel like that's getting way off a good path.

[rhaschke]

Do you have an idea how this could be implemented?

I thought about this a while the last days and came up with the idea of using templates: Let's assume the user declares an auto-converter like this:

template <typename IntrinsicType>
struct holder_converter_for {
  using holder_t = std::shared_ptr<IntrinsicType>;  // Explicitly remember the holder type for the compiler
  static holder_t cast(holder_t p) { return p; }  // pass-through converter
  static holder_t cast(std::unique_ptr<IntrinsicType> p) { return std::move(p); }
}

we could simply wrap all cast ops in function calls using this converter's methods. Using a type trait to just define the holder type for a custom type, we could maybe even use this template by default and users would only need to specialize if they want to support other auto conversions.
The basic idea is to have a fallback pass-through conversion, that just forwards the holder as is.
But, if the actual and registered holder types differ, a corresponding converter method is called. In the ideal case, the user would just need to explicitly declare (other than in the class_ definition) the holder types for his/her custom types.

Not sure though, this could really work out. For now, it's just a coarse idea.

[EricCousineau-TRI]

Nice! I've made some initial comments.

My recommendation is to defer the type conversion table, and maybe save that for a follow-up PR?

[EricCousineau-TRI]

nit To me, it looks like holder is really holder_caster. Should this be is_holder_caster?

[EricCousineau-TRI]

My brief reading of this PR leads me to believe that there's a sharp edge: a user could still hit a segfault if they call py::cast<mismatched_holder_type>(my_object). Totes get the idea of wanting to shift overhead from py::cast<>() (runtime) to function declarations (binding-time, e.g. .def()), but it now means there are now distinct entry points to this type-checking that users can trigger :(

Possible resolutions:

• if it's worth the risk, then just mention it in the holder / smart pointer docs (as a diff in this PR)
• If it's not worth the risk, somehow enable raw py::cast<>() to do a runtime check. This may make for some crazy dumb plumbing, though :(
• Alternatively, eat the cost of runtime casts and funnel it through type_caster<>. That's what we do for our fork (RobotLocomotion/pybind11), and we haven't yet had to point fingers at performance there (though our use case may be simpler).

@YannickJadoul or @rwgk Any chance y'all have a good (and mebbe easy?) timing performance benchmark for this, to see what the risk is in these terms?

EDIT: Hm... for docs/benchmark.py, it's only for compilation time and size, and doesn't really dip into the more nuanced things (e.g. inheritance, custom type casters).

[rwgk]

@EricCousineau-TRI wrote:

Any chance y'all have a good (and mebbe easy?) timing performance benchmark for this, to see what the risk is in these terms?

The TensorFlow core team has very sophisticated pybind11 benchmarks, but it's currently Google-internal only. The author already gave me permission to extract most of it for external view, including sources, but it may take me a few days (I want to show what I extract to the author for approval).

[EricCousineau-TRI]

Sweet!!! That'd be awesome! I'll see if I can gather some common "complex" patterns in Drake to see if I can get some patterns we have (but maintain feature-parity with upstream).

Have you thought any about where your benchmarks might live? I think the ones I'd generate would be simple enough (i.e. just pybind11 bits) to be in a benchmarking subdirectory.

[rhaschke]

I explicitly converted my previous code doing compatibility checks at cast time to this code, doing these tests at definition time only, because of efficiency concerns raised elsewhere.

[YannickJadoul]

I'm OK with the principle of trading 1 bit for 32 bits and to be able to make these checks.

The main thing bothering me is this string comparison. I feel a bit bad for saying this, because I don't have a great alternative, but it feels very brittle. For example, what about non-templated holder types for a certain type, which doesn't have this nice Holder<Type> name? (Maybe this is not a case that should be considered valid, though, but still).

One alternative would be to require a constexpr auto generic_holder_typeid; field (or similar) in the holder_helper struct, which would give a single typeid for all holders? E.g. for unique_ptr<T> and shared_ptr<T> it could just be unique_ptr<void> and shared_ptr<void>? (unique_ptr<void> can't be instantiated but still has a typeid: https://godbolt.org/z/1179xG) Alternatively, we use a dummy type instead of void, but the main point is that all holder_helper<Holder<T>>::generic_holder_typeid could map to the same unique std::type_info.

I'm not sure how easy this would be, but I think we could find sensible defaults. Happy to do some work and try this out myself, if you think this might work?

[YannickJadoul]

This breaks ABI, so should ideally be part of a batch of ABI-breaking PRs.

[YannickJadoul]

This already exists in cast.h, it seems?

// PYBIND11_DECLARE_HOLDER_TYPE holder types:
template <typename base, typename holder> struct is_holder_type :
    std::is_base_of<detail::type_caster_holder<base, holder>, detail::type_caster<holder>> {};
// Specialization for always-supported unique_ptr holders:
template <typename base, typename deleter> struct is_holder_type<base, std::unique_ptr<base, deleter>> :
    std::true_type {};

[rhaschke]

Yeah, I missed that. The existing definition should serve my purpose.

[YannickJadoul]

I'm OK with the principle of trading 1 bit for 32 bits and to be able to make these checks.

For the record: this is something that does need to pass @wjakob, once we have something we're proud of presenting. But @jagerman's old PR would probably be a good argument on this being a longstanding issue :-)

[rhaschke]

I'm OK with the principle of trading 1 bit for 32 bits and to be able to make these checks.

It's 8bits vs 32bits actually 😉

The main thing bothering me is this string comparison. I feel a bit bad for saying this, because I don't have a great alternative, but it feels very brittle. For example, what about non-templated holder types for a certain type, which doesn't have this nice Holder<Type> name? (Maybe this is not a case that should be considered valid, though, but still).

Aren't holder types always templated? But, I understand your hesitation.
Note that the string-based comparison is only required for the declaration of derived classes. Compatibility check for (the much more frequent) function signatures is based on pointers:
https://github.com/pybind/pybind11/pull/2644/files#diff-b8e97eea5f84a84248db1bc72afaccfd8ba28cad023917d52bdc6bbc3669cd74R1627

One alternative would be to require a constexpr auto generic_holder_typeid; field (or similar) in the holder_helper struct, which would give a single typeid for all holders? E.g. for unique_ptr<T> and shared_ptr<T> it could just be unique_ptr<void> and shared_ptr<void>? (unique_ptr<void> can't be instantiated but still has a typeid: https://godbolt.org/z/1179xG) Alternatively, we use a dummy type instead of void, but the main point is that all holder_helper<Holder<T>>::generic_holder_typeid could map to the same unique std::type_info.

Good idea! A remaining open issue (with both approaches) might be extra optional template arguments, e.g. the deleter of unique_ptr. Usually, we don't want to allow changing the deleter from a base to an derived type. But there is one counterexample in the unit tests:

pybind11/tests/test_smart_ptr.cpp

Lines 207 to 208 in b491b46

	// Object derived but with public destructor and no Deleter in default holder
	class MyObject4b : public MyObject4a {

[YannickJadoul]

It's 8bits vs 32bits actually wink

A bool, yes, but this is a bitfield : 1. And one that still fits in the alignment, so in practice probably 0 instead of 32, to be pedantic ;-)

Aren't holder types always templated? But, I understand your hesitation.

Yes, probably, but if you insist, I can make sure I set up some kind of holder where that doesn't work. But you're right it wouldn't be a frequent scenario, and maybe not even officially supported (the docs don't really mention it as possible or not-possible) ;-

Note that the string-based comparison is only required for the declaration of derived classes. Compatibility check for (the much more frequent) function signatures is based on pointers:
https://github.com/pybind/pybind11/pull/2644/files#diff-b8e97eea5f84a84248db1bc72afaccfd8ba28cad023917d52bdc6bbc3669cd74R1627)

Hmmm, does that mean I can't return/pass a std::unique_ptr<Base> instead of Derived? Oh, but that's already not possible anyway, if Base is not registered, I believe.

So yes (and actually, same_type can also do a string comparison if it needs to, but it's not trying to match substrings).
So agreeing that performance is not a huge issue, here. I'm just (maybe unnecessarily) scared of string manipulation when it comes to comparing types :-)

Good idea! A remaining open issue (with both approaches) might be extra optional template arguments, e.g. the deleter of unique_ptr. Usually, we don't want to allow changing the deleter from a base to an derived type. But there is one counterexample in the unit tests:

pybind11/tests/test_smart_ptr.cpp

Lines 207 to 208 in b491b46

	// Object derived but with public destructor and no Deleter in default holder
	class MyObject4b : public MyObject4a {

Given your previous rebuttal on the string comparison, we'd need to test if this is worth the extra interface, though?

And yes, the deleter is hairy. I'm not sure what to think of that. Would this be something to explicitly require users to opt-in to (saying, "yes, it's fine that these holder types are different, I take responsibility") ?
It's also another instance where substringing unique_ptr<A, FooDeleter>/unique_ptr<A, BarDeleter> to unique_ptr (cfr. the previous point) is tricky, though?

[rwgk]

I converted this to Draft and removed the needs review label.
This PR is high up on my mind in connection with the smart_holder work.
I'm thinking the best way to integrate the smart_holder functionality is to cover the goal of this PR as well. I.e. I think it's a pretty big project.
When @rhaschke has a block of time again (?) a meeting to discuss may be very useful.