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

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

Closed
wants to merge 4 commits into from

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Nov 4, 2022

Description

This PR

  • works as expected only with gcc, but
  • fails with any clang in otherwise identical environments, and also
  • fails with any MSVC.

The root cause for the failures is:

Full details of which platform is or is not using the __GLIBCXX__ branch: #4313 (comment)

Unknown: How could this be fixed?

@wjakob Are you aware of this situation already? — Reason for bringing this up now: If the internals version is bumped, this would be a good fix to include (assuming the fix might be an ABI break, which isn't clear until we have a fix).

@charlesbeattie FYI It turns out the root cause is not what we thought it is when we looked at this back in February.

Suggested changelog entry:

@rwgk rwgk requested a review from wjakob November 4, 2022 11:34
@rwgk
Copy link
Collaborator Author

rwgk commented Nov 4, 2022

Note re CI failures: 1 flake: CI / 🐍 3.10 • ubuntu-latest • x64

@rwgk
Copy link
Collaborator Author

rwgk commented Nov 4, 2022

@henryiii @Skylion007 @lalaland for awareness / comment.

```
g++ (Debian 12.2.0-3) 12.2.0
============================
LOOOK __GLIBCXX__ [lhs=N12_GLOBAL__N_110any_structE] /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/detail/../detail/internals.h:124
LOOOK __GLIBCXX__ [rhs=N12_GLOBAL__N_110any_structE] /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/detail/../detail/internals.h:125
LOOOK __GLIBCXX__ [(lhs==rhs)=FALSE] /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/detail/../detail/internals.h:129

Debian clang version 14.0.6-2
=============================
LOOOK __GLIBCXX__ [lhs=N12_GLOBAL__N_110any_structE] /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/detail/internals.h:124
LOOOK __GLIBCXX__ [rhs=N12_GLOBAL__N_110any_structE] /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/detail/internals.h:125
LOOOK __GLIBCXX__ [(lhs==rhs)=TRUE] /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/detail/internals.h:129
ImportError while loading conftest '/usr/local/google/home/rwgk/forked/pybind11/tests/conftest.py'.
conftest.py:16: in <module>
    import pybind11_tests
E   ImportError: generic_type: type "unnamed_namespace_b_any_struct" is already registered!
```
@rwgk
Copy link
Collaborator Author

rwgk commented Nov 4, 2022

For easy reference, the CI results for the current "Debug printf()s" commit are here (everything failed, as expected):

https://github.com/pybind/pybind11/actions/runs/3393602034/jobs/5641129754

The name of the "Download log archive" file: logs_24742.zip

Results of inspecting the logs:

$ grep 'LOOOK __GLIBCXX__' *.txt | sed 's/^[1-9][0-9]*//' | cutniq | sort
______3.10_____CUDA_11.7_____Ubuntu_22.04.txt
______3.10_____ubuntu-latest_____x64.txt
______3.11__deadsnakes______x64.txt
______3.11_____ubuntu-latest_____x64.txt
______3.6_____ubuntu-latest_____x64_-DPYBIND11_FINDPYTHON=ON_-DCM.txt
______3.7_____Debian_____x86______Install.txt
______3.9-dbg__deadsnakes______Valgrind_____x64.txt
______3.9_____ubuntu-latest_____x64.txt
______3_____almalinux8_____x64.txt
______3_____almalinux9_____x64.txt
______3_____CentOS7__PGI_22.9_____x64.txt
______3_____centos7_____x64.txt
______3_____Clang_10_____C++17_____x64.txt
______3_____Clang_10_____C++20_____x64.txt
______3_____Clang_11_____C++20_____x64.txt
______3_____Clang_12_____C++20_____x64.txt
______3_____Clang_13_____C++20_____x64.txt
______3_____Clang_14_____C++20_____x64.txt
______3_____Clang_3.6_____C++11_____x64.txt
______3_____Clang_3.7_____C++11_____x64.txt
______3_____Clang_3.9_____C++11_____x64.txt
______3_____Clang_5_____C++14_____x64.txt
______3_____Clang_7_____C++11_____x64.txt
______3_____Clang_9_____C++11_____x64.txt
______3_____Clang_dev_____C++11_____x64.txt
______3_____GCC_10_____C++17____x64.txt
______3_____GCC_11_____C++20____x64.txt
______3_____GCC_12_____C++20____x64.txt
______3_____GCC_7_____C++11____x64.txt
______3_____GCC_7_____C++17____x64.txt
______3_____GCC_8_____C++14____x64.txt
______3_____GCC_8_____C++17____x64.txt
______3_____ICC_latest_____x64.txt
______3_____windows-latest_____mingw32.txt
______3_____windows-latest_____mingw64.txt
______pypy-3.7_____ubuntu-latest_____x64.txt
______pypy-3.8_____ubuntu-latest_____x64_-DPYBIND11_FINDPYTHON=ON.txt
______pypy-3.9_____ubuntu-latest_____x64.txt
$ grep 'LOOOK ___OTHER___' *.txt | sed 's/^[1-9][0-9]*//' | cutniq | sort
______3.10_____macos-latest_____x64.txt
______3.10_____windows-2022_____x64.txt
______3.11_____macos-latest_____x64.txt
______3.11_____windows-2022_____x64.txt
______3.6_____macos-latest_____x64.txt
______3.6_____MSVC_2019_____x86.txt
______3.6_____windows-2019_____x64_-DPYBIND11_FINDPYTHON=ON.txt
______3.6_____windows-2022_____x64.txt
______3.7_____MSVC_2019_____x86_-DCMAKE_CXX_STANDARD=14.txt
______3.8_____MSVC_2019__Debug______x86_-DCMAKE_CXX_STANDARD=17.txt
______3.8_____MSVC_2019_____x86_-DCMAKE_CXX_STANDARD=17.txt
______3.9_____macos-latest_____x64.txt
______3.9_____MSVC_2019__Debug______x86_-DCMAKE_CXX_STANDARD=20.txt
______3.9_____MSVC_2019_____x86_-DCMAKE_CXX_STANDARD=20.txt
______3.9_____MSVC_2022_C++20_____x64.txt
______3.9_____windows-2019_____x64.txt
______3.9_____windows-2022_____x64.txt
______pypy-3.7_____macos-latest_____x64.txt
______pypy-3.7_____windows-2022_____x64.txt
______pypy-3.8_____macos-latest_____x64.txt
______pypy-3.8_____windows-2022_____x64.txt
______pypy-3.9_____macos-latest_____x64.txt
______pypy-3.9_____windows-2022_____x64.txt

Minor detail, the expected failures are (e.g.):

2022-11-04T12:05:02.8744572Z FAILED ../../tests/test_iostream.py::test_flush - AssertionError: assert '\nL...
2022-11-04T12:05:02.8744990Z FAILED ../../tests/test_iostream.py::test_redirect - AssertionError: assert '...
2022-11-04T12:05:02.8745293Z FAILED ../../tests/test_iostream.py::test_redirect_err - AssertionError: asse...
2022-11-04T12:05:02.8745615Z FAILED ../../tests/test_iostream.py::test_redirect_both - AssertionError: ass...

@EthanSteinberg
Copy link
Collaborator

Just to make sure I am understanding this correctly, the problem is that std::type_info is not unique in Clang?

@rwgk
Copy link
Collaborator Author

rwgk commented Nov 5, 2022

Just to make sure I am understanding this correctly, the problem is that std::type_info is not unique in Clang?

Yes, I believe so. Initially I wasn't sure at all, but I asked internally and it looks like this is an extremely long-lived clang bug. I'm still not completely sure, but the evidence is pretty obvious.

I found this with a Google search and I'm wondering if that's a fix: https://reviews.llvm.org/D122775

I tried clang-15 in a container that was apparently built in July, but that still had the problem. Again, I'm not sure yet what the situation is. Maybe I'll ask around more next week.

We also have a problem with the ! __GLIBCXX__ branch that's being used under Windows and macOS. I don't know if that can be fixed: how can we get unique names/ids/identifiers of some sort?

The easiest way out maybe: clearly document the issue. It's easy to work around when you know.

@EthanSteinberg
Copy link
Collaborator

The easiest way out maybe: clearly document the issue. It's easy to work around when you know.

Can we detect classes / structs that belong to unnamed scopes and throw an error message when users try to register one in Clang?

If we can just throw an appropriate error message, we don't even need to document this as any user could just see the error message.

@rwgk
Copy link
Collaborator Author

rwgk commented Nov 6, 2022

Can we detect classes / structs that belong to unnamed scopes and throw an error message when users try to register one in Clang?

If we can just throw an appropriate error message, we don't even need to document this as any user could just see the error message.

Interesting question.

Note that we're generating an error message already, it's just not immediately obvious why, e.g.:

ImportError: generic_type: type "unnamed_namespace_b_any_struct" is already registered!

We could look for these substrings in the mangled names:

clang (and gcc):

N12_GLOBAL__N_1

Windows:

`anonymous namespace'

Then add a hint to that error message, something like "Note that this type is in an unnamed namespace, which may not be handled correctly on some platforms. See for more information."

But inspecting the mangled names is a pretty ugly heuristic. I'm thinking, if we cannot find workarounds/fixes for all platforms, it's better to add a small section to the docs explaining the likely error case (class_ in two extensions) and the limitations for unnamed namespaces, and point to that unconditionally from the error message.

FWIW, while looking around just now I found this:

boost seems to have very comprehensive tools for working with mangled names. But again, I'm not sure it's worth the effort including long-term maintenance to fold this into the code for the pybind11 type registry.

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Nov 6, 2022
@rwgk
Copy link
Collaborator Author

rwgk commented Nov 6, 2022

For easy reference, link to comment under [Experiment] boost::typeindex::type_index & unnamed namespace:

Takeaway: I don't think there is much hope that unnamed namespace clashes can be fixed.

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Nov 6, 2022
@rwgk
Copy link
Collaborator Author

rwgk commented Nov 6, 2022

EDIT: At second thought: this only proves 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?


Original message:
Good news, I think:

I think we can make internals.h simpler & more correct, by removing the !__GLIBCXX__ code path entirely. I'll try this later in another PR.

@rwgk
Copy link
Collaborator Author

rwgk commented Nov 7, 2022

PR #4319 first observation: only macOS fails, and only test_local_bindings.py FFF.FF...F

@rwgk
Copy link
Collaborator Author

rwgk commented Nov 7, 2022

The unnamed namespace clang bug is still not fixed even in the latest nightly builds for clang-15 and clang-16, using PR #4316 for testing:

========================================================= short test summary info ==========================================================
SKIPPED [1] test_callbacks.py:203: Current PYBIND11_INTERNALS_VERSION too low
SKIPPED [1] test_exc_named_namespace_a.py:10: std::type_index-EQ-GOOD
SKIPPED [1] test_stl.py:143: no <experimental/optional>
SKIPPED [1] test_stl.py:175: no <boost/optional>
SKIPPED [1] test_unnamed_namespace_a.py:11: std::type_index-EQ-BAD
===================================================== 789 passed, 5 skipped in 11.21s ======================================================
root@53f2abca540e:/build_clang16# clang++-15 --version
Debian clang version 15.0.4-++20221102053030+5c68a1cb1231-1~exp1~20221102053120.92
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
root@53f2abca540e:/build_clang16# clang++-16 --version
Debian clang version 16.0.0-++20221106071805+b5626ae9751f-1~exp1~20221106071907.446
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
root@53f2abca540e:/build_clang16# cat /etc/os-release
PRETTY_NAME="Debian GNU/Linux 11 (bullseye)"
NAME="Debian GNU/Linux"
VERSION_ID="11"
VERSION="11 (bullseye)"
VERSION_CODENAME=bullseye
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Nov 7, 2022
@rwgk rwgk closed this Nov 7, 2022
rwgk pushed a commit that referenced this pull request Apr 25, 2023
…erywhere **except when libc++ is in use** (#4319)

* Try using `std::hash<std::type_index>`, `std::equal_to<std::type_index>` everywhere.

From PR #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?

* Revert "Try using `std::hash<std::type_index>`, `std::equal_to<std::type_index>` everywhere."

This reverts commit a06949a.

* Use "our own name-based hash and equality functions" for `std::type_index` only under macOS, based on results shown under #4316 (comment)

* Patch in PR #4313: Minimal reproducer for clash when binding types defined in the unnamed namespace.

* test_unnamed_namespace_b xfail for clang

* `PYBIND11_INTERNALS_VERSION 5`

* Add a note to docs/classes.rst

* For compatibility with Google-internal testing, test_unnamed_namespace_a & test_unnamed_namespace_b need to work when imported in any order.

* Trying "__GLIBCXX__ or Windows", based on observations from Google-internal testing.

* Try _LIBCPP_VERSION

* Account for libc++ behavior in tests and documentation.

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

* Add WindowsClang to ci.yml

Added block transferred from PR #4321

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

* Tweak the note in classes.rst again.

* Add `pip install --upgrade pip`, Show env, cosmetic changes

Already tested under PR #4321

* Add macos_brew_install_llvm to ci.yml

Added block transferred from PR #4324

* `test_cross_module_exception_translator` xfail 'Homebrew Clang'

* Revert back to base version of .github/workflows/ci.yml (the ci.yml changes were merged under #4323 and #4326)

* Fixes for ruff

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

* Remove PYBIND11_TEST_OVERRIDE when testing with PYBIND11_INTERNALS_VERSION=10000000

* Selectively exercise cmake `-DPYBIND11_TEST_OVERRIDE`: ubuntu, macos, 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
```

* Update skipif for Python 3.12a7 (the WIP needs to be handled in a separate PR).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants