bpo-41100: ctypes fixes for arm64 Mac OS #21249

lawrence-danna-apple · 2020-07-01T01:08:58Z

Posted in response to this comment by Ronald #21242 (comment)

This PR would supersede these two, combining because the third commit depends on both

This has three commits:

ctypes: use the correct ABI for variadic functions
use system libffi for Mac OS 10.15 and up
setup.py: probe libffi for ffi_closure_alloc and ffi_prep_cif_var

The first two are from those previous PRs. The third makes setup.py probe for those functions by searching libffi's headers.

https://bugs.python.org/issue41100

This updates setup.py to default to the system libffi on Mac OS 10.15 and forward. It also updates detect_ctypes to prefer finding libffi in the Xcode SDK, rather than /usr/include

On arm64 the calling convention for variardic functions is different than the convention for fixed-arg functions of the same arg types. ctypes needs to use ffi_prep_cif_var to tell libffi which calling convention to use.

This set of patches includes the following upstream pull requests: - python/cpython#21114 "Support `arm64` in Mac/Tools/pythonw" - python/cpython#21224 "allow python to build for macosx-11.0-arm64" - python/cpython#21249 "ctypes fixes for arm64 Mac OS" Adding the patches before upstream has released them is warranted here because `python@3.8` is widely used as a dependency, and the patch is needed to enable testing dependent formulae on arm64. CC: Sean Molenaar <1484494+SMillerDev@users.noreply.github.com>

maxbelanger · 2020-07-04T23:05:47Z

@lawrence-danna-apple We currently redistribute libffi along with Python to support Mac OS X 10.10 as our deployment target. Your change implies that there were changes made to libffi (and the copy that ships with macOS 11) for arm64 macOS. Are these upstreamed and available?

lawrence-danna-apple · 2020-07-04T23:54:58Z

@lawrence-danna-apple We currently redistribute libffi along with Python to support Mac OS X 10.10 as our deployment target. Your change implies that there were changes made to libffi (and the copy that ships with macOS 11) for arm64 macOS. Are these upstreamed and available?

We also distribute libffi as part of Mac OS. Our changes for 10.15, which cover the necessary API changes are available here: https://opensource.apple.com/source/libffi/libffi-25/

I'm not sure what the status is as to upstreaming, but I'll do a review of it and see if anything necessary is still missing from upstream.

Noe that in the changes I'm proposing here, the Mac OS system libffi is only used if the deployment target is set to 10.15 or better.

maxbelanger · 2020-07-05T00:05:13Z

@lawrence-danna-apple We currently redistribute libffi along with Python to support Mac OS X 10.10 as our deployment target. Your change implies that there were changes made to libffi (and the copy that ships with macOS 11) for arm64 macOS. Are these upstreamed and available?

We also distribute libffi as part of Mac OS. Our changes for 10.15, which cover the necessary API changes are available here: https://opensource.apple.com/source/libffi/libffi-25/

I'm not sure what the status is as to upstreaming, but I'll do a review of it and see if anything necessary is still missing from upstream.

Noe that in the changes I'm proposing here, the Mac OS system libffi is only used if the deployment target is set to 10.15 or better.

Got it, thanks for clarifying. It looks like there may be some issues in libffi w/ XC12 + arm64: libffi/libffi#571. Good to know that we could rely on libffi-25, though.

Doc/library/ctypes.rst

Co-authored-by: Arch Oversight <archoversight@gmail.com>

ronaldoussoren

@lawrence-danna-apple We currently redistribute libffi along with Python to support Mac OS X 10.10 as our deployment target. Your change implies that there were changes made to libffi (and the copy that ships with macOS 11) for arm64 macOS. Are these upstreamed and available?

Isn't it possible/save to use the system libffi on 10.9 or later? The 10.9 SDK includes libffi, even if it uses an older API (just like libffi_osx in the CPython tree).

I'd prefer to drop the copy of libffi included with CPython and always use the system libffi, without including a copy of libffi in the installers for macOS.

ronaldoussoren · 2020-07-14T19:44:47Z

I'm a bit conflicted about the "variadic" attribute for _FuncPtr. I agree that it is necessary, but this is an API change and as such is a lot harder to back port.

lawrence-danna-apple · 2020-07-14T19:47:46Z

I'd prefer to drop the copy of libffi included with CPython and always use the system libffi

I'm not sure if this is possible. The libffi prior to 10.15 won't have the APIs we need to support arm64. However, we may be able to use ifdefs and availability to sort it out. I'll try.

Do you know why libffi was bundled into cpython in the first place? Do we have reason to believe the libffi on older releases of mac OS was broken in some way?

This set of patches includes the following upstream pull requests, in this order: - PR 20171, "Fix _tkinter use" python/cpython#20171 (prerequisite for patch #21249 to apply) - PR 21114, "Support `arm64` in Mac/Tools/pythonw" python/cpython#21114 - PR 21224, "allow python to build for macosx-11.0-arm64" python/cpython#21224 - PR 21249, "ctypes fixes for arm64 Mac OS" python/cpython#21249 The patches for 20171 and 21249 have been minimally modified in order to backport them to 3.8.3. Note that these have been successfully tested for `python@3.8` but not for `python@3.7`. The patch directive should be surrounded by an `if Hardware::CPU.arm?` block.

This replaces the three unmerged PR patches with a hosted formula patch. This includes the following upstream pull requests: - python/cpython#20171, "Fix _tkinter use" (prerequisite for 21249) - python/cpython#21114, "Support arm64 in Mac/Tools/pythonw" - python/cpython#21224, "allow python to build for macosx-11.0-arm64" - python/cpython#21249, "ctypes fixes for arm64 Mac OS" See also: - Homebrew/formula-patches#292

lawrence-danna-apple · 2020-07-15T23:30:07Z

@ronaldoussoren

I'm a bit conflicted about the "variadic" attribute for _FuncPtr. I agree that it is necessary

Well, it looks like I misread the documentation. It turns out that in the case where a variadic function is called with zero variadic arguments, the two calling conventions coincide. So the flag is not necessary after all.

lawrence-danna-apple · 2020-07-15T23:44:06Z

@ronaldoussoren

I found it is possible to use the system libffi even if the deployment target is set to before 10.15. I've added runtime checks for ffi_prep_cif_var, ffi_prep_closure_loc and ffi_closure_alloc. If they are present, it will use them. If not, it will fall back on the old way of doing things.

I've also enabled the system ffi if either the deployment target is 10.15 OR the arm64 is in the target arches. I'm not sure if Mac OS on PPC is still supported by python, but these changes shouldn't break it if it is.

The only thing that will not work is to build a super-fat binary that works on ppc, x86 and arm64 at the same time.

Should I add a commit deleting libffi_osx altogether, or leave it in for ppc support?

This replaces the three unmerged PR patches with a hosted formula patch. This includes the following upstream pull requests: - python/cpython#20171, "Fix _tkinter use" (prerequisite for 21249) - python/cpython#21114, "Support arm64 in Mac/Tools/pythonw" - python/cpython#21224, "allow python to build for macosx-11.0-arm64" - python/cpython#21249, "ctypes fixes for arm64 Mac OS" See also: - Homebrew/formula-patches#292 Closes #57997. Signed-off-by: Claudia Pellegrino <1239874+claui@users.noreply.github.com>

ned-deily

As a smoke test, I tried building with --with-system-ffi on macOS 10.9 and ran into the issue that __builtin_available is apparently not available until Xcode 9.0 on macOS 10.12 and it is a requirement that we can continue to be built on older systems with their supported Xcode versions. I found a workaround from the curl project that might be usable here if we need to support using the system libffi which would be desirable.

Also we should avoid the potential pragma warnings when building on other platforms.

ned-deily · 2020-07-20T08:34:32Z

Modules/_ctypes/callbacks.c

-    result = ffi_prep_closure(p->pcl_write, &p->cif, closure_fcn, p);
+#if HAVE_FFI_PREP_CLOSURE_LOC
+#   if USING_APPLE_OS_LIBFFI
+#      define HAVE_FFI_PREP_CLOSURE_LOC_RUNTIME __builtin_available(macos 11, ios 13, watchos 6, tvos 13, *)


__builtin_available is itself not available with older versions of macOS and Xcode, in particular with macOS 10.9 and Xcode 6, which we currently need to use to build our most common installer variants. And there are other projects like MacPorts that builds Python for older macOS systems. The general review comment has a link for a possible workaround from the curl project.

This use of __builtin_available should be safe, it is within a guard of HAVE_FFI_PREP_CLOSURE_LOC and USING_APPLE_OS_LIBFFI: The system version of libffi does not have ffi_prep_closure_loc in the 10.9 SDK (it is introduced in 10.15).

ned-deily · 2020-07-20T08:34:52Z

Modules/_ctypes/callproc.c

-        return -1;
+
+#   if USING_APPLE_OS_LIBFFI
+#      define HAVE_FFI_PREP_CIF_VAR_RUNTIME __builtin_available(macos 11, ios 13, watchos 6, tvos 13, *)


__builtin_available again

ned-deily · 2020-07-20T08:35:23Z

Modules/_ctypes/malloc_closure.c

 {
+#if USING_APPLE_OS_LIBFFI
+    if (__builtin_available(macos 11, ios 13, watchos 6, tvos 13, *)) {


__builtin_available again

I'm afraid this needs a configure test, ffi_closure_free is not available at all in older SDKs.

ned-deily · 2020-07-20T08:35:56Z

Modules/_ctypes/malloc_closure.c

 {
+#if USING_APPLE_OS_LIBFFI
+    if (__builtin_available(macos 11, ios 13, watchos 6, tvos 13, *)) {


__builtin_available again

ned-deily · 2020-07-20T08:37:43Z

Modules/_ctypes/callbacks.c

-                                  p,
-                                  p->pcl_exec);
+        #pragma clang diagnostic push
+        #pragma clang diagnostic ignored "-Wdeprecated-declarations"


ned-deily · 2020-07-20T08:38:05Z

Modules/_ctypes/callbacks.c

+        #pragma clang diagnostic push
+        #pragma clang diagnostic ignored "-Wdeprecated-declarations"
+        result = ffi_prep_closure(p->pcl_write, &p->cif, closure_fcn, p);
+        #pragma clang diagnostic pop


ned-deily · 2020-07-20T08:38:26Z

Modules/_ctypes/callbacks.c

-    result = ffi_prep_closure_loc(p->pcl_write, &p->cif, closure_fcn,
-                                  p,
-                                  p->pcl_exec);
+        #pragma clang diagnostic push


We should avoid -Wunknown-pragmas warnings when building on other platforms.

bedevere-bot · 2020-07-20T08:52:29Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

ronaldoussoren

I've added some comments, and am finally working on this PR on the DTK.

ronaldoussoren · 2020-07-20T11:30:15Z

Modules/_ctypes/callbacks.c

-    result = ffi_prep_closure(p->pcl_write, &p->cif, closure_fcn, p);
+#if HAVE_FFI_PREP_CLOSURE_LOC
+#   if USING_APPLE_OS_LIBFFI
+#      define HAVE_FFI_PREP_CLOSURE_LOC_RUNTIME __builtin_available(macos 11, ios 13, watchos 6, tvos 13, *)


This use of __builtin_available should be safe, it is within a guard of HAVE_FFI_PREP_CLOSURE_LOC and USING_APPLE_OS_LIBFFI: The system version of libffi does not have ffi_prep_closure_loc in the 10.9 SDK (it is introduced in 10.15).

ronaldoussoren · 2020-07-20T11:30:46Z

Doc/library/ctypes.rst

@@ -2545,4 +2545,3 @@ Arrays and pointers

        Returns the object to which to pointer points.  Assigning to this
        attribute changes the pointer to point to the assigned object.
-


Unnecessary removal of a blank line (not important)

ronaldoussoren · 2020-07-20T11:33:15Z

Modules/_ctypes/callbacks.c

-    result = ffi_prep_closure(p->pcl_write, &p->cif, closure_fcn, p);
+#if HAVE_FFI_PREP_CLOSURE_LOC
+#   if USING_APPLE_OS_LIBFFI
+#      define HAVE_FFI_PREP_CLOSURE_LOC_RUNTIME __builtin_available(macos 11, ios 13, watchos 6, tvos 13, *)


Why test for macOS 11 and not 10.15? According to the header files this API is introduced in 10.15.

you're right it should be 10.15

ronaldoussoren · 2020-07-20T11:35:25Z

Modules/_ctypes/callproc.c

-        return -1;
+
+#   if USING_APPLE_OS_LIBFFI
+#      define HAVE_FFI_PREP_CIF_VAR_RUNTIME __builtin_available(macos 11, ios 13, watchos 6, tvos 13, *)


Likewise: the headers claim this _var variant is present in macOS 10.15.

ronaldoussoren · 2020-07-20T11:36:11Z

Modules/_ctypes/callproc.c

-                        "ffi_prep_cif failed");
-        return -1;
+
+#   if USING_APPLE_OS_LIBFFI


Proposed change:

#if USING_APPLE_OS_LIBFFI && HAVE_FFI_PREP_CIF_VAR

This way the __builtin_available is not used when building on older macOS versions.

ronaldoussoren · 2020-07-20T11:38:35Z

Modules/_ctypes/malloc_closure.c

 {
+#if USING_APPLE_OS_LIBFFI
+    if (__builtin_available(macos 11, ios 13, watchos 6, tvos 13, *)) {


I'm afraid this needs a configure test, ffi_closure_free is not available at all in older SDKs.

ronaldoussoren · 2020-07-20T11:38:47Z

Modules/_ctypes/malloc_closure.c

 {
+#if USING_APPLE_OS_LIBFFI
+    if (__builtin_available(macos 11, ios 13, watchos 6, tvos 13, *)) {


ronaldoussoren · 2020-07-20T11:39:51Z

setup.py

+            self.use_system_libffi = True
+        else:
+            self.use_system_libffi = '--with-system-ffi' in sysconfig.get_config_var("CONFIG_ARGS")
+


I'd prefer to just drop "use_system_libffi" and unconditionally use the system headers.

This is support for ctypes on macOS/arm64 based on PR 21249 by Lawrence D'Anna (Apple). Changes: - changed __builtin_available tests from 11.0 to 10.15 - added test to setup.py for ffi_closure_alloc and use that in malloc_closure.c - Minor change in the code path for ffi_prep_closure_var (coding style change)

lawrence-danna-apple · 2020-07-20T22:07:39Z

I have made the requested changes; please review again

bedevere-bot · 2020-07-20T22:07:42Z

Thanks for making the requested changes!

@ned-deily: please review the changes made to this pull request.

lawrence-danna-apple · 2020-07-20T23:03:18Z

@ned-deily

Your comments have alerted me to a new problem. If I understand you correctly, the way you're doing the official builds for python is to build on an old version of Mac OS, with an old version of Xcode, in order to assure compatibility with a broad range of Mac OS versions.

This will work for x86_64, but if you want a universal build that works on both 10.10 and 11.0-arm64, it will not work, becaue the old Xcode doesn't support arm64 for Mac OS

On the other hand, if you build with the new Xcode using MACOSX_DEPLOYMENT_TARGET=10.10, that also does not work on 10.10, because autoconf will discover several functions such as getentropy which are available in the SDK, but are not available at runtime on 10.10. This will cause -Wunguarded-availability warnings in the build and an abort at runtime if the symbol is used.

maxbelanger · 2020-07-20T23:27:26Z

@lawrence-danna-apple quite right. This has been an issue for us at Dropbox in recent years. Python's official approach for backwards compatibility is not based on setting the deployment target, which is problematic for use in client-side software. We've fixed this internally by patching pyconfig.h, but that's not a great fix (ideally we could do something like weak-link them).

See https://bugs.python.org/issue31359.

lawrence-danna-apple · 2020-07-21T02:30:30Z

@maxbelanger @ned-deily

I opened up a new PR to deal with the deployment target issue.

~~#21576~~
#21577

ned-deily · 2020-07-21T06:15:13Z

Thanks for the PR. We are aware of the issues with supporting multiple levels and architectures Being able to weaklink from a build on a newer version to an older version has been desirable but it hasn't been a high priority issue. Actively supporting multiple architectures again (something we haven't had to do since the retirement of PPC :) makes this a higher priority issue and @ronaldoussoren is now working on it. I've added him to the reviewers of the new PR (#21577).

jmroot · 2020-09-20T21:51:45Z

setup.py

+            ext.extra_compile_args.append("-DUSING_APPLE_OS_LIBFFI=1")
+            ffi_in_sdk = os.path.join(macosx_sdk_root(), "usr/include/ffi")


Please bear in mind that Python may be built against an external libffi on macOS that is not the OS-provided version. In MacPorts we build against our port of libffi 3.3 on all OS versions.

See previous comment by @ronaldoussoren who prefers to unconditionally use the system's libffi. I updated this PR accordingly. Is there any reason not to use the system libffi anymore? I tested going back to 10.10 i think for the other PR adding availability checks. What's the need to support alternative libffi these days?

Same reasons as any other library: the OS often ships an older version which naturally lacks the features and bug fixes of later versions.

ned-deily · 2020-10-26T06:18:26Z

Thanks for the PR. It has been included in and superseded by GH-22855.

lawrence-danna-apple added 2 commits June 30, 2020 17:53

use system libffi for Mac OS 10.15 and up

ff883bc

This updates setup.py to default to the system libffi on Mac OS 10.15 and forward. It also updates detect_ctypes to prefer finding libffi in the Xcode SDK, rather than /usr/include

ctypes: use the correct ABI for variadic functions

36b35bc

On arm64 the calling convention for variardic functions is different than the convention for fixed-arg functions of the same arg types. ctypes needs to use ffi_prep_cif_var to tell libffi which calling convention to use.

the-knights-who-say-ni added the CLA signed label Jul 1, 2020

bedevere-bot added the awaiting review label Jul 1, 2020

lawrence-danna-apple mentioned this pull request Jul 1, 2020

bpo-41100: ctypes: use the correct ABI for variadic functions #21242

Closed

ctypes: probe libffi for ffi_closure_alloc and ffi_prep_cif_var

bacf128

lawrence-danna-apple force-pushed the postback-ctypes-combined branch from 0da8223 to bacf128 Compare July 1, 2020 01:28

📜🤖 Added by blurb_it.

971ef00

claui mentioned this pull request Jul 1, 2020

macOS 11 Big Sur compatibility on Apple Silicon Homebrew/brew#7857

Closed

9 tasks

sumanthratna mentioned this pull request Jul 1, 2020

[WIP] bpo-41179: Fix ctypes.util.find_library on macOS Big Sur #21250

Closed

This was referenced Jul 2, 2020

python@3: add arm64 compatibility patches Homebrew/formula-patches#287

Closed

python@3.8: add arm64 compatibility patches Homebrew/homebrew-core#57425

Merged

archoversight reviewed Jul 13, 2020

View reviewed changes

Doc/library/ctypes.rst Outdated Show resolved Hide resolved

fix spelling in Doc/library/ctypes.rst

da30587

Co-authored-by: Arch Oversight <archoversight@gmail.com>

ronaldoussoren reviewed Jul 14, 2020

View reviewed changes

claui mentioned this pull request Jul 15, 2020

python@3: add arm64 compatibility patches Homebrew/formula-patches#292

Merged

lawrence-danna-apple mentioned this pull request Jul 15, 2020

bpo-41100: use system libffi for Mac OS 10.15 and up #21239

Closed

lawrence-danna-apple added 2 commits July 15, 2020 16:58

use system libffi for older releases of Mac OS

80f852b

remove FUNCFLAG_VARIADIC

aeac14b

lawrence-danna-apple force-pushed the postback-ctypes-combined branch from be472ad to aeac14b Compare July 15, 2020 23:58

fixup! use system libffi for older releases of Mac OS

62fe20a

fixup! use system libffi for older releases of Mac OS

e160a59

ned-deily requested changes Jul 20, 2020

View reviewed changes

bedevere-bot removed the awaiting review label Jul 20, 2020

bedevere-bot added the awaiting changes label Jul 20, 2020

ronaldoussoren reviewed Jul 20, 2020

View reviewed changes

Fix build failure on Mac OS 10.10 / Xcode 6, remove libffi_osx

bca75fd

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 20, 2020

bedevere-bot requested a review from ned-deily July 20, 2020 22:07

jmroot reviewed Sep 20, 2020

View reviewed changes

sumanthratna mentioned this pull request Sep 24, 2020

Unable to import on macOS Big Sur vispy/vispy#1885

Closed

ned-deily closed this Oct 26, 2020

minrk mentioned this pull request Dec 2, 2020

On macos arm64, objc.objc_msgSend seg-faults when called from ipython ipython/ipykernel#562

Open

ronaldoussoren mentioned this pull request Apr 10, 2022

Support macOS 11 and Apple Silicon Macs #85272

Closed

		@@ -2545,4 +2545,3 @@ Arrays and pointers

		Returns the object to which to pointer points. Assigning to this
		attribute changes the pointer to point to the assigned object.

		ext.extra_compile_args.append("-DUSING_APPLE_OS_LIBFFI=1")
		ffi_in_sdk = os.path.join(macosx_sdk_root(), "usr/include/ffi")

bpo-41100: ctypes fixes for arm64 Mac OS #21249

bpo-41100: ctypes fixes for arm64 Mac OS #21249

Conversation

lawrence-danna-apple commented Jul 1, 2020 • edited Loading

maxbelanger commented Jul 4, 2020

lawrence-danna-apple commented Jul 4, 2020

maxbelanger commented Jul 5, 2020

ronaldoussoren left a comment

Choose a reason for hiding this comment

ronaldoussoren commented Jul 14, 2020

lawrence-danna-apple commented Jul 14, 2020 • edited Loading

lawrence-danna-apple commented Jul 15, 2020

lawrence-danna-apple commented Jul 15, 2020 • edited Loading

ned-deily left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jul 20, 2020

ronaldoussoren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawrence-danna-apple commented Jul 20, 2020

bedevere-bot commented Jul 20, 2020

lawrence-danna-apple commented Jul 20, 2020

maxbelanger commented Jul 20, 2020

lawrence-danna-apple commented Jul 21, 2020 • edited Loading

ned-deily commented Jul 21, 2020

Choose a reason for hiding this comment

lawrence-danna-apple Sep 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ned-deily commented Oct 26, 2020

lawrence-danna-apple commented Jul 1, 2020 •

edited

Loading

lawrence-danna-apple commented Jul 14, 2020 •

edited

Loading

lawrence-danna-apple commented Jul 15, 2020 •

edited

Loading

ned-deily left a comment •

edited

Loading

lawrence-danna-apple commented Jul 21, 2020 •

edited

Loading

lawrence-danna-apple Sep 20, 2020 •

edited

Loading