Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use std::hash<std::type_index>, std::equal_to<std::type_index> everywhere **except when libc++ is in use** #4319

Merged
merged 29 commits into from
Apr 25, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a06949a
Try using `std::hash<std::type_index>`, `std::equal_to<std::type_inde…
Nov 6, 2022
9d70eba
Revert "Try using `std::hash<std::type_index>`, `std::equal_to<std::t…
Nov 7, 2022
9141519
Use "our own name-based hash and equality functions" for `std::type_i…
Nov 7, 2022
ba747dc
Patch in PR #4313: Minimal reproducer for clash when binding types de…
Nov 7, 2022
1912670
test_unnamed_namespace_b xfail for clang
Nov 7, 2022
d8b7118
`PYBIND11_INTERNALS_VERSION 5`
Nov 7, 2022
780d3b2
Add a note to docs/classes.rst
Nov 7, 2022
3373b47
For compatibility with Google-internal testing, test_unnamed_namespac…
Nov 7, 2022
8f0c28e
Trying "__GLIBCXX__ or Windows", based on observations from Google-in…
Nov 7, 2022
883cadf
Try _LIBCPP_VERSION
Nov 7, 2022
72a8895
Account for libc++ behavior in tests and documentation.
Nov 7, 2022
6f1ddb2
Adjust expectations for Windows Clang (and make code less redundant).
Nov 10, 2022
a1175ed
Add WindowsClang to ci.yml
Nov 10, 2022
5631d94
Add clang-latest to name that appears in the GitHub Actions web view.
Nov 10, 2022
81a4e7b
Tweak the note in classes.rst again.
Nov 10, 2022
7075e00
Add `pip install --upgrade pip`, Show env, cosmetic changes
Nov 10, 2022
b84a163
Add macos_brew_install_llvm to ci.yml
Nov 10, 2022
4f06c06
`test_cross_module_exception_translator` xfail 'Homebrew Clang'
Nov 10, 2022
2180d04
Revert back to base version of .github/workflows/ci.yml (the ci.yml c…
Mar 20, 2023
d2d2c0a
Merge branch 'master' into internals_std_type_index_modernization
Mar 20, 2023
b882060
Fixes for ruff
Mar 20, 2023
66e0fac
Merge branch 'master' into internals_std_type_index_modernization
Mar 28, 2023
f1c3055
Make updated condition in internals.h dependent on ABI version.
Mar 28, 2023
4842b9f
Merge branch 'master' into internals_std_type_index_modernization
Mar 30, 2023
0ff73a9
Remove PYBIND11_TEST_OVERRIDE when testing with PYBIND11_INTERNALS_VE…
Mar 30, 2023
9f60e9a
Merge branch 'master' into internals_std_type_index_modernization
Apr 21, 2023
d0276c0
Selectively exercise cmake `-DPYBIND11_TEST_OVERRIDE`: ubuntu, macos,…
Apr 21, 2023
4f4fd04
Merge branch 'master' into internals_std_type_index_modernization
Apr 25, 2023
1b4a508
Update skipif for Python 3.12a7 (the WIP needs to be handled in a sep…
Apr 25, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions docs/classes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,15 @@ interactive Python session demonstrating this example is shown below:
Static member functions can be bound in the same way using
:func:`class_::def_static`.

.. note::

Binding C++ types in unnamed namespaces (also known as anonymous namespaces)
works reliably only with GCC and MSVC, but not with CLANG.
See `#4316 <https://github.com/pybind/pybind11/pull/4316>`_ for background.
If portability is a concern, it is therefore not recommended to bind C++
types in unnamed namespaces. It will be safest to manually pick unique
namespace names.

Keyword and default arguments
=============================
It is possible to specify keyword and default arguments using the syntax
Expand Down
4 changes: 2 additions & 2 deletions include/pybind11/detail/internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
/// further ABI-incompatible changes may be made before the ABI is officially
/// changed to the new version.
#ifndef PYBIND11_INTERNALS_VERSION
# define PYBIND11_INTERNALS_VERSION 4
# define PYBIND11_INTERNALS_VERSION 5
#endif

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
Expand Down Expand Up @@ -114,7 +114,7 @@ inline void tls_replace_value(PYBIND11_TLS_KEY_REF key, void *value) {
// libstdc++, this doesn't happen: equality and the type_index hash are based on the type name,
// which works. If not under a known-good stl, provide our own name-based hash and equality
// functions that use the type name.
#if defined(__GLIBCXX__)
#if !defined(__apple_build_version__)
Copy link
Collaborator

@Skylion007 Skylion007 Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about non_apple_clang Clang15, Clang16? I am actually not sure we test that in our CI surprisingly, but llvm supports Linux, Windows, and MacOS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question.

  • Windows clang: I'm trying internal testing, which in the past at least triggered some Windows clang testing, although I never fully understood what exactly.
  • Non-Apple macOS clang: Could someone please help with testing?

But:

  • I'm not in a rush to merge this PR.
  • I just want this to be in the pipeline if and when we decide to increment PYBIND11_INTERNALS_VERSION.

FWIW, initially I thought the problem is elsewhere and it may be an easy fix. I didn't anticipate that it would go in the direction you see now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwgk Is there an issue opened up on the LLVM repo for this bug btw?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwgk Is there an issue opened up on the LLVM repo for this bug btw?

Not to my knowledge.

I think it'll be a couple hours work (at least) to create a minimal reproducer for them, assuming that they will not be able to work with a pybind11-based reproducer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about non_apple_clang Clang15, Clang16? I am actually not sure we test that in our CI surprisingly, but llvm supports Linux, Windows, and MacOS.

#4316 (comment)

That platform is actually the worst: std::type_index does not work as desired, and cross-module exception translation is broken, even if the exceptions have default visibility (IOW "are exported").

Copy link
Collaborator

@ax3l ax3l Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-Apple macOS clang: Could someone please help with testing?

We could test this using the conda-forge compilers (they are vanilla clang on macOS). This also work in CI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-Apple macOS clang: Could someone please help with testing?

We could test this using the conda-forge compilers (they are vanilla clang on macOS). This also work in CI.

In the meantime I solved this (but forgot to note that here):

#4326

inline bool same_type(const std::type_info &lhs, const std::type_info &rhs) { return lhs == rhs; }
using type_hash = std::hash<std::type_index>;
using type_equal_to = std::equal_to<std::type_index>;
Expand Down
2 changes: 2 additions & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ set(PYBIND11_TEST_FILES
test_tagbased_polymorphic
test_thread
test_union
test_unnamed_namespace_a
test_unnamed_namespace_b
test_virtual_functions)

# Invoking cmake with something like:
Expand Down
9 changes: 9 additions & 0 deletions tests/test_unnamed_namespace_a.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#include "pybind11_tests.h"

namespace {
struct any_struct {};
} // namespace

TEST_SUBMODULE(unnamed_namespace_a, m) {
py::class_<any_struct>(m, "unnamed_namespace_a_any_struct");
}
8 changes: 8 additions & 0 deletions tests/test_unnamed_namespace_a.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# NOTE: This test relies on pytest SORT ORDER:
# test_unnamed_namespace_a.py imported before test_unnamed_namespace_b.py

from pybind11_tests import unnamed_namespace_a as m


def test_have_class_any_struct():
assert m.unnamed_namespace_a_any_struct is not None
19 changes: 19 additions & 0 deletions tests/test_unnamed_namespace_b.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#include "pybind11_tests.h"

namespace {
struct any_struct {};
} // namespace

TEST_SUBMODULE(unnamed_namespace_b, m) {
if (py::detail::get_type_info(typeid(any_struct)) == nullptr) {
py::class_<any_struct>(m, "unnamed_namespace_b_any_struct");
} else {
m.attr("unnamed_namespace_b_any_struct") = py::none();
}
m.attr("defined___clang__") =
#if defined(__clang__)
true;
#else
false;
#endif
}
15 changes: 15 additions & 0 deletions tests/test_unnamed_namespace_b.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# NOTE: This test relies on pytest SORT ORDER:
# test_unnamed_namespace_a.py imported before test_unnamed_namespace_b.py

import pytest

from pybind11_tests import unnamed_namespace_b as m


@pytest.mark.xfail(
"m.defined___clang__",
reason="Known issue with all clang versions: https://github.com/pybind/pybind11/pull/4316",
strict=True,
)
def test_have_class_any_struct():
assert m.unnamed_namespace_b_any_struct is not None