Skip to content

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

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

Merged
merged 4 commits into from
May 29, 2019

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Dec 20, 2017

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

@jdemeyer
Copy link
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
…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
Copy link
Member Author

pitrou commented May 6, 2019

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

@pitrou
Copy link
Member Author

pitrou commented May 6, 2019

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

@jdemeyer
Copy link
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
Copy link
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
Copy link
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
Copy link
Member

cc @encukou

@encukou
Copy link
Member

encukou commented May 28, 2019

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
Copy link
Member Author

pitrou commented May 29, 2019

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
Copy link
Contributor

What value do you suggest Py_TPFLAGS_HAVE_FINALIZE should have then?

The same value that it always had.

@pitrou
Copy link
Member Author

pitrou commented May 29, 2019

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
Copy link
Member

encukou commented May 29, 2019

Thank you!

@scoder
Copy link
Contributor

scoder commented Jun 1, 2019

Nice, happy to see that this was merged.

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
…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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants