Skip to content
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

Use of ffi_type_complex_double is unguarded #128156

Closed
zanieb opened this issue Dec 21, 2024 · 22 comments
Closed

Use of ffi_type_complex_double is unguarded #128156

zanieb opened this issue Dec 21, 2024 · 22 comments
Assignees
Labels
build The build process and cross-build extension-modules C modules in the Modules dir OS-mac type-bug An unexpected behavior, bug, or error

Comments

@zanieb
Copy link
Contributor

zanieb commented Dec 21, 2024

Bug report

Bug description:

e.g.

   -std=c11 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror=implicit-function-declaration -fvisibility=hidden -Werror=unguarded-availability-new  -I./Include/internal -I./Include/internal/mimalloc  -I. -I./Include     -c ./Modules/_ctypes/cfield.c -o Modules/_ctypes/cfield.o
./Modules/_ctypes/cfield.c:1610:36: error: 'ffi_type_complex_double' is only available on macOS 10.15 or newer [-Werror,-Wunguarded-availability-new]
        case 'C': fd->pffi_type = &ffi_type_complex_double; break;
                                   ^~~~~~~~~~~~~~~~~~~~~~~
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi/ffi.h:236:46: note: 'ffi_type_complex_double' has been marked as being introduced in macOS 10.15 here, but the deployment target is macOS 10.9.0
FFI_AVAILABLE_APPLE_2019 FFI_EXTERN ffi_type ffi_type_complex_double;
                                             ^
./Modules/_ctypes/cfield.c:1610:36: note: enclose 'ffi_type_complex_double' in a __builtin_available check to silence this warning
        case 'C': fd->pffi_type = &ffi_type_complex_double; break;
                                   ^~~~~~~~~~~~~~~~~~~~~~~
./Modules/_ctypes/cfield.c:1611:36: error: 'ffi_type_complex_float' is only available on macOS 10.15 or newer [-Werror,-Wunguarded-availability-new]
        case 'E': fd->pffi_type = &ffi_type_complex_float; break;
                                   ^~~~~~~~~~~~~~~~~~~~~~
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi/ffi.h:235:46: note: 'ffi_type_complex_float' has been marked as being introduced in macOS 10.15 here, but the deployment target is macOS 10.9.0
FFI_AVAILABLE_APPLE_2019 FFI_EXTERN ffi_type ffi_type_complex_float;
                                             ^
./Modules/_ctypes/cfield.c:1611:36: note: enclose 'ffi_type_complex_float' in a __builtin_available check to silence this warning
        case 'E': fd->pffi_type = &ffi_type_complex_float; break;
                                   ^~~~~~~~~~~~~~~~~~~~~~
./Modules/_ctypes/cfield.c:1612:36: error: 'ffi_type_complex_double' is only available on macOS 10.15 or newer [-Werror,-Wung
uarded-availability-new]
        case 'F': fd->pffi_type = &ffi_type_complex_longdouble; break;
                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi/ffi.h:240:37: note: expanded from macro 'ffi_type_complex_longdouble'
#define ffi_type_complex_longdouble ffi_type_complex_double
                                    ^~~~~~~~~~~~~~~~~~~~~~~
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi/ffi.h:236:46: note: 'ffi_type_complex_double' has been marked as being introduced in macOS 10.15 here, but the deployment target is macOS 10.9.0
FFI_AVAILABLE_APPLE_2019 FFI_EXTERN ffi_type ffi_type_complex_double;
                                             ^
./Modules/_ctypes/cfield.c:1612:36: note: enclose 'ffi_type_complex_double' in a __builtin_available check to silence this warning
        case 'F': fd->pffi_type = &ffi_type_complex_longdouble; break;
                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi/ffi.h:240:37: note: expanded from macro 'ffi_type_complex_longdouble'
#define ffi_type_complex_longdouble ffi_type_complex_double
                                    ^~~~~~~~~~~~~~~~~~~~~~~

See #100384 for context and similar issues.

CPython versions tested on:

CPython main branch

Operating systems tested on:

macOS

Linked PRs

@zanieb zanieb added the type-bug An unexpected behavior, bug, or error label Dec 21, 2024
@zanieb
Copy link
Contributor Author

zanieb commented Dec 21, 2024

I started taking a look at a patch for this, but it doesn't look trivial.

The existing checks use defined(Py_HAVE_C_COMPLEX) && defined(Py_FFI_SUPPORT_C_COMPLEX) and I think it needs to check defined(USING_APPLE_OS_LIBFFI) && defined(HAVE_FFI_TYPE_COMPLEX_DOUBLE_RUNTIME) where the latter is defined with

#ifdef HAVE_BUILTIN_AVAILABLE
#  define HAVE_FFI_TYPE_COMPLEX_DOUBLE_RUNTIME __builtin_available(macOS 10.15, *)
#endif

The ergonomic way to do this is beyond my knowledge, but I'm very happy to submit a patch if someone wants to provide some guidance.

@picnixz picnixz added extension-modules C modules in the Modules dir build The build process and cross-build labels Dec 21, 2024
@picnixz
Copy link
Member

picnixz commented Dec 21, 2024

cc @skirpichev

@zanieb
Copy link
Contributor Author

zanieb commented Dec 21, 2024

@picnixz you can also apply the OS-mac label, there are no runtime symbol checks for Linux yet.

Thank you for your quick triage :)

@carljm carljm added the OS-mac label Dec 21, 2024
@skirpichev
Copy link
Member

Looks as a false positive to me. I don't understand why we need MacOS-specific workaround when nothing is broken.

@skirpichev skirpichev added the pending The issue will be closed if no feedback is provided label Dec 22, 2024
@zanieb
Copy link
Contributor Author

zanieb commented Dec 22, 2024

@skirpichev won't CPython segfault when this function is called on macOS older than 10.15?

The typical pattern on macOS is to guard usage, e.g., in these similar situations:

This is also blocking adding a guard against future regressions, see #128155 (comment).

cc @ned-deily

@ned-deily
Copy link
Member

won't CPython segfault when this function is called on macOS older than 10.15?

Context: this is for cases where CPython is built to run on a range of macOS releases, like we do for the python.org macOS installer builds. This involves building on a newer (current) version of macOS (like 15) but with MACOSX_DEPLOYMENT_TARGET set to an earlier version, say, 10.13.

@skirpichev
Copy link
Member

skirpichev commented Dec 22, 2024

won't CPython segfault when this function is called on macOS older than 10.15?

How? If libffi has no proper support for complex types, then configure script will not define Py_FFI_SUPPORT_C_COMPLEX.

Edit:

This involves building on a newer (current) version of macOS (like 15) but with MACOSX_DEPLOYMENT_TARGET set to an earlier version, say, 10.13.

I see. So, for such builds you want to revert compile-time decision on Py_FFI_SUPPORT_C_COMPLEX at runtime, right?

That will require substantial rewrite of complex numbers support in ctypes.

@skirpichev skirpichev removed the pending The issue will be closed if no feedback is provided label Dec 22, 2024
@skirpichev
Copy link
Member

I would suggest rather unset Py_FFI_SUPPORT_C_COMPLEX for such builds at compile time.

@zanieb
Copy link
Contributor Author

zanieb commented Dec 22, 2024

So, for such builds you want to revert compile-time decision on Py_FFI_SUPPORT_C_COMPLEX at runtime, right?

Basically, yes.

I would suggest rather unset Py_FFI_SUPPORT_C_COMPLEX for such builds at compile time.

That's a possibility, but weak symbols allow for portable builds which retain the functionality when available. This is common on macOS because there's robust support for it, but there's also interest in using weak symbols for Linux targets.

That will require substantial rewrite of complex numbers support in ctypes.

This is unfortunate. I'm not sure how to weigh the trade-offs here — but I'm curious if the idea that it's not macOS-specific changes your opinion?

Anyway, I don't think it's a great outcome to need to rewrite this module. I think an "easy" path forward here is to bump the minimum supported macOS version to 10.15 — it's not a big bump and @ned-deily had previously suggested this. There's some data on usage here.

We may need to revisit this module when adding weak symbols for Linux, but I have no clue how modern of library versions the functionality here depends on yet.

@skirpichev
Copy link
Member

I'm curious if the idea that it's not macOS-specific

So far it doesn't look relevant for Linux builds. Unfortunately, here we can't rely just on existence of libffi type at runtime. See #125206.

an "easy" path forward here is to bump the minimum supported macOS version to 10.15

If this is an option - looks great.

@zanieb
Copy link
Contributor Author

zanieb commented Dec 22, 2024

We've set the minimum in CI to 10.15 but formalizing that in a policy will be a separate task (see #128155 (comment)).

I can't tell if it makes sense to leave this issue open since ffi_type_complex_double usage is unguarded (and all of the other macOS 10.15 built-in usage is guarded). It sounds quite challenging to do anything about it though and we won't be enforcing guards for <=10.15 in the foreseeable future.

@encukou
Copy link
Member

encukou commented Jan 2, 2025

On the ctypes side, I think I can add run-time checks to all uses of Py_FFI_SUPPORT_C_COMPLEX, and everything that depends on that. (There's a few changes in this area that I wanted to make anyway.)
But, I don't know much about macOS and I can only test on one macOS version.

I'd appreciate a patch that defines, on any platform with Py_FFI_SUPPORT_C_COMPLEX, a function/macro that returns true iff ffi_type_complex_* can be used. With that I can write a PR; after that I'd ask you to test it.

To be clear: float complex or csqrtf() are fine; just ffi_type_complex_*, right?


Note that I changed the cfield.c code recently. If you have a draft in progress, you'll probably get conflicts with current main. If you need them resolved, push to a public branch and I can do the merge.

@skirpichev
Copy link
Member

To be clear: float complex or csqrtf() are fine; just ffi_type_complex_*, right?

Not sure what you meant. We have two defines: Py_HAVE_C_COMPLEX - if Annex G complex types are present and correctly works, and Py_FFI_SUPPORT_C_COMPLEX - if libffi really support complex types (it might just define FFI_TARGET_HAS_COMPLEX_TYPE, I'm not sure if this issue is valid also for MacOS libffi).

For the struct module, Py_HAVE_C_COMPLEX - is enough. But in the ctypes - adding complex types is useless without Py_FFI_SUPPORT_C_COMPLEX. In principle, we could add c_*_complex in pure-Python (if platform support Annex G complex types):

class c_double_complex(ctypes.Structure):
    _fields_ = [("real", ctypes.c_double),
                ("imag", ctypes.c_double)]
    @property
    def value(self):
        return complex(self.real, self.imag)

And than call e.g. libm's functions like csqrt(). That was working for me before. But I'm not sure if that will work for all platforms.

@encukou
Copy link
Member

encukou commented Jan 3, 2025

Thanks for the clarification!

So, ctypes.c_*_complex will be missing if not available in libffi at runtime.

(Later, we could add them, so that you can pass pointers to them as arguments or put them in Structures, but they wouldn't be usable as function arguments. But that's out of scope here.)

@skirpichev
Copy link
Member

So, ctypes.c_*_complex will be missing if not available in libffi at runtime.

Yes, if not properly supported by libffi at runtime. We can add such types, if Annex G C types are available, but using them as function arguments will produce garbage (see #125206).

@encukou
Copy link
Member

encukou commented Jan 3, 2025

We can add such types, if Annex G C types are available, but using them as function arguments will produce garbage

Not that different from something like c_int.__ctype_be__, then...

Anyway, back to this issue: I'd appreciate if a macOS expert could write the guard code -- it feels like a few lines that would take a lot of research for me.

@zanieb
Copy link
Contributor Author

zanieb commented Jan 3, 2025

@encukou for what it's worth, the macOS-specific parts are pretty transferable from other cases where we check for runtime availability, e.g., #128156 (comment) and #128156 (comment).

@encukou encukou self-assigned this Jan 9, 2025
encukou added a commit that referenced this issue Jan 21, 2025
…ffi (GH-128680)

* Determine ffi complex support at runtime
* Also, generate SIMPLE_TYPE_CHARS once at runtime
@encukou
Copy link
Member

encukou commented Jan 21, 2025

Fixed in 3.14.
How important is it to backport this? Due to recent ctypes changes a backport wouldn't be straightforward, but I can do it if needed.

@skirpichev
Copy link
Member

AFAIR, we don't have complex types in 3.13. (BTW, maybe it's worth a whatsnew entry.) So, I think this might be closed.

@encukou
Copy link
Member

encukou commented Jan 21, 2025

Oh! Well, that's one assumption I didn't check :)

@encukou encukou closed this as completed Jan 21, 2025
@encukou
Copy link
Member

encukou commented Jan 21, 2025

PR for the whatsnew entry: #129129

@zanieb
Copy link
Contributor Author

zanieb commented Jan 21, 2025

Thanks @encukou !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build The build process and cross-build extension-modules C modules in the Modules dir OS-mac type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

6 participants