[WIP, DO NOT MERGE] bpo-41188: Prepare CPython for opague PyObject structure. #21262

WildCard65 · 2020-07-01T16:04:12Z

The end goal of this pull request is to make the structure "PyObject" opaque to EVERYTHING except Python core.

To accomplish this, the following must be done:

Add new member to 'PyTypeObject' to hold pointer offset to type's data structure.
Add implementation defined "slib_python310" static library for end users to link against.
Determine if more changes are required.

https://bugs.python.org/issue41188

This member is the companion member required for 'Py_TPFLAGS_USES_OPAQUE_OBJECT' flag.

vstinner

Add implementation defined "slib_python310" static library for end users to link against.

I don't understand this.

Change as many types as possible to use 'Py_TPFLAGS_USES_OPAQUE_OBJECT'

Please don't do that in the same PR.

vstinner · 2020-07-01T17:06:05Z

Include/object.h

@@ -317,6 +317,9 @@ Code can use PyType_HasFeature(type_ob, flag_value) to test whether the
 given type object has a specified feature.
 */

+/* Set if the type object's tp_basicsize is set for opague object */
+#define Py_TPFLAGS_OPAQUE_OBJECT (1UL << 8)


I would prefer a more explicit name, lilke: Py_TPFLAGS_OMIT_PYOBJECT_SIZE.

vstinner · 2020-07-01T17:07:35Z

I would prefer to open a separated issue on bugs.python.org for this work.

WildCard65 · 2020-07-01T17:32:52Z

@vstinner The static library will contain '.c' files that reference internal implementations FOR the runtime it's designed for.
The goal of the static library is that end users link against it when building their extensions WITHOUT having access to internals directly.

As it currently stands, the current Py_TYPE still requires direct access to internals for end users while this static library remove that requirement.

example would be:
Py_TYPE macro references a method called "PyObject_GetType", but the definition of the method is not in the header, instead, it's in the static library which will be linked into the final binary.

This allows for implementation independent access while still allowing the C compiler to aggressively inline out the small functions.

…thon's internals and external users. Note: This static library is incomplete.

WildCard65 · 2020-07-01T18:56:53Z

@vstinner Before I proceed any further, I want you to review "slib_pythoncore" first.

vstinner · 2020-07-01T21:18:53Z

The goal of the static library is that end users link against it when building their extensions WITHOUT having access to internals directly.

This is the purpose of the limited C API which provides a stable ABI. It's already implemented since Python 3.2. I don't see the point of adding a second flavor of the "libpython" library.

WildCard65 · 2020-07-01T21:30:02Z

As it stands, functions like _Py_TYPE, _Py_SIZE, _Py_INCREF, _Py_DECREF rely on "PyObject" being a complete type. Opaque objects on the other hand are classified as incomplete types.

Incomplete types can't be used for anything but pointers (*) and references (&), these functions will fail when PyObject goes opaque in the limited API, infact, sizeof() also fails on incomplete types.

The purpose of the static library is not to be a different flavour of Python, but to act as a bridge into Python's internals while respecting the limitations of incomplete types, while also eliminating function call overhead.

As an added bonus, this static library can be "reskinned" for runtimes that take a different approach to PyObject (if any) and descendant built-in types while end users don't have to worry about what the runtime's schematic is.

WildCard65 · 2020-07-02T02:24:45Z

Added a basic implementation of the flag, but I do not know where all usages of "tp_basicsize" is, I'm also debating on whether or not I need to make GET_TYPE_TOTALSIZE into an inlinable function.

vstinner

IMHO you gone too far in the implementation before discussing the design. You should first propose the design on python-ideas or python-dev list.

vstinner · 2020-07-02T10:37:27Z

Include/cpython/object.h

@@ -263,6 +263,10 @@ struct _typeobject {

    destructor tp_finalize;
    vectorcallfunc tp_vectorcall;
+
+    /* INTERNAL USE ONLY! MODIFYING THIS CAN CRASH PYTHON! */
+    const Py_ssize_t tp_obj_offset; /* Offset from "PyObject *" pointer */


I don't think that this is needed. When I get a "PyObject *ob", the offset to the PyObject structure is 0: "PyObject copy = *ob;" in Python internals is valid.

It's not required if everything inherited ONLY PyObject, that will never be true.

The offset is calculated from the immediate base type of the type, which can be for example: list, tuple, dict, type, custom types.

Same thing applies to "tp_obj_size", without these members, heap corruption can be very likely.

vstinner · 2020-07-02T10:38:47Z

Include/cpython/object.h

+
+    /* INTERNAL USE ONLY! MODIFYING THIS CAN CRASH PYTHON! */
+    const Py_ssize_t tp_obj_offset; /* Offset from "PyObject *" pointer */
+    const Py_ssize_t tp_obj_size; /* Total memory allocation size */


I don't see the point of adding a new member. You can just check for Py_TPFLAGS_USES_OPAQUE_OBJECT flag in functions like PyType_GenericAlloc(), _PyObject_SIZE() and _PyObject_VAR_SIZE(). If the flag is set, add sizeof(PyObject).

See previous comment.

vstinner · 2020-07-02T10:40:08Z

Include/object.h

+Py_SLIB_LOCAL(PyTypeObject *) PyObject_GetType(const PyObject *ob);
+
+Py_SLIB_LOCAL(void) PyObject_SetRefCount(PyObject *ob, const Py_ssize_t refcnt);
+Py_SLIB_LOCAL(void) PyObject_SetType(PyObject *ob, const PyTypeObject *type);


Python 3.9 already have Py_SET_REFCNT() and Py_SET_TYPE() functions. What is the advantage of adding new functions?

These aren't essentially "new functions", they are bridges into CPython's internals WITHOUT leaking implementation details.

As mentioned before, the existing functions WILL FAIL when PyObject goes full opaque.

vstinner · 2020-07-02T12:26:48Z

Py_TPFLAGS_USES_OPAQUE_OBJECT looks useful, but other changes like "slib" looks useless to me.

vstinner

I don't see the point of tp_obj_offset, tp_obj_size and slib.

bedevere-bot · 2020-07-02T12:27:34Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

WildCard65 · 2020-07-02T13:46:38Z

Consider the following:

struct myobject;

void use_myobject(struct myobect *obj); // Valid, incomplete types can be used as pointers.

size_t get_object_value(struct my object *obj)
{
    return obj->m_value; // ERROR: my_object is an incomplete type!
}

Above demonstrates why functions like _Py_Type will fail in limited api.

'Py_TPFLAGS_OMIT_OBJECT_SIZE' was intended as the stepping stone for making types like PyTypeObject opaque as well, this makes knowing the true offset off the base time a runtime dependant scenario, one with minimal impact to performance, probably less than having _Py_TYPE, etc, exported from python binary.

WildCard65 · 2020-07-02T20:12:19Z

@vstinner One way or another, PY_TYPE and co need to have their implementations PURGED from the Python header files for a truly opaque PyObject.

Another thing that needs to be purged is "ob_base" usage of objects like PyTupleObject, PyTypeObject, PyListObject to name a few.

I'm willing to replace slib with a precompiled header trio, but that's a can of worms on its own (at least on MSVC).

I'm not budging in regards to "tp_obj_offset" and "tp_obj_size", see above mentioned purging.

the-knights-who-say-ni added the CLA signed label Jul 1, 2020

bedevere-bot added the awaiting review label Jul 1, 2020

Added new type member 'tp_obj_offset'.

3d5826b

This member is the companion member required for 'Py_TPFLAGS_USES_OPAQUE_OBJECT' flag.

WildCard65 force-pushed the bpo-39573 branch from 6457e92 to 3d5826b Compare July 1, 2020 16:05

vstinner reviewed Jul 1, 2020

View reviewed changes

WildCard65 changed the title ~~[WIP, DO NOT MERGE] bpo-39573: Prepare CPython for opague PyObject structure.~~ [WIP, DO NOT MERGE] bpo-41188: Prepare CPython for opague PyObject structure. Jul 1, 2020

Added static library "slib_pythoncore" to act as a medium between CPy…

ff05339

…thon's internals and external users. Note: This static library is incomplete.

WildCard65 requested a review from vstinner July 1, 2020 18:57

Added basic implementation of "Py_TPFLAGS_OMIT_PYOBJECT_SIZE".

8aa000e

WildCard65 added 2 commits July 1, 2020 22:29

Fixed GCC error

8774b02

Add forgotten member documentation.

da3b98a

vstinner reviewed Jul 2, 2020

View reviewed changes

vstinner requested changes Jul 2, 2020

View reviewed changes

bedevere-bot removed the awaiting review label Jul 2, 2020

bedevere-bot added the awaiting changes label Jul 2, 2020

Trick PyObject into believing it's an opaque type.

686d657

WildCard65 closed this Sep 15, 2020

WildCard65 mannequin mentioned this pull request Jun 25, 2022

Prepare CPython for opaque PyObject structure. #85360

Closed

vstinner mentioned this pull request Mar 12, 2024

[C API] Avoid accessing PyObject and PyVarObject members directly: add Py_SET_TYPE() and Py_IS_TYPE(), disallow Py_TYPE(obj)=type #83754

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP, DO NOT MERGE] bpo-41188: Prepare CPython for opague PyObject structure. #21262

[WIP, DO NOT MERGE] bpo-41188: Prepare CPython for opague PyObject structure. #21262

WildCard65 commented Jul 1, 2020 •

edited

Loading

vstinner left a comment

vstinner Jul 1, 2020

vstinner commented Jul 1, 2020

WildCard65 commented Jul 1, 2020

WildCard65 commented Jul 1, 2020

vstinner commented Jul 1, 2020

WildCard65 commented Jul 1, 2020 •

edited

Loading

WildCard65 commented Jul 2, 2020

vstinner left a comment

vstinner Jul 2, 2020

WildCard65 Jul 2, 2020

vstinner Jul 2, 2020

WildCard65 Jul 2, 2020

vstinner Jul 2, 2020

WildCard65 Jul 2, 2020

vstinner commented Jul 2, 2020

vstinner left a comment

bedevere-bot commented Jul 2, 2020

WildCard65 commented Jul 2, 2020 •

edited

Loading

WildCard65 commented Jul 2, 2020 •

edited

Loading

[WIP, DO NOT MERGE] bpo-41188: Prepare CPython for opague PyObject structure. #21262

[WIP, DO NOT MERGE] bpo-41188: Prepare CPython for opague PyObject structure. #21262

Conversation

WildCard65 commented Jul 1, 2020 • edited Loading

vstinner left a comment

Choose a reason for hiding this comment

vstinner Jul 1, 2020

Choose a reason for hiding this comment

vstinner commented Jul 1, 2020

WildCard65 commented Jul 1, 2020

WildCard65 commented Jul 1, 2020

vstinner commented Jul 1, 2020

WildCard65 commented Jul 1, 2020 • edited Loading

WildCard65 commented Jul 2, 2020

vstinner left a comment

Choose a reason for hiding this comment

vstinner Jul 2, 2020

Choose a reason for hiding this comment

WildCard65 Jul 2, 2020

Choose a reason for hiding this comment

vstinner Jul 2, 2020

Choose a reason for hiding this comment

WildCard65 Jul 2, 2020

Choose a reason for hiding this comment

vstinner Jul 2, 2020

Choose a reason for hiding this comment

WildCard65 Jul 2, 2020

Choose a reason for hiding this comment

vstinner commented Jul 2, 2020

vstinner left a comment

Choose a reason for hiding this comment

bedevere-bot commented Jul 2, 2020

WildCard65 commented Jul 2, 2020 • edited Loading

WildCard65 commented Jul 2, 2020 • edited Loading

WildCard65 commented Jul 1, 2020 •

edited

Loading

WildCard65 commented Jul 1, 2020 •

edited

Loading

WildCard65 commented Jul 2, 2020 •

edited

Loading

WildCard65 commented Jul 2, 2020 •

edited

Loading