[smart_holder]: trampoline_self_life_support -fvisibility question

danielcjacobs commented 2 years ago

Required prerequisites

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

Problem description

I'm attempting to use the Progressive mode to solve a lifetime issue with a trampoline class via the following steps:

Added -DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT to compilation commands
Replaced std::shared_ptr<...> holder with PYBIND11_SH_DEF(Foo)
Inherit trampoline class from public py::trampoline_self_life_support

When I compile, I get this warning:

warning: ‘PyFoo’ declared with greater visibility than its base ‘pybind11::trampoline_self_life_support’ [-Wattributes]
  134 | class PyFoo : Foo, py::trampoline_self_life_support {

Reproducible example code

// C++
#include <pybind11/pybind11.h>

class Foo {
public:
    virtual ~Foo()                   = default;
};

// Trampoline class
class PyFoo : Foo, public py::trampoline_self_life_support {
public:
    using Foo::Foo;
};

// Bindings
PYBIND11_MODULE("smart_holder", m) {
    py::class_<Foo, PyFoo, PYBIND11_SH_DEF(Foo)>(m, "Foo")
        .def(py::init<>())
}

danielcjacobs commented 2 years ago

Note, if I don't derive from py::trampoline_self_life_support, the code still compiles and runs without any lifetime issues (and the warning goes away of course).

Skylion007 commented 2 years ago

Ping @rwgk

rwgk commented 2 years ago

tried to call pure virtual function The smart holder branch seems intended to solve this very issue.

Only if there is actually a lifetime issue (with pybind11 master), but not in general. It looks like your case is in the "general" domain.

warning: ‘PyFoo’ declared with greater visibility than its base ‘pybind11::trampoline_self_life_support’ [-Wattributes] 134 | class PyFoo : Foo, py::trampoline_self_life_support {

This seems to be related:

https://stackoverflow.com/questions/2828738/c-warning-declared-with-greater-visibility-than-the-type-of-its-field

With a lot of guessing, PyFoo seems to have default visibility, while trampoline_self_life_support has hidden visibility.

Spontaneous best guesses:

Build your extension with -fvisibility=hidden (I think that's what the documentation recommends).
Silence the warning if you cannot use -fvisibility=hidden.
Or build everything with -fvisibility=default (that's what we do internally at Google).

Your milage may vary.

Or do you think we need to PYBIND11_EXPORT ‘trampoline_self_life_support’? (I hope not, but it totally depends, I guess.)

danielcjacobs commented 2 years ago

Seeing that it works without deriving from py::trampoline_self_life_support, I'm curious as to what the purpose of inheriting from this class is?

danielcjacobs commented 2 years ago

Or do you think we need to PYBIND11_EXPORT ‘trampoline_self_life_support’? (I hope not, but it totally depends, I guess.)

I'm curious, why would we not export this symbol? Seems like it should be exported since it's intended to be used by other libraries.

rwgk commented 2 years ago

Seeing that it works without deriving from py::trampoline_self_life_support, I'm curious as to what the purpose of inheriting from this class is?

Did you see this already?

https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst#trampolines-and-stdunique_ptr

Takeaway:

You only need it if you want to pass unique_ptrs between Python and C++.
Best practice: Always use it (maybe with #ifdef if you want to stay compatible with master).

rwgk commented 2 years ago

Or do you think we need to PYBIND11_EXPORT ‘trampoline_self_life_support’? (I hope not, but it totally depends, I guess.)

I'm curious, why would we not export this symbol? Seems like it should be exported since it's intended to be used by other libraries.

I'm not sure to be honest, especially because we (Google) internally use "default" visibility, i.e. everything is exported.

I am thinking/guessing for the "external" world that does many things differently, keeping the symbol hidden is better, for situations in which extensions are built with different pybind11 versions, compilers, or options, or all of it.

rwgk commented 2 years ago

Unrelated to this issue, I recently got to understand more about -fvisibility=hidden and namespace pybind11 __attribute__((visibility("hidden"))) (pybind11/detail/common.h).

My thinking now:

Exporting py::trampoline_self_life_support is definitely not the right approach, because it could lead to issues if extensions built with different versions of pybind11 are used in the same process.
With respect to the "declared with greater visibility" warning shown in the original posting, that can be resolved by marking the enclosing namespace with __attribute__((visibility("hidden"))), similar to (currently draft) PR #4072.
An alternative is to use a #pragma GCC diagnostic push, ignored, pop around the trampoline class, to suppress the warning.
Another more radical alternative is to -DPYBIND11_NAMESPACE=pybind11, which means all module-local features will be unavailable. That is probably only an option in tightly controlled environments, when it is certain that RTLD_LOCAL or equivalent is in effect, or that all extensions are built with the same pybind11 version.

pybind / pybind11