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 — bpo-36974: inherit tp_vectorcall_offset unconditionally by jdemeyer · Pull Request #13858 · python/cpython

Conversation

@jdemeyer

@jdemeyer jdemeyer commented Jun 6, 2019 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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

CC @encukou

https://bugs.python.org/issue36974

Sorry, something went wrong.

Uh oh!

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

All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jun 6, 2019
@bedevere-bot bedevere-bot added the awaiting review label Jun 6, 2019
@vstinner

vstinner commented Jun 6, 2019

Copy link
Copy Markdown
Member

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:

  • https://pythoncapi.readthedocs.io/type_object.html
  • https://pythoncapi.readthedocs.io/stable_abi.html

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner
vstinner requested a review from encukou June 6, 2019 09:08
@jdemeyer

jdemeyer commented Jun 6, 2019 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@jdemeyer

jdemeyer commented Jun 6, 2019

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@jdemeyer

jdemeyer commented Jun 6, 2019

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Jun 7, 2019

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@jdemeyer

jdemeyer commented Jun 7, 2019

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@jdemeyer jdemeyer mentioned this pull request Jun 7, 2019
bpo-37191: Move PEP 590 vectorcall tests #13892
Merged
@jdemeyer
bpo-36974: inherit tp_vectorcall_offset unconditionally
4782dd1
@jdemeyer
jdemeyer force-pushed the pep590_inherit branch from 4770b0a to 4782dd1 Compare June 8, 2019 16:37
@jdemeyer jdemeyer mentioned this pull request Jun 11, 2019
bpo-37207: Use PEP 590 vectorcall to speed up range(), list() and dict() by about 30% #13930
Closed
@jdemeyer

jdemeyer commented Jun 16, 2019

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Jun 23, 2019

Copy link
Copy Markdown
Member

LGTM.

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

All reactions

Sorry, something went wrong.

Uh oh!

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

encukou
encukou approved these changes Jun 24, 2019
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Jun 24, 2019
@encukou

encukou commented Jun 24, 2019

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@encukou
encukou merged commit a8b27e6 into python:master Jun 24, 2019
@bedevere-bot bedevere-bot removed the awaiting merge label Jun 24, 2019
@jdemeyer

jdemeyer commented Jun 24, 2019

Copy link
Copy Markdown
Contributor Author

Thanks and don't forget to backport to 3.8

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Jun 24, 2019

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@encukou encukou added the needs backport to 3.8 label Jun 24, 2019
@miss-islington

miss-islington commented Jun 24, 2019

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington

miss-islington commented Jun 24, 2019

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@encukou encukou added needs backport to 3.8 and removed needs backport to 3.8 labels Jun 24, 2019
@miss-islington

miss-islington commented Jun 24, 2019

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot

bedevere-bot commented Jun 24, 2019

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the needs backport to 3.8 label Jun 24, 2019
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 24, 2019
@jdemeyer @miss-islington
bpo-36974: inherit tp_vectorcall_offset unconditionally (pythonGH-13858)
8d7a23d
(cherry picked from commit a8b27e6)

Co-authored-by: Jeroen Demeyer <[email protected]>
encukou pushed a commit that referenced this pull request Jun 25, 2019
@miss-islington @jdemeyer @encukou
bpo-36974: inherit tp_vectorcall_offset unconditionally (GH-13858) (G…
26fe6c3
…H-14342)

(cherry picked from commit a8b27e6)

Co-authored-by: Jeroen Demeyer <[email protected]>
@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
@jdemeyer @lisroach
bpo-36974: inherit tp_vectorcall_offset unconditionally (pythonGH-13858)
c89351c
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@jdemeyer @DinoV
bpo-36974: inherit tp_vectorcall_offset unconditionally (pythonGH-13858)
515285a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou encukou approved these changes

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.

7 participants

@jdemeyer @vstinner @markshannon @encukou @miss-islington @bedevere-bot @the-knights-who-say-ni
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 01402503 — 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.