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

fix: use manual padding of instance_map_shard #5200

Merged
merged 4 commits into from
Jun 26, 2024

Conversation

colesbury
Copy link
Contributor

Description

The alignas(64) specifier requires aligned allocation, which is not available on macOS when targeting versions before 10.14.

See scipy/scipy#21056.

Suggested changelog entry:

Avoid aligned allocation in free-threaded build in order to support macOS versions before 10.14.

The alignas(64) specifier requires aligned allocation, which is not
available on macOS when targeting versions before 10.14.
@colesbury
Copy link
Contributor Author

@henryiii, should this target v2.13?

@henryiii
Copy link
Collaborator

No, we can backport. Would it make sense to align as on linux?

@henryiii
Copy link
Collaborator

Ahh, you manually padded.

@colesbury
Copy link
Contributor Author

@rgommers mentioned that they've run into multiple issues related to aligned allocations, so I think it'd be safer to just stick with manual padding everywhere, even if we don't know of any issues right now.

include/pybind11/detail/internals.h Outdated Show resolved Hide resolved
@henryiii
Copy link
Collaborator

How hard would it be to also move to PyMutex in 2.13.1?

@henryiii
Copy link
Collaborator

I think it'd be safer to just stick with manual padding everywhere, even if we don't know of any issues right now

Yeah, I missed that this was manually padded, looks fine.

I wonder if [[maybe_unused]] (via PYBIND11_MAYBE_USED) would be reasonable to apply to the padding member?

@colesbury
Copy link
Contributor Author

How hard would it be to also move to PyMutex in 2.13.1?

I think it'll be pretty easy, but we should wait until 3.13 beta 3 is released and available in manylinux.

@henryiii
Copy link
Collaborator

Okay. Versions numbers are cheap, that's fine for a 2.13.2. :)

@rwgk
Copy link
Collaborator

rwgk commented Jun 26, 2024

@henryiii Ideally we'd have a macos-13 Python 3.13t job in the CI (separate PR), corresponding to the scipy CI job that uncovered the problem solved here. Did you try that already?

@henryiii
Copy link
Collaborator

macos-13 Python 3.13t

This isn't that easy, setup-python doesn't support it, so we'd need cibuildwheel. It probably wouldn't be as hard if #4745 was possible.

@henryiii henryiii merged commit 2e35470 into pybind:master Jun 26, 2024
84 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jun 26, 2024
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jun 26, 2024
henryiii added a commit that referenced this pull request Jun 26, 2024
* Use manual padding of instance_map_shard.

The alignas(64) specifier requires aligned allocation, which is not
available on macOS when targeting versions before 10.14.

* Add 'see #5200'

* Update include/pybind11/detail/internals.h

* Update include/pybind11/detail/internals.h

---------

Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
@colesbury colesbury deleted the alignas branch June 27, 2024 17:24
@henryiii
Copy link
Collaborator

henryiii commented Jun 28, 2024

I think it'll be pretty easy, but we should wait until 3.13 beta 3 is released and available in manylinux.

Going into manylinux here: pypa/manylinux#1639

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.

3 participants