◐ Shell
reader mode source ↗
Skip to content

bpo-32388: Remove cross-version binary compatibility requirement in tp_flags#4944

Merged
pitrou merged 4 commits into
python:masterfrom
pitrou:no_tpflags_compatibility
May 29, 2019
Merged

bpo-32388: Remove cross-version binary compatibility requirement in tp_flags#4944
pitrou merged 4 commits into
python:masterfrom
pitrou:no_tpflags_compatibility

Conversation

@pitrou

@pitrou pitrou commented Dec 20, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Starting with 3.8, it is now allowed to add new fields at the end of the PyTypeObject struct in feature releases without having to allocate a dedicated compatibility flag in tp_flags. This will reduce the risk of running out of bits in the 32-bit tp_flags value.

https://bugs.python.org/issue32388

@pitrou pitrou requested review from 1st1 and asvetlov as code owners December 20, 2017 17:29
@jdemeyer

jdemeyer commented Jul 31, 2018

Copy link
Copy Markdown
Contributor

You write "Starting with 3.7, binary compatibility of C extensions accross feature releases of Python is not supported anymore" but has such binary compatibility ever been supported? This PR seems to imply that it was, but PEP 384 explicitly says that the layout of PyTypeObject is not part of the stable ABI.

PS: typo accross -> across

@pitrou pitrou closed this May 6, 2019
@pitrou pitrou force-pushed the no_tpflags_compatibility branch from d49cf6f to f7b494c Compare May 6, 2019 16:30
@pitrou pitrou reopened this May 6, 2019
@pitrou
bpo-32388: Remove cross-version binary compatibility requirement in t… …
19770ae 
…p_flags

It is now allowed to add new fields at the end of the PyTypeObject struct
(and even in the middle, if we are so inclined) in feature releases without
having to allocate a dedicated compatibility flag in tp_flags.

This will reduce the risk of running out of bits in the 32-bit tp_flags value.
@pitrou pitrou force-pushed the no_tpflags_compatibility branch from 57ea6a3 to 19770ae Compare May 6, 2019 16:33
@pitrou

pitrou commented May 6, 2019

Copy link
Copy Markdown
Member Author

I updated this PR against master and addressed the review comments.

@pitrou

pitrou commented May 6, 2019

Copy link
Copy Markdown
Member Author

@zooba Is there a way to retry an Azure job which failed because it couldn't "provision the agent"?

@pitrou pitrou added and removed awaiting merge labels May 6, 2019
@jdemeyer

jdemeyer commented May 15, 2019

Copy link
Copy Markdown
Contributor

It would be good to get this done before the first 3.8 beta.

@scoder: is there any chance that you could finish the "core review"?

@jdemeyer

jdemeyer commented May 15, 2019

Copy link
Copy Markdown
Contributor

Starting with 3.8, it is now allowed to add new fields at the end of the PyTypeObject struct (and even in the middle, if we are so inclined)

Not "in the middle" as that would break the typical code of the form

PyTypeObject PyCFunction_Type = {
    PyVarObject_HEAD_INIT(&PyType_Type, 0)
    "builtin_function_or_method",
    sizeof(PyCFunctionObject),
    0,
    (destructor)meth_dealloc,                   /* tp_dealloc */
    0,                                          /* tp_print */
    0,                                          /* tp_getattr */
    0,                                          /* tp_setattr */
    0,                                          /* tp_reserved */
    (reprfunc)meth_repr,                        /* tp_repr */
    0,                                          /* tp_as_number */
    0,                                          /* tp_as_sequence */
    0,                                          /* tp_as_mapping */
    (hashfunc)meth_hash,                        /* tp_hash */
    PyCFunction_Call,                           /* tp_call */
    0,                                          /* tp_str */
    PyObject_GenericGetAttr,                    /* tp_getattro */
    0,                                          /* tp_setattro */
    0,                                          /* tp_as_buffer */
    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC,/* tp_flags */
    0,                                          /* tp_doc */
    (traverseproc)meth_traverse,                /* tp_traverse */
    0,                                          /* tp_clear */
    meth_richcompare,                           /* tp_richcompare */
    offsetof(PyCFunctionObject, m_weakreflist), /* tp_weaklistoffset */
    0,                                          /* tp_iter */
    0,                                          /* tp_iternext */
    meth_methods,                               /* tp_methods */
    meth_members,                               /* tp_members */
    meth_getsets,                               /* tp_getset */
    0,                                          /* tp_base */
    0,                                          /* tp_dict */
};

With C99, there are better ways to write the above, but we should still support that.

@jdemeyer

jdemeyer commented May 15, 2019

Copy link
Copy Markdown
Contributor

One thing that keeps bugging me when I see this diff: Do we really have to change the value of Py_TPFLAGS_HAVE_FINALIZE? I don't see any immediate problem with changing the value, but it would be more prudent to keep it as 1 << 0.

@vstinner

vstinner commented May 27, 2019

Copy link
Copy Markdown
Member

cc @encukou

@encukou

encukou commented May 28, 2019

Copy link
Copy Markdown
Member

Can we keep the bit for backwards compatibility?
We're not running out of bits any time soon, especially since we'll not be adding one for each new feature. (Even though I want to add two of them now.)

Pedantically, something like MY_USUAL_FLAGS & ~Py_TPFLAGS_HAVE_FINALIZE would be correct in 3.7, so it should work in 3.8 as well.

@pitrou

pitrou commented May 29, 2019

Copy link
Copy Markdown
Member Author

Pedantically, something like MY_USUAL_FLAGS & ~Py_TPFLAGS_HAVE_FINALIZE would be correct in 3.7, so it should work in 3.8 as well.

What value do you suggest Py_TPFLAGS_HAVE_FINALIZE should have then?

@jdemeyer

jdemeyer commented May 29, 2019

Copy link
Copy Markdown
Contributor

What value do you suggest Py_TPFLAGS_HAVE_FINALIZE should have then?

The same value that it always had.

@pitrou

pitrou commented May 29, 2019

Copy link
Copy Markdown
Member Author

Ok. Unless someone opposes, I'll merge this once CI is green.

@pitrou pitrou merged commit ada319b into python:master May 29, 2019
@pitrou pitrou deleted the no_tpflags_compatibility branch May 29, 2019 20:12
@encukou

encukou commented May 29, 2019

Copy link
Copy Markdown
Member

Thank you!

@scoder

scoder commented Jun 1, 2019

Copy link
Copy Markdown
Contributor

Nice, happy to see that this was merged.

@jdemeyer jdemeyer mentioned this pull request Jun 6, 2019
Merged
@jdemeyer jdemeyer mentioned this pull request Jul 2, 2019
Merged
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@pitrou @DinoV
bpo-32388: Remove cross-version binary compatibility requirement in t… …
6275b36 
…p_flags (pythonGH-4944)

It is now allowed to add new fields at the end of the PyTypeObject struct without having to allocate a dedicated compatibility flag in tp_flags.

This will reduce the risk of running out of bits in the 32-bit tp_flags value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scoder scoder scoder left review comments

@jdemeyer jdemeyer jdemeyer left review comments

@tirkarthi tirkarthi tirkarthi left review comments

@1st1 1st1 Awaiting requested review from 1st1

@asvetlov asvetlov

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

+1 more reviewer

@JimJJewett JimJJewett JimJJewett left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@pitrou @jdemeyer @vstinner @encukou @scoder @tirkarthi @JimJJewett @the-knights-who-say-ni @bedevere-bot