-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
macOS: disable fixup chains when linking extension modules #4301
base: master
Are you sure you want to change the base?
Conversation
FWIW this commit only fixes the pybind11 parts. If somebody uses the CMake linker function |
If somebody on the Google or CMake team (or elsewhere) has an Apple support ticket to spend, this might be an excellent use for it. I have many questions:
I've not been able to find much concrete information on these very low level Darwin dynamic linker changes. It would be great to route this to a person at Apple who knows the nitty gritty details of the dynamic linker and can tell us what to do here. (@rwgk) |
FYI: I just posted a question internally to ask about this. Waiting. |
Issue on the CPython repository: python/cpython#97524 |
Note that See https://developer.apple.com/forums/thread/689991, ocaml/ocaml#10723, Homebrew/brew#12225. CC @aconchillo, since you might have more insight here. |
Using |
Following the trail of experimentation, I made a nanobind version that uses a "ld response file" to more efficiently inform Link: wjakob/nanobind@ccb1ac5 ( The commit also contains a script With these changes, nanobind compiles without warnings and passes the test suite with a deployment target of OSX 12. I would expect the same behavior for pybind11. What remains unclear to me is whether the absence of a warning means that we are now safe of undefined behavior, or if somebody simply forgot to implement that same warning message for the |
One more update: one can inspect the rebase and binding tables using the command
In particular, note the Three different styles of linker flags are under consideration at this point:
Preliminary analysis using This means that the main benefit of approach #2 over #1 is that an error message would be generated if there are other undefined symbols besides CPython API functions. The current flag What I find curious is that option #1 (
while option #2 (specifying Whether or not #1 and #2 will lead to undefined behavior together with chained fixups remains unclear. We will have to wait for Apple's response on whether this just a fluke. In contrast, option #3 proposed in this PR ( |
…space On macOS, extension library builds usually rely on the flag ``-undefined dynamic_lookup`` to get the linker to temporarily accept missing CPython API symbols. As the name implies, those symbols are then dynamically resolved when Python imports such an extension library. Binaries produced in this way are more portable since they don't contain a hardcoded path to the CPython library. This is critical when distributing binary wheels on PyPI, etc.. When targeting macOS>=12, XCode recently started to generate a somewhat ominous warning: ``-undefined dynamic_lookup may not work with chained fixups`` This suggested that the dynamic resolution functionality became broken. I reported this to Apple via feedback request FB11767124 and switched nanobind to an alternative set of flags (``-undefined suppress -flat_namespace``) in the meantime. The flat namespace setup has various limitations though, so this was not a satisfactory long-term solution. Apple investigated the request and updated the behavior of ``ld`` starting with XCode 14.3b1+: it now disables the fixup chain linker optimization whenever ``-undefined dynamic_lookup`` is specified. The feedback from Apple also stated that the parameter ``-Wl,-no_fixup_chains`` should be specified on pre-14.3 XCode versions to ensure correct behavior. This commit realizes those changes by reverting to a two-level namespace for the overall linking process, checking the XCode version, and potentially manually disabling fixup chains when needed. Related discussion is available in the pybind11 (pybind/pybind11#4301) and CPython repositories (python/cpython#97524).
ad1bb26
to
70a0a37
Compare
I amended this commit to remove the |
On macOS, extension library builds usually rely on the flag ``-undefined dynamic_lookup`` to get the linker to temporarily accept missing CPython API symbols. As the name implies, those symbols are then dynamically resolved when Python imports such an extension library. Binaries produced in this way are more portable since they don't contain a hardcoded path to the CPython library. This is critical when distributing binary wheels on PyPI, etc.. When targeting macOS>=12, XCode recently started to generate a somewhat ominous warning: ``-undefined dynamic_lookup may not work with chained fixups`` For further detail on what chained fixups are, see the following informative WWDC https://developer.apple.com/videos/play/wwdc2022/110362/ (the relevant part starts about 20mins into the video) The message that the dynamic resolution functionality became broken. I reported this to Apple via feedback request FB11767124. Apple investigated the request and updated the behavior of ``ld`` starting with XCode 14.3b1+: it now disables the fixup chain linker optimization whenever ``-undefined dynamic_lookup`` is specified. The feedback from Apple also stated that the parameter ``-Wl,-no_fixup_chains`` should be specified on pre-14.3 XCode versions to ensure correct behavior. This commit realizes those changes by passing a flag to *always* disable fixup chains. Related discussion is available in the CPython repository (python/cpython#97524).
70a0a37
to
e076c45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to check how older versions of Apple ld
handle the -no_fixup_chains
flag. Could produce errors for being an unrecognised flag.
How much older? From what I understand, the latest version's don't require the flag (they default to this behavior to help dynamic languages that use this), and this just hides the warning for older versions. So (probably answering the question I posed above), I'd expect this would work on versions that produce the warning, so the thing to test is versions before this warning started showing up? |
Anything older than Xcode 14, I think. We can check the output of
If it's Reference: https://en.wikipedia.org/wiki/Xcode#Xcode_11.0_-_14.x_(since_SwiftUI_framework)_2 |
Actually, based on testing on GitHub runners, the flag seems to be supported as early as Edit: Based on testing various versions of Xcode, you can pass |
So we should only add this flag if Xcode is between 13 and 14.2 (14.3+ always have this, so there's no need to add it there, I'd guess?) We can't really detect the linker version, but we could base it on the compiler version, as they should match. |
Only if the user is using Apple Clang. That's probably the most common case, but it's not guaranteed (e.g. a user may have $CC -Wl,-v though. |
I think it's pretty safe to only add this check when using AppleClang between 13 and 14.2, as the version there is pre-cached and doesn't require us to run an extra shell command. If we miss adding this, then the user just sees a warning, so it's not that terrible, IMO, more of a minor quality of life improvement. |
FYI, the source of the warning is finally understood -- please see this comment on the CPython tracker for details: python/cpython#97524 (comment) To retain chained fixups while fixing the issue, I use the following solution in nanobind: wjakob/nanobind@2f29ec7 |
On macOS, extension library builds usually rely on the flag
-undefined dynamic_lookup
to get the linker to temporarily accept missing CPython API symbols. As the name implies, those symbols are then dynamically resolved when Python imports such an extension library. Binaries produced in this way are more portable since they don't contain a hardcoded path to the CPython library. This is critical when distributing binary wheels on PyPI, etc..When targeting macOS>=12, XCode recently started to generate a somewhat ominous warning:
-undefined dynamic_lookup may not work with chained fixups
For further detail on what chained fixups are, see the following informative WWDC https://developer.apple.com/videos/play/wwdc2022/110362/ (the relevant part starts about 20mins into the video)
The message that the dynamic resolution functionality became broken. I reported this to Apple via feedback request FB11767124.
Apple investigated the request and updated the behavior of
ld
starting with XCode 14.3b1+: it now disables the fixup chain linker optimization whenever-undefined dynamic_lookup
is specified. The feedback from Apple also stated that the parameter-Wl,-no_fixup_chains
should be specified on pre-14.3 XCode versions to ensure correct behavior.This commit realizes those changes by passing a flag to always disable fixup chains. Related discussion is available in the CPython repository (python/cpython#97524).