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

rwgk · 2022-11-06T23:10:12Z

Description

This PR was informed by experiments under PR #4316. See there for background.

EDIT 2024-01-24: Related LLVM issue: llvm/llvm-project#79367

The only production code change is this:

--- a/include/pybind11/detail/internals.h
+++ b/include/pybind11/detail/internals.h
@@ -123,7 +123,8 @@ 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 (PYBIND11_INTERNALS_VERSION <= 4 && defined(__GLIBCXX__))                                     \
+    || (PYBIND11_INTERNALS_VERSION >= 5 && !defined(_LIBCPP_VERSION))
 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>;

Net change for PYBIND11_INTERNALS_VERSION 5 only (default only for Python 3.12+):

For MSVC this resolves clashes when binding types defined in the unnamed namespace (Minimal reproducer for clash when binding types defined in the unnamed namespace. #4313).

Note for completeness: This PR does not help with the template alias problem seen under #4581.

Suggested changelog entry:

With `PYBIND11_INTERNALS_VERSION 5` (default for Python 3.12+), MSVC builds use `std::hash<std::type_index>` and `std::equal_to<std::type_index>` instead of string-based type comparisons. This resolves issues when binding types defined in the unnamed namespace.

…x>` everywhere. From PR pybind#4316 we know that types in the unnamed namespace in different translation units do not compare equal, as desired. But do types in named namespaces compare equal, as desired?

…ype_index>` everywhere." This reverts commit a06949a.

…ndex` only under macOS, based on results shown under pybind#4316 (comment)

rwgk · 2022-11-07T06:23:46Z

The 2 CI failures are unrelated flakes:

CI / 🐍 3.6 • macos-latest • x64 — Fatal Python error: This thread state must be current when releasing test_embed/test_interpreter.cpp:285
CI / 🐍 3 • GCC 11 • C++20• x64 — Unable to connect to deb.debian.org:http

Not rerunning because I want to fold the tests under PR #4313 into this PR anyway.

…pes defined in the unnamed namespace.

rwgk · 2022-11-07T13:02:59Z

The 1 CI failure is a test_iostream flake (most common flake we have): CI / 🐍 pypy-3.7 • windows-2022 • x64

Skylion007 · 2022-11-07T14:16:50Z

include/pybind11/detail/internals.h

@@ -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__)


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.

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.

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

@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.

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").

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.

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

…e_a & test_unnamed_namespace_b need to work when imported in any order.

…ternal testing.

rwgk · 2022-11-07T21:33:54Z

To explain 8f0c28e:

Internally we have tests under Ubuntu and Debian that use -stdlib=libc++ -nostdlib++. Those broke.

The commit is backtracking to a more conservative approach: instead of "all but" the conditions for using std::hash<std::type_index> & std::equal_to<std::type_index> are now back to "only these".

The current version passes testing here and limited internal testing (global testing takes more time), but I don't think "only these" is ideal, as the situation for MSVC shows: it went unnoticed that newer MSVC versions have a working stl.

rwgk · 2022-11-07T22:43:20Z

Commit 883cadf restores the "all but" approach, and passes limited internal testing and the CI here.

The idea is: if we encounter other non-cooperating stl implementations, our unit tests will fail and we can then "opt out" those (from using std::hash<std::type_index> & std::equal_to<std::type_index>) as needed, by tweaking the #ifdef.

rwgk · 2022-11-08T16:49:49Z

This PR passed Google global testing (internal testing ID OCL:486686530:BASE:486954198:1667923752339:e2b79b0f).

This includes https://github.com/deepmind/mujoco internal testing with Windows clang 15.0.3, Debian libc++, Ubuntu libc++, which are not covered in the GitHub CI here.

Added block transferred from PR pybind#4321

rwgk · 2022-11-10T07:27:58Z

The CI failure is a rare flake (PyPy refcount mismatch). Ignoring.

…ybind#4319)

rwgk · 2023-03-28T16:57:24Z

@ax3l @rwgk Have we opened a public bug on LLVM yet?

Not AFAIK.

They are usually responsive. Even if it does not solve our issue today, we will thank our future selves when it does eventually make into our oldest supported Clang.

I put what I have in the previous comment. Could someone maybe help?

These are actually 2 bugs, in the same part of the clang code probably, but different I think.

rwgk · 2023-03-29T00:30:28Z

@ax3l @henryiii @lalaland @Skylion007 @wjakob

What's your vote on this PR?

The production code change is really minimal. It boils down to: for Python 3.12+ we're fixing an MSVC problem (binding types defined in the unnamed namespace).

The only downside I see: it's a little more involved to reason about pybind11 behavior under Windows, 1. simply because we're makaing a change and 2. conditional indirectly on the Python version.

EthanSteinberg · 2023-03-29T15:46:27Z

I think this looks good to me!

henryiii

I'm okay with it.

Skylion007 · 2023-03-29T20:59:11Z

Are we sure it's a LLVM or could it be a GCC bug btw?

rwgk · 2023-03-30T06:53:42Z

Are we sure it's a LLVM or could it be a GCC bug btw?

I looked at the internal discussion from November last year again. It concludes with this sentence:

So, I guess clang+libc++ could improve the implementation that uses strcmp.

This came from someone apparently very familiar with clang and libc++, based on other things they wrote (including pointers to libc++ sources).

That makes me think: yes, it's almost certainly a libc++ issue.

…RSION=10000000

rwgk · 2023-03-30T17:04:26Z

@henryiii what's you opinion on 0ff73a9 (removes PYBIND11_TEST_OVERRIDE)?

I did that so that we get to see differences like this (🐍 3.11 • windows-2022 • x64, ~randomly picked):

C++ Info: MSVC 193431943 C++17 __pybind11_internals_v4_msvc_debug__ PYBIND11_SIMPLE_GIL_MANAGEMENT=False
...
..\..\tests\test_unnamed_namespace_a.py Xx.x                             [ 98%]

C++ Info: MSVC 193431943 C++17 __pybind11_internals_v10000000_msvc_debug__ PYBIND11_SIMPLE_GIL_MANAGEMENT=False
...
..\..\tests\test_unnamed_namespace_a.py ....                             [ 98%]

The downside is that we don't have any PYBIND11_TEST_OVERRIDE anymore in GHA. Do we want to add one back, but decoupled from PYBIND11_INTERNALS_VERSION?

… windows Extra work added to quick jobs, based on timings below, to not increase the GHA start-to-last-job-finished time. ``` Duration ^ Number of pytest runs ^ ^ Job identifier ^ ^ ^ 0:03:48.024227 1 1___3___Clang_3.6___C++11___x64.txt 0:03:58.992814 1 2___3___Clang_3.7___C++11___x64.txt 0:04:25.758942 1 1___3.7___Debian___x86____Install.txt 0:04:50.148276 1 4___3___Clang_7___C++11___x64.txt 0:04:55.784558 1 13___3___Clang_15___C++20___x64.txt 0:04:57.048754 1 6___3___Clang_dev___C++11___x64.txt 0:05:00.485181 1 7___3___Clang_5___C++14___x64.txt 0:05:03.744964 1 2___3___almalinux8___x64.txt 0:05:06.222752 1 5___3___Clang_9___C++11___x64.txt 0:05:11.767022 1 2___3___GCC_7___C++17__x64.txt 0:05:18.634930 1 2___3.11__deadsnakes____x64.txt 0:05:22.810995 1 1___3___GCC_7___C++11__x64.txt 0:05:25.275317 1 12___3___Clang_14___C++20___x64.txt 0:05:32.058174 1 5___3___GCC_10___C++17__x64.txt 0:05:39.381351 1 7___3___GCC_12___C++20__x64.txt 0:05:40.502252 1 8___3___Clang_10___C++17___x64.txt 0:05:59.344905 1 3___3___Clang_3.9___C++11___x64.txt 0:06:10.825147 1 6___3___GCC_11___C++20__x64.txt 0:06:20.655443 1 3___3___almalinux9___x64.txt 0:06:22.472061 1 3___3___GCC_8___C++14__x64.txt 0:06:42.647406 1 11___3___Clang_13___C++20___x64.txt 0:06:53.352720 1 1___3.10___CUDA_11.7___Ubuntu_22.04.txt 0:07:07.357801 1 2___3.7___MSVC_2019___x86_-DCMAKE_CXX_STANDARD=14.txt 0:07:09.057603 1 1___3___centos7___x64.txt 0:07:15.546282 1 1___3.8___MSVC_2019__Debug____x86_-DCMAKE_CXX_STANDARD=17.txt 0:07:22.566022 1 4___3___GCC_8___C++17__x64.txt 0:08:13.592674 1 2___3.9___MSVC_2019__Debug____x86_-DCMAKE_CXX_STANDARD=20.txt 0:08:16.422768 1 9___3___Clang_11___C++20___x64.txt 0:08:21.168457 1 3___3.8___MSVC_2019___x86_-DCMAKE_CXX_STANDARD=17.txt 0:08:27.129468 1 10___3___Clang_12___C++20___x64.txt 0:09:35.045470 1 1___3.10___windows-latest___clang-latest.txt 0:09:57.361843 1 1___3.9___MSVC_2022_C++20___x64.txt 0:10:35.187767 1 1___3.6___MSVC_2019___x86.txt 0:11:14.691200 4 2___3.9___ubuntu-20.04___x64.txt 0:11:37.701167 1 1_macos-latest___brew_install_llvm.txt 0:11:38.688299 4 4___3.11___ubuntu-20.04___x64.txt 0:11:52.720216 1 4___3.9___MSVC_2019___x86_-DCMAKE_CXX_STANDARD=20.txt 0:13:23.456591 4 6___pypy-3.8___ubuntu-20.04___x64_-DPYBIND11_FINDPYTHON=ON.txt 0:13:25.863592 2 1___3___ICC_latest___x64.txt 0:13:32.411758 3 9___3.9___windows-2022___x64.txt 0:13:45.473377 4 3___3.10___ubuntu-20.04___x64.txt 0:13:55.366447 4 5___pypy-3.7___ubuntu-20.04___x64.txt 0:13:57.969502 3 10___3.10___windows-2022___x64.txt 0:14:19.837475 3 11___3.11___windows-2022___x64.txt 0:14:33.316770 4 1___3.6___ubuntu-20.04___x64_-DPYBIND11_FINDPYTHON=ON_-DCMA.txt 0:15:34.449278 4 22___3.6___windows-2019___x64_-DPYBIND11_FINDPYTHON=ON.txt 0:16:25.189055 2 1___3.9-dbg__deadsnakes____Valgrind___x64.txt 0:17:20.956667 4 15___3.6___macos-latest___x64.txt 0:17:27.513891 4 23___3.9___windows-2019___x64.txt 0:17:58.783286 3 8___3.6___windows-2022___x64.txt 0:18:25.917828 4 7___pypy-3.9___ubuntu-20.04___x64.txt 0:19:17.399820 3 13___pypy-3.8___windows-2022___x64.txt 0:19:45.002122 3 12___pypy-3.7___windows-2022___x64.txt 0:20:03.201926 4 16___3.9___macos-latest___x64.txt 0:20:15.415178 4 17___3.10___macos-latest___x64.txt 0:20:20.263216 4 20___pypy-3.8___macos-latest___x64.txt 0:20:31.998226 3 1___3___windows-latest___mingw64.txt 0:20:40.812286 4 18___3.11___macos-latest___x64.txt 0:22:47.714749 4 19___pypy-3.7___macos-latest___x64.txt 0:23:04.435859 3 2___3___windows-latest___mingw32.txt 0:25:48.719597 3 14___pypy-3.9___windows-2022___x64.txt 0:26:01.211688 4 21___pypy-3.9___macos-latest___x64.txt 0:28:19.971015 1 1___3___CentOS7__PGI_22.9___x64.txt ```

rwgk · 2023-04-21T07:21:16Z

The downside is that we don't have any PYBIND11_TEST_OVERRIDE anymore in GHA. Do we want to add one back, but decoupled from PYBIND11_INTERNALS_VERSION?

I didn't get any feedback to this question, so I decided to try d0276c0. The 3 CI logs look good (https://github.com/pybind/pybind11/actions/runs/4762077158: 3.9___MSVC_2022_C++20___x64.txt, macos-latest___brew_install_llvm.txt, 3___GCC_12___C++20__x64.txt). I'll break out the ci.yml change as a separate PR.

The Python 3.12 failure (Upstream workflow) is expected and needs special attention (separate PR).

…arate PR).

rwgk · 2023-04-25T21:03:18Z

Thanks for the reviews @henryiii, @lalaland, @Skylion007!

With #4635 merged today, I think this PR is also good to get merged. I'll go ahead now.

The only non-reviewed tweak is a one-character change for Python 3.12 testing (skipif for alpha 7 instead of 6): 1b4a508

With that Python 3.12 testing passes.

Try using std::hash<std::type_index>, `std::equal_to<std::type_inde…

a06949a

…x>` everywhere. From PR pybind#4316 we know that types in the unnamed namespace in different translation units do not compare equal, as desired. But do types in named namespaces compare equal, as desired?

This was referenced Nov 7, 2022

Minimal reproducer for clash when binding types defined in the unnamed namespace. #4313

Closed

[Experiment] std::type_index & un/named namespace #4316

Closed

Ralf W. Grosse-Kunstleve added 2 commits November 6, 2022 21:22

Revert "Try using std::hash<std::type_index>, `std::equal_to<std::t…

9d70eba

…ype_index>` everywhere." This reverts commit a06949a.

Use "our own name-based hash and equality functions" for `std::type_i…

9141519

…ndex` only under macOS, based on results shown under pybind#4316 (comment)

Ralf W. Grosse-Kunstleve added 3 commits November 7, 2022 03:36

Patch in PR pybind#4313: Minimal reproducer for clash when binding ty…

ba747dc

…pes defined in the unnamed namespace.

test_unnamed_namespace_b xfail for clang

1912670

PYBIND11_INTERNALS_VERSION 5

d8b7118

rwgk changed the title ~~Try using std::hash<std::type_index>, std::equal_to<std::type_index> everywhere.~~ Use std::hash<std::type_index>, std::equal_to<std::type_index> everywhere **except macOS** Nov 7, 2022

rwgk added the abi break label Nov 7, 2022

Add a note to docs/classes.rst

780d3b2

Skylion007 reviewed Nov 7, 2022

View reviewed changes

Ralf W. Grosse-Kunstleve added 2 commits November 7, 2022 10:14

For compatibility with Google-internal testing, test_unnamed_namespac…

3373b47

…e_a & test_unnamed_namespace_b need to work when imported in any order.

Trying "__GLIBCXX__ or Windows", based on observations from Google-in…

8f0c28e

…ternal testing.

Ralf W. Grosse-Kunstleve added 2 commits November 7, 2022 14:04

Try _LIBCPP_VERSION

883cadf

Account for libc++ behavior in tests and documentation.

72a8895

rwgk changed the title Use std::hash<std::type_index>, std::equal_to<std::type_index> everywhere **except macOS** Use std::hash<std::type_index>, std::equal_to<std::type_index> everywhere **except when libc++ is in use** Nov 7, 2022

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Nov 9, 2022

Patching in snapshot of PR pybind#4319

5aa6093

Ralf W. Grosse-Kunstleve added 3 commits November 9, 2022 21:06

Adjust expectations for Windows Clang (and make code less redundant).

6f1ddb2

Add WindowsClang to ci.yml

a1175ed

Added block transferred from PR pybind#4321

Add clang-latest to name that appears in the GitHub Actions web view.

5631d94

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Nov 10, 2022

Add windows_clang to ci.yml (previously tested under PRs pybind#4321, p…

f18b3e9

…ybind#4319)

rwgk mentioned this pull request Nov 10, 2022

Add windows_clang to ci.yml #4323

Merged

Tweak the note in classes.rst again.

81a4e7b

Ralf W. Grosse-Kunstleve added 2 commits March 28, 2023 15:45

Merge branch 'master' into internals_std_type_index_modernization

66e0fac

Make updated condition in internals.h dependent on ABI version.

f1c3055

rwgk removed the abi break label Mar 29, 2023

rwgk marked this pull request as ready for review March 29, 2023 00:22

rwgk requested a review from henryiii as a code owner March 29, 2023 00:22

henryiii approved these changes Mar 29, 2023

View reviewed changes

Skylion007 approved these changes Mar 29, 2023

View reviewed changes

Merge branch 'master' into internals_std_type_index_modernization

4842b9f

rwgk added the python dev Working on development versions of Python label Mar 30, 2023

Skylion007 mentioned this pull request Mar 30, 2023

Differing behavior of std::type_index hash and equality from libc++ and libstdc++ llvm/llvm-project#61848

Closed

Remove PYBIND11_TEST_OVERRIDE when testing with PYBIND11_INTERNALS_VE…

0ff73a9

…RSION=10000000

Ralf W. Grosse-Kunstleve added 2 commits April 20, 2023 22:08

Merge branch 'master' into internals_std_type_index_modernization

9f60e9a

rwgk mentioned this pull request Apr 21, 2023

Fully test unstable ABI #4635

Merged

Ralf W. Grosse-Kunstleve added 2 commits April 25, 2023 10:32

Merge branch 'master' into internals_std_type_index_modernization

4f4fd04

Update skipif for Python 3.12a7 (the WIP needs to be handled in a sep…

1b4a508

…arate PR).

rwgk merged commit 6de6191 into pybind:master Apr 25, 2023

rwgk deleted the internals_std_type_index_modernization branch April 25, 2023 21:03

github-actions bot added the needs changelog Possibly needs a changelog entry label Apr 25, 2023

rwgk removed the needs changelog Possibly needs a changelog entry label May 22, 2023

henryiii added needs changelog Possibly needs a changelog entry and removed needs changelog Possibly needs a changelog entry labels Mar 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rwgk commented Nov 6, 2022 •

edited

Loading

rwgk commented Nov 7, 2022

rwgk commented Nov 7, 2022

Skylion007 Nov 7, 2022 •

edited

Loading

rwgk Nov 7, 2022

Skylion007 Nov 7, 2022

rwgk Nov 7, 2022

rwgk Nov 10, 2022

ax3l Mar 20, 2023 •

edited

Loading

rwgk Mar 20, 2023

rwgk commented Nov 7, 2022

rwgk commented Nov 7, 2022 •

edited

Loading

rwgk commented Nov 8, 2022

rwgk commented Nov 10, 2022

rwgk commented Mar 28, 2023

rwgk commented Mar 29, 2023

EthanSteinberg commented Mar 29, 2023

henryiii left a comment

Skylion007 commented Mar 29, 2023

rwgk commented Mar 30, 2023

rwgk commented Mar 30, 2023

rwgk commented Apr 21, 2023

rwgk commented Apr 25, 2023

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

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

Conversation

rwgk commented Nov 6, 2022 • edited Loading

Description

Suggested changelog entry:

rwgk commented Nov 7, 2022

rwgk commented Nov 7, 2022

Skylion007 Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

rwgk Nov 7, 2022

Choose a reason for hiding this comment

Skylion007 Nov 7, 2022

Choose a reason for hiding this comment

rwgk Nov 7, 2022

Choose a reason for hiding this comment

rwgk Nov 10, 2022

Choose a reason for hiding this comment

ax3l Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

rwgk Mar 20, 2023

Choose a reason for hiding this comment

rwgk commented Nov 7, 2022

rwgk commented Nov 7, 2022 • edited Loading

rwgk commented Nov 8, 2022

rwgk commented Nov 10, 2022

rwgk commented Mar 28, 2023

rwgk commented Mar 29, 2023

EthanSteinberg commented Mar 29, 2023

henryiii left a comment

Choose a reason for hiding this comment

Skylion007 commented Mar 29, 2023

rwgk commented Mar 30, 2023

rwgk commented Mar 30, 2023

rwgk commented Apr 21, 2023

rwgk commented Apr 25, 2023

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

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

rwgk commented Nov 6, 2022 •

edited

Loading

Skylion007 Nov 7, 2022 •

edited

Loading

ax3l Mar 20, 2023 •

edited

Loading

rwgk commented Nov 7, 2022 •

edited

Loading