Skip to content

gh-125206: Bug in ctypes with old libffi is fixed #125322

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 5 commits into from
Oct 15, 2024
Merged

gh-125206: Bug in ctypes with old libffi is fixed #125322

merged 5 commits into from
Oct 15, 2024

Conversation

efimov-mikhail
Copy link
Contributor

@efimov-mikhail efimov-mikhail commented Oct 11, 2024 •
edited by bedevere-app bot
Loading

Workaround for old libffi versions is added.
Module ctypes now supports C11 double complex only with libffi >= 3.3.0.

@skirpichev, could you please review this?

@efimov-mikhail
pythongh-125206: Bug in ctypes with old libffi is fixed
911788f 
Workaround for old libffi versions is added.
Module ctypes now supports C11 double complex only with libffi >= 3.3.0.
@bedevere-app bedevere-app bot mentioned this pull request Oct 11, 2024
Closed
@skirpichev skirpichev self-requested a review October 11, 2024 15:09
skirpichev
skirpichev reviewed Oct 11, 2024
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.

Mostly LGTM.

One minor suggestion, lets rename Py_FFI_TARGET_HAS_COMPLEX_TYPE to something like Py_FFI_SUPPORT_C_COMPLEX.

Only non-trivial change here is in the configure.ac. I assume, you have tested this check on your system with old libffi.

Probably, this should be tested with build bots. Old workaround with FFI_TARGET_HAS_COMPLEX_TYPE was working e.g. on Sparc #120894 (comment)

@efimov-mikhail
Copy link
Contributor Author

One minor suggestion, lets rename Py_FFI_TARGET_HAS_COMPLEX_TYPE to something like Py_FFI_SUPPORT_C_COMPLEX.

Ok.

Only non-trivial change here is in the configure.ac.

Definitely.

I assume, you have tested this check on your system with old libffi.

Yes. It works fine with libffi.so.6:

root@efimov:/usr/lib/x86_64-linux-gnu# rm libffi.so && ln -s libffi.so.6 libffi.so
-> % (./configure --with-pydebug && make -j && ./python -m unittest -v test.test_ctypes.test_libc.LibTest.test_csqrt) 2>&1 | grep -E 'libffi|lffi'
checking for libffi... yes
checking libffi has complex type support... no
gcc -pthread -shared      Modules/_ctypes/_ctypes.o Modules/_ctypes/callbacks.o Modules/_ctypes/callproc.o Modules/_ctypes/stgdict.o Modules/_ctypes/cfield.o -lffi -ldl  -o Modules/_ctypes.cpython-314d-x86_64-linux-gnu.so
gcc -pthread -shared      Modules/_ctypes/_ctypes_test.o -lffi -ldl -lm  -o Modules/_ctypes_test.cpython-314d-x86_64-linux-gnu.so
test_csqrt (test.test_ctypes.test_libc.LibTest.test_csqrt) ... skipped 'requires C11 complex type and libffi >= 3.3.0'

And with libffi.so.7:

root@efimov:/usr/lib/x86_64-linux-gnu# rm libffi.so && ln -s libffi.so.7 libffi.so
-> % (./configure --with-pydebug && make -j && ./python -m unittest -v test.test_ctypes.test_libc.LibTest.test_csqrt) 2>&1 | grep -E 'libffi|lffi'
checking for libffi... yes
checking libffi has complex type support... yes
gcc -pthread -shared      Modules/_ctypes/_ctypes.o Modules/_ctypes/callbacks.o Modules/_ctypes/callproc.o Modules/_ctypes/stgdict.o Modules/_ctypes/cfield.o -lffi -ldl  -o Modules/_ctypes.cpython-314d-x86_64-linux-gnu.so
gcc -pthread -shared      Modules/_ctypes/_ctypes_test.o -lffi -ldl -lm  -o Modules/_ctypes_test.cpython-314d-x86_64-linux-gnu.so

Probably, this should be tested with build bots. Old workaround with FFI_TARGET_HAS_COMPLEX_TYPE was working e.g. on Sparc #120894 (comment)

I know nothing about build bot configuration.
It would be great if somebody helps me with it.

skirpichev
skirpichev approved these changes Oct 11, 2024
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.

LGTM

But someone else should trigger testing with buildbots. This and/or this.

@picnixz

This comment was marked as outdated.

Sign in to view
@bedevere-bot

This comment was marked as outdated.

Sign in to view
@picnixz

This comment was marked as resolved.

Sign in to view
@bedevere-bot

This comment was marked as resolved.

Sign in to view
@picnixz

This comment was marked as outdated.

Sign in to view
@bedevere-bot

This comment was marked as outdated.

Sign in to view
@picnixz
Copy link
Member

picnixz commented Oct 11, 2024

@skirpichev You can request build bots now that you're a member :)

@skirpichev
Copy link
Member

SPARCv9 fails, but I see no failures, related to affected test. On another hand, configure now prints: checking libffi has complex type support... yes. Maybe this bot now uses newer libffi?

Lets wait PPC64LE, I guess it's queried.

@efimov-mikhail
Copy link
Contributor Author

efimov-mikhail commented Oct 11, 2024 •
edited
Loading

SPARCv9 fails, but I see no failures, related to affected test. On another hand, configure now prints: checking libffi has complex type support... yes. Maybe this bot now uses newer libffi?

Maybe it is.

I can see such line in log from this issue:

gcc -fno-strict-overflow -I/usr/lib/sparcv9/libffi-3.2.1/include -fno-strict-overflow -fstack-protector-strong -Wtrampolines -Wsign-compare -g -Og -Wall -D_REENTRANT   -std=c11 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror=implicit-function-declaration -fvisibility=hidden  -I./Include/internal -I./Include/internal/mimalloc  -I. -I./Include    -fPIC -c ./Modules/_ctypes/_ctypes.c -o Modules/_ctypes/_ctypes.o

But in actual log it differs:

gcc -fno-strict-overflow -DFFI_NO_RAW_API -fno-strict-overflow -Wsign-compare -g -Og -Wall -D_REENTRANT   -std=c11 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror=implicit-function-declaration -fvisibility=hidden  -I./Include/internal -I./Include/internal/mimalloc  -I. -I./Include    -fPIC -c ./Modules/_ctypes/_ctypes.c -o Modules/_ctypes/_ctypes.o

Lets wait PPC64LE, I guess it's queried.

Yes, it is.

skirpichev
skirpichev reviewed Oct 12, 2024
View reviewed changes
@skirpichev
Copy link
Member

Ok, PPC64LE build was successful (and it doesn't detect a working libffi, as expected). I can't suggest some other built bot to test. LGTM.

PS: It's silly that PKG_CHECK_MODULES doesn't report a version found. (Of course, we could echoing somewhere pkg-config --modversion libffi, but...)

@efimov-mikhail @skirpichev
Update configure.ac
bcdf7b0 
PEP 7

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
skirpichev
skirpichev reviewed Oct 12, 2024
View reviewed changes
@skirpichev
Copy link
Member

CC @vstinner

@thesamesam
Copy link
Contributor

Not to derail this at all, but may want to consider looking at #103438 (#103439) in a followup too?

@skirpichev
Copy link
Member

@thesamesam, that's a separate issue.

@thesamesam
Copy link
Contributor

thesamesam commented Oct 14, 2024 •
edited
Loading

Yes, I know. I'm mentioning it because there's other libffi cleanups which have gone awry because an approach couldn't be agreed upon for them. The same approach used here could work, but nevermind.

@skirpichev
Copy link
Member

The same approach used here could work

It was already suggested in the referenced issue thread.

@efimov-mikhail
Copy link
Contributor Author

Maybe, there is anything I could improve?

@skirpichev
Copy link
Member

CC @erlend-aasland

vstinner
vstinner approved these changes Oct 15, 2024
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

@vstinner vstinner enabled auto-merge (squash) October 15, 2024 16:16
@vstinner vstinner merged commit aac89b5 into python:main Oct 15, 2024
39 checks passed
@efimov-mikhail efimov-mikhail deleted the ctypes_complex_double_libffi_issue branch October 15, 2024 16:23
@vstinner
Copy link
Member

Merged, thanks for the fix @efimov-mikhail.

@skirpichev
Copy link
Member

Correction: #126104

@skirpichev skirpichev mentioned this pull request Oct 29, 2024
Merged
@skirpichev
Copy link
Member

And yet another: #132865. Sorry :(

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

@vstinner vstinner vstinner approved these changes

@skirpichev skirpichev skirpichev approved these changes

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

@corona10 corona10 Awaiting requested review from corona10 corona10 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.
6 participants
@efimov-mikhail @picnixz @bedevere-bot @skirpichev @thesamesam @vstinner