bpo-36974: inherit tp_vectorcall_offset unconditionally by jdemeyer · Pull Request #13858 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation15 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

jdemeyer

@vstinner

@jdemeyer

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

@jdemeyer

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

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

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__)

@jdemeyer

@jdemeyer

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

LGTM.

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

encukou

@encukou

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

@jdemeyer

Thanks and don't forget to backport to 3.8

@vstinner

@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

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

@miss-islington

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

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

@bedevere-bot

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jun 24, 2019

@jdemeyer @miss-islington

(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

lisroach pushed a commit to lisroach/cpython that referenced this pull request

Sep 10, 2019

@jdemeyer @lisroach

DinoV pushed a commit to DinoV/cpython that referenced this pull request

Jan 14, 2020

@jdemeyer @DinoV