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

qualify use of tp_pypy_flags and tp_vectorcall on PyPy #4509

Merged
merged 1 commit into from
Dec 21, 2021

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Dec 20, 2021

The release of Cython 0.29.26 exposed a problem with the way PyPy3.8 defined PyTypeObject: PyPy is missing the tp_vectorcall slot. See PyPy issue 3618. The reason this only came up after the release of Cython 0.29.26 is that release 0.29.26 backported the code from commit b0d8de7 to the 0.29 release branch. Before that commit, the last few fields of the PyTypeObject in Cython were

  • For Python3.8:
    • Cython<0.29.26 initializes a struct with the last fields tp_finalize, tp_vectorcall, tp_print
    • Cython>=0.29.26 initializes a struct with the last fields tp_finalize, tp_vectorcall, tp_print, tp_pypy_flags
  • For Python3.9:
    • Cython<0.29.26 initializes a struct with the last fields tp_finalize, tp_vectorcall
    • Cython>=0.29.26 initializes a struct with the last fields tp_finalize, tp_vectorcall, tp_pypy_flags

For PyPy3.8 (the current release is 7.3.7)

  • PyPy<7.3.8 declares a struct with the last fields tp_finalize, tp_print, tp_pypy_flags
  • PyPy>=7.3.8 will declares a struct with the last fields tp_finalize, tp_vectorcall, tp_print

There are only 3 PyPy 3.8 slots, and Cython 0.29.26 tries to fill 4 slots, causing a compiler error (on MSVC).

PyPy3.9 (not yet released) will declare a struct with the last fields tp_finalize, tp_vectorcall, tp_pypy_flags

The reason PyPy chose to still have 3 fields for PyPy3.8 is to not break ABI compatibility (under the assumptions that (1) no-one uses tp_print nor tp_vectorcall and (2) tp_pypy_flags is an internal private field and (3) for PyPy>=7.3.8, there is a work-around for removing tp_pypy_flags).

So this PR proposes to codify the situation:

  • like pre-0.29.26, do not emit tp_pypy_flags for PyPy<3.8. This will cause compilation to emit a compiler warning but not an error.
  • only emit a tp_vectorcall slot for upcoming PyPy releases (reflecting reality)

Sorry for the mess.

@mattip
Copy link
Contributor Author

mattip commented Dec 21, 2021

Lots of CI failures, but that seems to be across the last few PRs :(. This seems to be the last CI run of all green, the next merge to master has failures (by looking at "Actions" and rows that have "master" as the target). It seems 7fca8c8 is the cause?

@mattip
Copy link
Contributor Author

mattip commented Dec 21, 2021

Or maybe 7fca8c8 turned already-occurring crashes into failures?

@scoder
Copy link
Contributor

scoder commented Dec 21, 2021

It seems 7fca8c8 is the cause?

No, it's not that easy. I investigated this for some time yesterday and couldn't reproduce it locally. It seems more related to the recent pyximport changes in #4339, but the failures only started a couple of days after merging that, with the completely innocent looking change that you mentioned above. Can't say why. Might be a change in test dependencies or the parallel execution relation. Or cosmic radiation leading to predictable failures for once. I even fixed several surrounding issues along the way, but none of them changed the situation. It seems indicative that it only fails in Py3.7+, though.

@mattip
Copy link
Contributor Author

mattip commented Dec 21, 2021

Comparing the build logs from the ci (ubuntu-18.04, c, 3.7) build in the two runs, the older one builds Cython with

gcc -pthread -shared -Wl,... -lpython3.7m -o ...

and the new one without -pthread and without -lpython3.7m

gcc -shared -Wl, ... -o ...

This might be due to differences from using setuptools-59.7.0 wheel-0.37.0 vs. setuptools-60.0.0 wheel-0.37.0. I think v60 started using the internal setuptools.distutils instead of the stdlib one. It might be worth trying to pin setuptools to the older version? There are more differences, grepping on "Successfully installed" and massaging the complete diff I get

old new
setuptools-59.7.0 setuptools-60.0.0
numpy-1.21.4 numpy-1.21.5
importlib-metadata-4.9.0 importlib-metadata-4.10.0

@mattip mattip mentioned this pull request Dec 21, 2021
@scoder
Copy link
Contributor

scoder commented Dec 21, 2021

Triggering clean CI build.

@scoder scoder closed this Dec 21, 2021
@scoder scoder reopened this Dec 21, 2021
@scoder scoder added this to the 0.29.27 milestone Dec 21, 2021
@scoder scoder merged commit a68ef91 into cython:master Dec 21, 2021
scoder pushed a commit that referenced this pull request Dec 21, 2021
For PyPy3.8 (the current release is 7.3.7)

* PyPy<7.3.8 declares a struct with the last fields tp_finalize, tp_print, tp_pypy_flags
* PyPy>=7.3.8 will declare a struct with the last fields tp_finalize, tp_vectorcall, tp_print

PyPy3.9 (not yet released) will declare a struct with the last fields tp_finalize, tp_vectorcall, tp_pypy_flags

See https://foss.heptapod.net/pypy/pypy/-/issues/3618
@scoder
Copy link
Contributor

scoder commented Dec 21, 2021

Picked over to 0.29.x in af5b1ce

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants