Skip to content

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

Merged
encukou merged 8 commits intopython:mainfrom
encukou:guard-complex_double
Jan 21, 2025
Merged

gh-128156: Guard use of ffi_type_complex_double on macOS system libffi#128680
encukou merged 8 commits intopython:mainfrom
encukou:guard-complex_double

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

bedevere-bot commented Jan 13, 2025

🤖 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

ned-deily commented Jan 14, 2025

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
41 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