[BUG]: differing typeids for pybind11::memory::guarded_delete

[petersteneteg]

Required prerequisites

• Make sure you've read the documentation. Your issue may be addressed there.
• Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
• Consider asking first in the Gitter chat room or in a Discussion.

What version (or hash if on master) of pybind11 are you using?

98bd78f

Problem description

When casting python objects from python to c++, that have "python_instance_is_alias == true"
one can end up on this line:

pybind11/include/pybind11/detail/type_caster_base.h

Line 772 in 03b4a9e

auto *vptr_gd_ptr = std::get_deleter<memory::guarded_delete>(hld.vptr);

Where we try to extract a "pybind11::memory::guarded_delete" deleter from a std::shared_ptr.
Since the deleter is type erased the implementation needs to check that the typeids of the requested deleter and the currently set deleter are the same.

But when we compile with visibility hidden and on at least libc++ all non exported symbols will have a "unique" id to that module.
But if we create the object in one module as cast it in a different module the ids will not match! And the cast will fail.

I think the solution should be to export pybind11::memory::guarded_delete here:

pybind11/include/pybind11/detail/struct_smart_holder.h

Line 81 in 03b4a9e

struct guarded_delete {

doing the struct PYBIND11_EXPORT guarded_delete makes the example code below work as expected.

Reproducible example code

https://github.com/petersteneteg/pybind11-visibility

CMakeLists.txt:
cmake_minimum_required(VERSION 3.30...4.0)
project(pybind11-find-package LANGUAGES CXX C)
include(GenerateExportHeader)

set(CMAKE_CXX_STANDARD 23)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
find_package(Python3 COMPONENTS Interpreter Development)
find_package(pybind11 CONFIG REQUIRED)

add_library(lib1 lib1.hpp lib1.cpp)
add_library(lib1::lib1 ALIAS lib1)
target_include_directories(lib1 PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>)
target_include_directories(lib1 PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>)

generate_export_header(lib1)

pybind11_add_module(bindings bindings.cpp)
target_link_libraries(bindings PUBLIC lib1)

add_executable(main main.cpp)
target_link_libraries(main PUBLIC lib1::lib1 Python3::Python pybind11::embed)

# Ensure that we have built the python bindings since we load them in main
add_dependencies(main bindings)

lib1.h:
#pragma once

#include <lib1_export.h>
#include <memory>

namespace lib1 {

class LIB1_EXPORT Base : public std::enable_shared_from_this<Base> {
public:
    Base(int a, int b);
    virtual ~Base() = default;

    virtual int get() const { return a + b; }

    int a;
    int b;
};

class LIB1_EXPORT Foo : public Base {
public:
    Foo(int a, int b);

    int get() const override { return 2 * a + b; }
};

}  // namespace lib1

lib1.cpp:
#include <lib1.hpp>

namespace lib1 {

Base::Base(int a, int b) : a(a), b(b) {}

Foo::Foo(int a, int b) : Base{a, b} {}

}  // namespace lib1

bindings.cpp:
#include <pybind11/pybind11.h>

#include <lib1.hpp>

class BaseTrampoline : public lib1::Base, public pybind11::trampoline_self_life_support {
public:
    using lib1::Base::Base;
    int get() const override { PYBIND11_OVERLOAD(int, lib1::Base, get); }
};

PYBIND11_MODULE(bindings, m) {
    pybind11::classh<lib1::Base, BaseTrampoline>(m, "Base")
        .def(pybind11::init<int, int>())
        .def_readwrite("a", &lib1::Base::a)
        .def_readwrite("b", &lib1::Base::b);

    m.def("get_foo", [](int a, int b) -> std::shared_ptr<lib1::Base> {
        return std::make_shared<lib1::Foo>(a, b);
    });
}


main.cpp:
#include <lib1.hpp>
#include <pybind11/embed.h>

#include <print>

static constexpr std::string_view script = R"(
import bindings

class Bar(bindings.Base):
    def __init__(self, a, b):
        bindings.Base.__init__(self, a, b)
    
    def get(self):
        return 4 * self.a + self.b


def get_bar(a, b):
    return Bar(a, b)

)";



int main() {
    pybind11::scoped_interpreter guard{};

    // "Simple" case this will not have "python_instance_is_alias" set in type_cast_base.h:771
    auto bindings = pybind11::module_::import("bindings");
    auto holder = bindings.attr("get_foo")(1,2);
    auto foo = holder.cast<std::shared_ptr<lib1::Base>>();
    auto res = foo->get();
    std::print("Result from Foo: {}\n", res);

    // "Complex" case this will have "python_instance_is_alias" set in type_cast_base.h:771
    pybind11::exec(script);
    auto main = pybind11::module::import("__main__");
    auto holder2 = main.attr("get_bar")(1,2);

    // this will trigger "std::get_deleter<memory::guarded_delete>" in type_cast_base.h:772
    // This will fail since the program will see two different typeids for "memory::guarded_delete"
    // on from the bindings module and one from "main", which will both have "__is_type_name_unique"
    // as true and but still have different values. Hence we will not find the deleter and the cast
    // fill fail.
    // See "__eq(__type_name_t __lhs, __type_name_t __rhs)" in typeinfo in libc++
    auto bar = holder2.cast<std::shared_ptr<lib1::Base>>();
    auto res2 = bar->get();
    std::print("Result from Bar: {}\n", res2);

    // To solve this we must ensure that the typeinfo for `memory::guarded_delete` is the same
    // everywhere and to easiest way is to export using `PYBIND11_EXPORT"
    // in `struct_smart_holder.h`:81
    // like: `struct PYBIND11_EXPORT guarded_delete {...`

    return 0;
}

Is this a regression? Put the last known working version here if it is.

Not a regression

[rwgk]

@petersteneteg Could you please help by starting a PR with a unit test for this situation?

I think we need to factor out PYBIND11_EXPORT into include/pybind11/detail/pybind11_export_macro.h (similar to pybind11_namespace_macros.h).

But I'm actually a bit worried, about ABI compatibility:

But if we create the object in one module as cast it in a different module the ids will not match!

What if the two modules are not ABI compatible?

[petersteneteg]

@rwgk I mean if they are not ABI compatible it that should be detected, (and I have not looked at those parts.) But If they are ABI compatible, (I build everything from source...) I should work!.

Basically the all modules has to see the same "guarded_delete" otherwise that code path will not work.

I think that any non-template class that is not exported, should never be used across module boundaries. Which is clearly the case here. I have only run into this class as a breaking error. But I think one might review all non-template classes in the code base.
For example "trampoline_self_life_support" is also a non exported non-template class. And in some cases I have fond that I need to export the derived Trampoline class to get the correct runtime behavior. That seems to work, but will usually warn that the base class is not exported...

Anyway I will try and convert my example code above into a test.