search

pybind / pybind11

Seamless operability between C++11 and Python
https://pybind11.readthedocs.io/
Other
15.56k stars 2.09k forks source link

fix: use manual padding of instance_map_shard #5200

Closed colesbury closed 3 months ago

colesbury commented 3 months ago

Description

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

See https://github.com/scipy/scipy/issues/21056.

Suggested changelog entry:

Avoid aligned allocation in free-threaded build in order to support macOS versions before 10.14.
colesbury commented 3 months ago

@henryiii, should this target v2.13?

henryiii commented 3 months ago

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

henryiii commented 3 months ago

Ahh, you manually padded.

colesbury commented 3 months ago

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

henryiii commented 3 months ago

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

henryiii commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

rwgk commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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: https://github.com/pypa/manylinux/pull/1639