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

bpo-36974: inherit tp_vectorcall_offset unconditionally #13858

Merged
merged 1 commit into from
Jun 24, 2019

Conversation

jdemeyer
Copy link
Contributor

@jdemeyer jdemeyer commented Jun 6, 2019

This fixes one more corner case in vectorcall inheritance. Bugfix, so needs backport to 3.8.

CC @encukou

https://bugs.python.org/issue36974

@vstinner
Copy link
Member

vstinner commented Jun 6, 2019

Does this change support the stable ABI? Is it still possible to use a C extension compiled with Python 3.6 on Python 3.9 with this change? The C extension compiled with Python 3.6 can use static types with Python 3.6 PyTypeObject which doesn't have tp_vectorcall field.

My notes on these topics:

I decided to give up on my tp_fastcall slot experiment because of these problems.

@vstinner vstinner requested a review from encukou June 6, 2019 09:08
@jdemeyer
Copy link
Contributor Author

jdemeyer commented Jun 6, 2019

Isn't the whole "stable ABI for static types" issue gone since #4944?

Regardless of that, tp_vectorcall_offset replaces tp_print, a slot which has always existed.

@jdemeyer
Copy link
Contributor Author

jdemeyer commented Jun 6, 2019

See also https://www.python.org/dev/peps/pep-0580/#using-tp-print (this is in PEP 580 which was rejected but the same applies to PEP 590).

@jdemeyer
Copy link
Contributor Author

jdemeyer commented Jun 6, 2019

which doesn't have tp_vectorcall field.

At the moment, tp_vectorcall is not used anywhere. It is meant for implementing vectorcalls of type objects (i.e. for type.__call__) but that hasn't been implemented. I know that @markshannon had some prototypes but there is no PR yet.

@vstinner
Copy link
Member

vstinner commented Jun 7, 2019

Isn't the whole "stable ABI for static types" issue gone since #4944?

Honestly... I have no idea :-( All I know is that PyQt rely on the stable ABI.

At the moment, tp_vectorcall is not used anywhere. It is meant for implementing vectorcalls of type objects (i.e. for type.call) but that hasn't been implemented.

For example, if Python 3.6 PyTypeObject is 100 bytes long, but tp_vectorcall offset is 108: setting tp_vectorcall field of a Python 3.6 static type would lead to a buffer overflow causing undefined behavior.

I don't know if it's the case. I'm just raising the question.

cc @pitrou

@jdemeyer
Copy link
Contributor Author

jdemeyer commented Jun 7, 2019

For example, if Python 3.6 PyTypeObject is 100 bytes long, but tp_vectorcall offset is 108: setting tp_vectorcall field of a Python 3.6 static type would lead to a buffer overflow causing undefined behavior.

The thing that matters is tp_vectorcall_offset. This replaces tp_print, so there is no stable ABI issue here. And tp_vectorcall_offset is an offset in the object structure, analogous to tp_dictoffset or tp_weaklistoffset. The layout of PyTypeObject doesn't matter for that.

Your concerns might be valid when we implement vectorcall for the type object itself (i.e. for type.__call__)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@jdemeyer
Copy link
Contributor Author

Can I please ask again for a review? This is the only open bugfix for PEP 590 (there are many other open tickets regarding PEP 590, but those are enhancements, not bugfixes).

@markshannon
Copy link
Member

LGTM.

@vstinner I'd like to merge this if you have no further objections.

@encukou
Copy link
Member

encukou commented Jun 24, 2019

I think the questions were answered. Victor, if you have any more, let me know.

@encukou encukou merged commit a8b27e6 into python:master Jun 24, 2019
@jdemeyer
Copy link
Contributor Author

Thanks and don't forget to backport to 3.8

@vstinner
Copy link
Member

@vstinner I'd like to merge this if you have no further objections.

I only had questions about the ABI compatibility, but I think that it was discussed in length here and in https://bugs.python.org/issue37250 In short, we don't provide any backward compatibility warranty at the ABI level for static types. If you want a stable ABI, avoid static types and use PyType_FromSpec() instead.

@miss-islington
Copy link
Contributor

Thanks @jdemeyer for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

@miss-islington
Copy link
Contributor

Thanks @jdemeyer for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-14342 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 24, 2019
(cherry picked from commit a8b27e6)

Co-authored-by: Jeroen Demeyer <J.Demeyer@UGent.be>
encukou pushed a commit that referenced this pull request Jun 25, 2019
…H-14342)

(cherry picked from commit a8b27e6)

Co-authored-by: Jeroen Demeyer <J.Demeyer@UGent.be>
@jdemeyer jdemeyer deleted the pep590_inherit branch June 26, 2019 08:18
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants