fix: expose required symbol using clang (#5700)

* test: Added test case for visibility of common symbols across shared libraries

* style: pre-commit fixes

* tests: cmake target name fix

* tests: Added visibility test to ci

* tests: set the default visibility to hidden

* prototype/proof-of-concept fix: PYBIND11_EXPORT_GUARDED_DELETE

* Fix silly oversight: actually use PYBIND11_EXPORT_GUARDED_DELETE

* Update struct_smart_holder.h

* style: pre-commit fixes

* Update include/pybind11/detail/struct_smart_holder.h

* Update struct_smart_holder.h

* ci: fix addition to reusable-standard.yml

* Update CMakeLists.txt

* refactor: rename tests to test_cross_module_rtti

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

---------

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
Co-authored-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>

.github/workflows/ci.yml

@@ -230,6 +230,9 @@ jobs:
     - name: Interface test
       run: cmake --build . --target test_cmake_build
 
+    - name: Visibility test
+      run: cmake --build . --target test_cross_module_rtti
+
 
   manylinux:
     name: Manylinux on 🐍 3.13t • GIL
@@ -328,6 +331,9 @@ jobs:
     - name: C++ tests
       run: cmake --build --preset default --target cpptest
 
+    - name: Visibility test
+      run: cmake --build --preset default --target test_cross_module_rtti
+
     - name: Run Valgrind on Python tests
       if: matrix.valgrind
       run: cmake --build --preset default --target memcheck
@@ -386,6 +392,8 @@ jobs:
     - name: Interface test
       run: cmake --build build --target test_cmake_build
 
+    - name: Visibility test
+      run: cmake --build build --target test_cross_module_rtti
 
   # Testing NVCC; forces sources to behave like .cu files
   cuda:
@@ -505,6 +513,8 @@ jobs:
     - name: Interface test
       run: cmake --build build --target test_cmake_build
 
+    - name: Visibility test
+      run: cmake --build build --target test_cross_module_rtti
 
   # Testing on GCC using the GCC docker images (only recent images supported)
   gcc:
@@ -556,6 +566,9 @@ jobs:
     - name: Interface test
       run: cmake --build build --target test_cmake_build
 
+    - name: Visibility test
+      run: cmake --build build --target test_cross_module_rtti
+
     - name: Configure - Exercise cmake -DPYBIND11_TEST_OVERRIDE
       if: matrix.gcc == '12'
       shell: bash
@@ -638,6 +651,11 @@ jobs:
         set +e; source /opt/intel/oneapi/setvars.sh; set -e
         cmake --build build-11 --target test_cmake_build
 
+    - name: Visibility test
+      run: |
+        set +e; source /opt/intel/oneapi/setvars.sh; set -e
+        cmake --build build-11 --target test_cross_module_rtti
+
     - name: Configure C++17
       run: |
         set +e; source /opt/intel/oneapi/setvars.sh; set -e
@@ -670,6 +688,10 @@ jobs:
         set +e; source /opt/intel/oneapi/setvars.sh; set -e
         cmake --build build-17 --target test_cmake_build
 
+    - name: Visibility test
+      run: |
+        set +e; source /opt/intel/oneapi/setvars.sh; set -e
+        cmake --build build-17 --target test_cross_module_rtti
 
   # Testing on CentOS (manylinux uses a centos base).
   centos:
@@ -732,6 +754,9 @@ jobs:
     - name: Interface test
       run: cmake --build build --target test_cmake_build
 
+    - name: Visibility test
+      run: cmake --build build --target test_cross_module_rtti
+
 
   # This tests an "install" with the CMake tools
   install-classic:
@@ -961,6 +986,9 @@ jobs:
     - name: Interface test C++20
       run: cmake --build build --target test_cmake_build
 
+    - name: Visibility test
+      run: cmake --build build --target test_cross_module_rtti
+
     - name: Configure C++20 - Exercise cmake -DPYBIND11_TEST_OVERRIDE
       run: >
         cmake -S . -B build_partial
@@ -1034,6 +1062,9 @@ jobs:
     - name: Interface test C++11
       run: PYTHONHOME=/${{matrix.sys}} PYTHONPATH=/${{matrix.sys}} cmake --build build --target test_cmake_build
 
+    - name: Visibility test
+      run: PYTHONHOME=/${{matrix.sys}} PYTHONPATH=/${{matrix.sys}} cmake --build build --target test_cross_module_rtti
+
     - name: Clean directory
       run: git clean -fdx
 
@@ -1055,6 +1086,9 @@ jobs:
     - name: Interface test C++14
       run: PYTHONHOME=/${{matrix.sys}} PYTHONPATH=/${{matrix.sys}} cmake --build build2 --target test_cmake_build
 
+    - name: Visibility test
+      run: PYTHONHOME=/${{matrix.sys}} PYTHONPATH=/${{matrix.sys}} cmake --build build2 --target test_cross_module_rtti
+
     - name: Clean directory
       run: git clean -fdx
 
@@ -1076,6 +1110,9 @@ jobs:
     - name: Interface test C++17
       run: PYTHONHOME=/${{matrix.sys}} PYTHONPATH=/${{matrix.sys}} cmake --build build3 --target test_cmake_build
 
+    - name: Visibility test
+      run: PYTHONHOME=/${{matrix.sys}} PYTHONPATH=/${{matrix.sys}} cmake --build build3 --target test_cross_module_rtti
+
   windows_clang:
     if: github.event.pull_request.draft == false
 
@@ -1143,6 +1180,9 @@ jobs:
       - name: Interface test
         run: cmake --build . --target test_cmake_build -j 2
 
+      - name: Visibility test
+        run: cmake --build . --target test_cross_module_rtti -j 2
+
       - name: Clean directory
         run: git clean -fdx
 
@@ -1210,6 +1250,9 @@ jobs:
       - name: Interface test
         run: cmake --build . --target test_cmake_build -j 2
 
+      - name: Visibility test
+        run: cmake --build . --target test_cross_module_rtti -j 2
+
       - name: CMake Configure - Exercise cmake -DPYBIND11_TEST_OVERRIDE
         run: >
           cmake -S . -B build_partial

.github/workflows/reusable-standard.yml

@@ -74,6 +74,9 @@ jobs:
       - name: Interface test
         run: cmake --build build --target test_cmake_build
 
+      - name: Visibility test
+        run: cmake --build build --target test_cross_module_rtti
+
       - name: Setuptools helpers test
         run: |
           uv pip install --python=python --system setuptools

CMakePresets.json

@@ -61,13 +61,13 @@
       "name": "tests",
       "displayName": "Tests (for workflow)",
       "configurePreset": "default",
-      "targets": ["pytest", "cpptest", "test_cmake_build"]
+      "targets": ["pytest", "cpptest", "test_cmake_build", "test_cross_module_rtti"]
     },
     {
       "name": "testsvenv",
       "displayName": "Tests Venv (for workflow)",
       "configurePreset": "venv",
-      "targets": ["pytest", "cpptest", "test_cmake_build"]
+      "targets": ["pytest", "cpptest", "test_cmake_build", "test_cross_module_rtti"]
     }
   ],
   "workflowPresets": [

include/pybind11/detail/struct_smart_holder.h

@@ -58,6 +58,19 @@ Details:
 #include <typeinfo>
 #include <utility>
 
+// IMPORTANT: This code block must stay BELOW the #include <stdexcept> above.
+// This is only required on some builds with libc++ (one of three implementations
+// in
+// https://github.com/llvm/llvm-project/blob/a9b64bb3180dab6d28bf800a641f9a9ad54d2c0c/libcxx/include/typeinfo#L271-L276
+// require it)
+#if !defined(PYBIND11_EXPORT_GUARDED_DELETE)
+#    if defined(_LIBCPP_VERSION) && !defined(WIN32) && !defined(_WIN32)
+#        define PYBIND11_EXPORT_GUARDED_DELETE __attribute__((visibility("default")))
+#    else
+#        define PYBIND11_EXPORT_GUARDED_DELETE
+#    endif
+#endif
+
 PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
 PYBIND11_NAMESPACE_BEGIN(memory)
 
@@ -78,7 +91,7 @@ static constexpr bool type_has_shared_from_this(const void *) {
     return false;
 }
 
-struct guarded_delete {
+struct PYBIND11_EXPORT_GUARDED_DELETE guarded_delete {
     std::weak_ptr<void> released_ptr;    // Trick to keep the smart_holder memory footprint small.
     std::function<void(void *)> del_fun; // Rare case.
     void (*del_ptr)(void *);             // Common case.

tests/CMakeLists.txt

@@ -649,4 +649,7 @@ if(NOT PYBIND11_CUDA_TESTS)
 
   # Test CMake build using functions and targets from subdirectory or installed location
   add_subdirectory(test_cmake_build)
+
+  # Test visibility of common symbols across shared libraries
+  add_subdirectory(test_cross_module_rtti)
 endif()

tests/test_cross_module_rtti/CMakeLists.txt

@@ -0,0 +1,68 @@
+possibly_uninitialized(PYTHON_MODULE_EXTENSION Python_INTERPRETER_ID)
+
+set(CMAKE_CXX_VISIBILITY_PRESET hidden)
+
+if("${PYTHON_MODULE_EXTENSION}" MATCHES "pypy"
+   OR "${Python_INTERPRETER_ID}" STREQUAL "PyPy"
+   OR "${PYTHON_MODULE_EXTENSION}" MATCHES "graalpy")
+  message(STATUS "Skipping visibility test on PyPy or GraalPy")
+  add_custom_target(test_cross_module_rtti
+  )# Dummy target on PyPy or GraalPy. Embedding is not supported.
+  set(_suppress_unused_variable_warning "${DOWNLOAD_CATCH}")
+  return()
+endif()
+
+if(TARGET Python::Module AND NOT TARGET Python::Python)
+  message(STATUS "Skipping visibility test since no embed libs found")
+  add_custom_target(test_cross_module_rtti) # Dummy target since embedding is not supported.
+  set(_suppress_unused_variable_warning "${DOWNLOAD_CATCH}")
+  return()
+endif()
+
+find_package(Catch 2.13.10)
+
+if(CATCH_FOUND)
+  message(STATUS "Building interpreter tests using Catch v${CATCH_VERSION}")
+else()
+  message(STATUS "Catch not detected. Interpreter tests will be skipped. Install Catch headers"
+                 " manually or use `cmake -DDOWNLOAD_CATCH=ON` to fetch them automatically.")
+  return()
+endif()
+
+include(GenerateExportHeader)
+
+add_library(test_cross_module_rtti_lib SHARED lib.h lib.cpp)
+add_library(test_cross_module_rtti_lib::test_cross_module_rtti_lib ALIAS
+            test_cross_module_rtti_lib)
+target_include_directories(test_cross_module_rtti_lib PUBLIC ${CMAKE_CURRENT_BINARY_DIR})
+target_include_directories(test_cross_module_rtti_lib PUBLIC ${CMAKE_CURRENT_SOURCE_DIR})
+target_compile_features(test_cross_module_rtti_lib PUBLIC cxx_std_11)
+
+generate_export_header(test_cross_module_rtti_lib)
+
+pybind11_add_module(test_cross_module_rtti_bindings SHARED bindings.cpp)
+target_link_libraries(test_cross_module_rtti_bindings
+                      PUBLIC test_cross_module_rtti_lib::test_cross_module_rtti_lib)
+
+add_executable(test_cross_module_rtti_main catch.cpp test_cross_module_rtti.cpp)
+target_link_libraries(
+  test_cross_module_rtti_main PUBLIC test_cross_module_rtti_lib::test_cross_module_rtti_lib
+                                     pybind11::embed Catch2::Catch2)
+
+# Ensure that we have built the python bindings since we load them in main
+add_dependencies(test_cross_module_rtti_main test_cross_module_rtti_bindings)
+
+pybind11_enable_warnings(test_cross_module_rtti_main)
+pybind11_enable_warnings(test_cross_module_rtti_bindings)
+pybind11_enable_warnings(test_cross_module_rtti_lib)
+
+add_custom_target(
+  test_cross_module_rtti
+  COMMAND "$<TARGET_FILE:test_cross_module_rtti_main>"
+  DEPENDS test_cross_module_rtti_main
+  WORKING_DIRECTORY "$<TARGET_FILE_DIR:test_cross_module_rtti_main>")
+
+set_target_properties(test_cross_module_rtti_bindings PROPERTIES LIBRARY_OUTPUT_DIRECTORY
+                                                                 "${CMAKE_CURRENT_BINARY_DIR}")
+
+add_dependencies(check test_cross_module_rtti)

tests/test_cross_module_rtti/bindings.cpp

@@ -0,0 +1,20 @@
+#include <pybind11/pybind11.h>
+
+#include <lib.h>
+
+class BaseTrampoline : public lib::Base, public pybind11::trampoline_self_life_support {
+public:
+    using lib::Base::Base;
+    int get() const override { PYBIND11_OVERLOAD(int, lib::Base, get); }
+};
+
+PYBIND11_MODULE(test_cross_module_rtti_bindings, m) {
+    pybind11::classh<lib::Base, BaseTrampoline>(m, "Base")
+        .def(pybind11::init<int, int>())
+        .def_readwrite("a", &lib::Base::a)
+        .def_readwrite("b", &lib::Base::b);
+
+    m.def("get_foo", [](int a, int b) -> std::shared_ptr<lib::Base> {
+        return std::make_shared<lib::Foo>(a, b);
+    });
+}

tests/test_cross_module_rtti/catch.cpp

@@ -0,0 +1,22 @@
+// The Catch implementation is compiled here. This is a standalone
+// translation unit to avoid recompiling it for every test change.
+
+#include <pybind11/embed.h>
+
+// Silence MSVC C++17 deprecation warning from Catch regarding std::uncaught_exceptions (up to
+// catch 2.0.1; this should be fixed in the next catch release after 2.0.1).
+PYBIND11_WARNING_DISABLE_MSVC(4996)
+
+// Catch uses _ internally, which breaks gettext style defines
+#ifdef _
+#    undef _
+#endif
+
+#define CATCH_CONFIG_RUNNER
+#include <catch.hpp>
+
+int main(int argc, char *argv[]) {
+    pybind11::scoped_interpreter guard{};
+    auto result = Catch::Session().run(argc, argv);
+    return result < 0xff ? result : 0xff;
+}

tests/test_cross_module_rtti/lib.cpp

@@ -0,0 +1,13 @@
+#include <lib.h>
+
+namespace lib {
+
+Base::Base(int a, int b) : a(a), b(b) {}
+
+int Base::get() const { return a + b; }
+
+Foo::Foo(int a, int b) : Base{a, b} {}
+
+int Foo::get() const { return 2 * a + b; }
+
+} // namespace lib

tests/test_cross_module_rtti/lib.h

@@ -0,0 +1,30 @@
+#pragma once
+
+#include <memory>
+#include <test_cross_module_rtti_lib_export.h>
+
+#if defined(_MSC_VER)
+__pragma(warning(disable : 4251))
+#endif
+
+    namespace lib {
+
+    class TEST_CROSS_MODULE_RTTI_LIB_EXPORT Base : public std::enable_shared_from_this<Base> {
+    public:
+        Base(int a, int b);
+        virtual ~Base() = default;
+
+        virtual int get() const;
+
+        int a;
+        int b;
+    };
+
+    class TEST_CROSS_MODULE_RTTI_LIB_EXPORT Foo : public Base {
+    public:
+        Foo(int a, int b);
+
+        int get() const override;
+    };
+
+} // namespace lib

tests/test_cross_module_rtti/test_cross_module_rtti.cpp

@@ -0,0 +1,50 @@
+
+#include <pybind11/embed.h>
+#include <pybind11/pybind11.h>
+
+#include <catch.hpp>
+#include <lib.h>
+
+static constexpr auto script = R"(
+import test_cross_module_rtti_bindings
+
+class Bar(test_cross_module_rtti_bindings.Base):
+    def __init__(self, a, b):
+        test_cross_module_rtti_bindings.Base.__init__(self, a, b)
+
+    def get(self):
+        return 4 * self.a + self.b
+
+
+def get_bar(a, b):
+    return Bar(a, b)
+
+)";
+
+TEST_CASE("Simple case where without is_alias") {
+    // "Simple" case this will not have `python_instance_is_alias` set in type_cast_base.h:771
+    auto bindings = pybind11::module_::import("test_cross_module_rtti_bindings");
+    auto holder = bindings.attr("get_foo")(1, 2);
+    auto foo = holder.cast<std::shared_ptr<lib::Base>>();
+    REQUIRE(foo->get() == 4); // 2 * 1 + 2 = 4
+}
+
+TEST_CASE("Complex case where with it_alias") {
+    // "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__");
+
+    // The critical part of "Bar" is that it will have the `is_alias` `instance` flag set.
+    // I'm not quite sure what is required to get that flag, this code is derived from a
+    // larger code where this issue was observed.
+    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<lib::Base>>();
+    REQUIRE(bar->get() == 6); // 4 * 1 + 2 = 6
+}