Skip to content
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

gh-103082: Fix shifted field initialization in instrumentation.c #103561

Merged

Conversation

arhadthedev
Copy link
Member

@arhadthedev arhadthedev commented Apr 15, 2023

If Py_TRACE_REFS is defined, PyObject initializers without field names like:

static PyObject DISABLE =
{
_PyObject_IMMORTAL_REFCNT,
&PyBaseObject_Type
};
are compiled in a totally unusable way:

_ob_next = _PyObject_IMMORTAL_REFCNT (999999999)
_ob_prev = &PyBaseObject_Type
ob_refcnt = 0
ob_type = NULL

The reason is implicit insertion of two extra fields under #ifdef Py_TRACE_REFS:

cpython/Include/object.h

Lines 102 to 106 in 2b6f5c3

struct _object {
_PyObject_HEAD_EXTRA
Py_ssize_t ob_refcnt;
PyTypeObject *ob_type;
};

cpython/Include/object.h

Lines 65 to 76 in 2b6f5c3

#ifdef Py_TRACE_REFS
/* Define pointers to support a doubly-linked list of all live heap objects. */
#define _PyObject_HEAD_EXTRA \
PyObject *_ob_next; \
PyObject *_ob_prev;
#define _PyObject_EXTRA_INIT _Py_NULL, _Py_NULL,
#else
# define _PyObject_HEAD_EXTRA
# define _PyObject_EXTRA_INIT
#endif

This commit fixes declarations that fail the AMD64 Arch Linux TraceRefs PR buildbot. Other declarations will be fixed in another, pending PR. (edit: all other places use _PyObject_HEAD_EXTRA)

I desided to not use _PyObject_HEAD_EXTRA because it can be forgotten easily (unlike explicit field names) so we should phase it out from CPython codebase if possible.

@arhadthedev arhadthedev added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump needs backport to 3.11 only security fixes labels Apr 15, 2023
@arhadthedev arhadthedev added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 15, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @arhadthedev for commit 286fdc9 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 15, 2023
@arhadthedev arhadthedev added 3.12 bugs and security fixes and removed needs backport to 3.11 only security fixes labels Apr 15, 2023
@arhadthedev arhadthedev changed the title Fix shifted field initialization in instrumentation.c gh-103082: Fix shifted field initialization in instrumentation.c Apr 15, 2023
@arhadthedev arhadthedev requested a review from markshannon April 15, 2023 13:32
@arhadthedev arhadthedev marked this pull request as ready for review April 15, 2023 13:32
@arhadthedev
Copy link
Member Author

@markshannon as an author of gh-103083 this PR fixes.

@@ -16,14 +16,14 @@

static PyObject DISABLE =
{
_PyObject_IMMORTAL_REFCNT,
&PyBaseObject_Type
.ob_refcnt = _PyObject_IMMORTAL_REFCNT,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicit setting of these fields seems a bit odd to me.
@markshannon should review and merge it since this is recently introduced file.

@markshannon
Copy link
Member

I think the use of named initializers is fine.

Since we can always change it to use macros later, and this fixes compilation, I'm merging this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) skip news type-crash A hard crash of the interpreter, possibly with a core dump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants