Skip to content

gh-128156: Guard use of ffi_type_complex_double on macOS system libffi #128680

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jan 21, 2025
Merged

gh-128156: Guard use of ffi_type_complex_double on macOS system libffi #128680

merged 8 commits into from
Jan 21, 2025

Conversation

encukou
Copy link
Member

@encukou encukou commented Jan 9, 2025 •
edited by github-actions bot
Loading

formattable.fmt{C,E,F} now have a runtime guard (if USING_APPLE_OS_LIBFFI).
The list of type codes (previously SIMPLE_TYPE_CHARS) is now generated at runtime.

📚 Documentation preview 📚: https://cpython-previews--128680.org.readthedocs.build/

@bedevere-app bedevere-app bot mentioned this pull request Jan 9, 2025
Closed
skirpichev
skirpichev reviewed Jan 10, 2025
View reviewed changes
Copy link
Member

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but we need some feedback from MacOS experts.

@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 13, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit e8b0992 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 13, 2025
@ned-deily
Copy link
Member

cc: @ronaldoussoren @python/macos-team

ned-deily
ned-deily approved these changes Jan 21, 2025
View reviewed changes
Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I verified that with the PR we could now build and install cpython on the current macOS 15.2 using MACOSX_DEPLOYMENT_TARGET=10.14 and then also install and execute the same binaries on a macOS 10.14.x system. As expected, on 15.2 the three complex types were available and on 10.14, they were not. And test_ctypes ran without errors on both systems. Nice work, @encukou !

@encukou
Copy link
Member Author

encukou commented Jan 21, 2025

Thank you for the thorough review!

@encukou encukou merged commit d3b1bb2 into python:main Jan 21, 2025
45 checks passed
@encukou encukou deleted the guard-complex_double branch January 21, 2025 09:59
@picnixz
Copy link
Member

picnixz commented Jan 21, 2025 •
edited
Loading

Not sure if it's my system or not but I'm unable to compile :(

In file included from ./Modules/_ctypes/callproc.c:87:0:
./Modules/_ctypes/ctypes.h:12:72: error: missing binary operator before token "("
 #   if USING_APPLE_OS_LIBFFI && defined(__has_builtin) && __has_builtin(__builtin_available)

Other uses of __has_builtin are guarded by #if defined(__APPLE__):

#if defined(__APPLE__) && defined(__has_builtin)

I'm using OpenSUSE and gcc 7.5

@picnixz
Copy link
Member

picnixz commented Jan 21, 2025 •
edited
Loading

I think we should do the following:

diff --git a/Modules/_ctypes/ctypes.h b/Modules/_ctypes/ctypes.h
index 1330754a7e0..dc079f2441b 100644
--- a/Modules/_ctypes/ctypes.h
+++ b/Modules/_ctypes/ctypes.h
@@ -9,8 +9,12 @@
 // For Apple's libffi, this must be determined at runtime (see gh-128156).
 #if defined(Py_HAVE_C_COMPLEX) && defined(Py_FFI_SUPPORT_C_COMPLEX)
 #   include "../_complex.h"       // complex
-#   if USING_APPLE_OS_LIBFFI && defined(__has_builtin) && __has_builtin(__builtin_available)
-#       define Py_FFI_COMPLEX_AVAILABLE __builtin_available(macOS 10.15, *)
+#   ifdef __has_builtin
+#       if USING_APPLE_OS_LIBFFI && __has_builtin(__builtin_available)
+#           define Py_FFI_COMPLEX_AVAILABLE __builtin_available(macOS 10.15, *)
+#       else
+#           define Py_FFI_COMPLEX_AVAILABLE 1
+#       endif
 #   else
 #       define Py_FFI_COMPLEX_AVAILABLE 1
 #   endif

This is also what's recommended by https://gcc.gnu.org/onlinedocs/cpp/_005f_005fhas_005fbuiltin.html.

cc @encukou @zanieb @ned-deily

@encukou
Copy link
Member Author

encukou commented Jan 21, 2025 via email

@picnixz
Copy link
Member

picnixz commented Jan 21, 2025

Yes, but somehow this doesn't work so I don't know :( (I'll check tomorrow again but the current code doesn't compile on my side)

@encukou
Copy link
Member Author

encukou commented Jan 21, 2025

I'd be interested to know what compiler, OS and configure settings you're using.

@picnixz
Copy link
Member

picnixz commented Jan 21, 2025

I'm using gcc 7.5.0, my OS is openSUSE 15.5 and I used ./configure --with-pydebug. I can recheck tomorrow (I temporarily fixed this by defining __has_builtin).

@picnixz
Copy link
Member

picnixz commented Jan 21, 2025

See #129141 for the relevant issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skirpichev skirpichev skirpichev approved these changes

@ned-deily ned-deily ned-deily approved these changes

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@encukou @bedevere-bot @ned-deily @picnixz @skirpichev