Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
markshannon merged 1 commit into from
Apr 21, 2023
Merged

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

markshannon merged 1 commit into from
Apr 21, 2023

Conversation

@arhadthedev
Copy link
Member

@arhadthedev arhadthedev commented Apr 15, 2023 •
edited
Loading

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

cpython/Python/instrumentation.c

Lines 17 to 21 in 2b6f5c3

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

bedevere-bot commented Apr 15, 2023

🤖 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 only 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
@bedevere-bot bedevere-bot mentioned this pull request Apr 15, 2023
Closed
4 tasks
@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

arhadthedev commented Apr 19, 2023

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

@arhadthedev arhadthedev mentioned this pull request Apr 19, 2023
Closed
orsenthil
orsenthil reviewed Apr 21, 2023
View reviewed changes
Python/instrumentation.c
{
_PyObject_IMMORTAL_REFCNT,
&PyBaseObject_Type
.ob_refcnt = _PyObject_IMMORTAL_REFCNT,
Copy link
Member

@orsenthil orsenthil Apr 21, 2023

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

markshannon commented Apr 21, 2023

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.

@markshannon markshannon merged commit a4967d9 into python:main Apr 21, 2023
@arhadthedev arhadthedev deleted the fix-instrumentation-disable-missing branch April 21, 2023 13:27
@arhadthedev arhadthedev mentioned this pull request Apr 21, 2023
Merged
2 tasks
@encukou encukou mentioned this pull request Apr 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@orsenthil orsenthil orsenthil left review comments

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

@markshannon markshannon Awaiting requested review from markshannon

Assignees

No one assigned

Labels

3.12 only 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@arhadthedev @bedevere-bot @markshannon @orsenthil @kumaraditya303