GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-125206: Bug in ctypes with old libffi is fixed by efimov-mikhail · Pull Request #125322 · python/cpython

Conversation

@efimov-mikhail

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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?

  • Issue: CTypes test failed, complex double problem #125206

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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.
@efimov-mikhail
efimov-mikhail requested review from corona10 and erlend-aasland as code owners October 11, 2024 15:06
@bedevere-app bedevere-app Bot mentioned this pull request Oct 11, 2024
CTypes test failed, complex double problem #125206
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Oct 11, 2024
@skirpichev
skirpichev self-requested a review October 11, 2024 15:09
skirpichev
skirpichev reviewed Oct 11, 2024
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2024-10-11-18-03-05.gh-issue-125206.pWRRK6.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev
Update Misc/NEWS.d/next/Library/2024-10-11-18-03-05.gh-issue-125206.p…
b583c21 
…WRRK6.rst
skirpichev
skirpichev reviewed Oct 11, 2024
View reviewed changes

@skirpichev skirpichev left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@efimov-mikhail

efimov-mikhail commented Oct 11, 2024

Copy link
Copy Markdown
Member 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@efimov-mikhail
pythongh-125206: Py_FFI_TARGET_HAS_COMPLEX_TYPE -> Py_FFI_SUPPORT_C_C…
f0f54e2 
…OMPLEX
@efimov-mikhail
efimov-mikhail requested a review from skirpichev October 11, 2024 15:42
skirpichev
skirpichev approved these changes Oct 11, 2024
View reviewed changes

@skirpichev skirpichev left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
efimov-mikhail reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Oct 11, 2024
@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

picnixz commented Oct 11, 2024

Copy link
Copy Markdown
Member

@skirpichev You can request build bots now that you're a member :)
skirpichev and efimov-mikhail reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev

skirpichev commented Oct 11, 2024

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@efimov-mikhail

efimov-mikhail commented Oct 11, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
skirpichev
skirpichev reviewed Oct 12, 2024
View reviewed changes
Comment thread configure.ac Resolved

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev

skirpichev commented Oct 12, 2024

Copy link
Copy Markdown
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...)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@efimov-mikhail @skirpichev
Update configure.ac
bcdf7b0 
PEP 7

Co-authored-by: Sergey B Kirpichev <[email protected]>
skirpichev
skirpichev reviewed Oct 12, 2024
View reviewed changes
Comment thread configure Resolved

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev
Update configure
05f4b17
@skirpichev

skirpichev commented Oct 12, 2024

Copy link
Copy Markdown
Member

CC @vstinner
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thesamesam

thesamesam commented Oct 14, 2024

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev

skirpichev commented Oct 14, 2024

Copy link
Copy Markdown
Member

@thesamesam, that's a separate issue.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thesamesam

thesamesam commented Oct 14, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev

skirpichev commented Oct 14, 2024

Copy link
Copy Markdown
Member

The same approach used here could work

It was already suggested in the referenced issue thread.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@efimov-mikhail

efimov-mikhail commented Oct 15, 2024

Copy link
Copy Markdown
Member Author

Maybe, there is anything I could improve?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
skirpichev
skirpichev approved these changes Oct 15, 2024
View reviewed changes
@skirpichev

skirpichev commented Oct 15, 2024

Copy link
Copy Markdown
Member

CC @erlend-aasland
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner approved these changes Oct 15, 2024
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
efimov-mikhail reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Oct 15, 2024
@vstinner
vstinner enabled auto-merge (squash) October 15, 2024 16:16
@vstinner
vstinner merged commit aac89b5 into python:main Oct 15, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 15, 2024
@efimov-mikhail
efimov-mikhail deleted the ctypes_complex_double_libffi_issue branch October 15, 2024 16:23
@vstinner

vstinner commented Oct 15, 2024

Copy link
Copy Markdown
Member

Merged, thanks for the fix @efimov-mikhail.
efimov-mikhail reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev

skirpichev commented Oct 29, 2024

Copy link
Copy Markdown
Member

Correction: #126104
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev skirpichev mentioned this pull request Oct 29, 2024
gh-125206: Correct detection of complex numbers support in libffi #126104
Merged
@skirpichev

skirpichev commented Apr 24, 2025

Copy link
Copy Markdown
Member

And yet another: #132865. Sorry :(
efimov-mikhail reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@efimov-mikhail @picnixz @bedevere-bot @skirpichev @thesamesam @vstinner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.