gh-103082: Fix shifted field initialization in `instrumentation.c` by arhadthedev · Pull Request #103561 · python/cpython · GitHub
Skip to content

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

Merged
markshannon merged 1 commit into
python:mainfrom
arhadthedev:fix-instrumentation-disable-missing
Apr 21, 2023
Merged

gh-103082: Fix shifted field initialization in instrumentation.c#103561
markshannon merged 1 commit into
python:mainfrom
arhadthedev:fix-instrumentation-disable-missing

Conversation

@arhadthedev

@arhadthedev arhadthedev commented Apr 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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

cpython/Python/instrumentation.c

Lines 17 to 21 in 2b6f5c3

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

bedevere-bot commented Apr 15, 2023

Copy link
Copy Markdown

🤖 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

arhadthedev commented Apr 19, 2023

Copy link
Copy Markdown
Member Author

@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
Comment thread Python/instrumentation.c
{
_PyObject_IMMORTAL_REFCNT,
&PyBaseObject_Type
.ob_refcnt = _PyObject_IMMORTAL_REFCNT,

@orsenthil orsenthil Apr 21, 2023

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

markshannon commented Apr 21, 2023

Copy link
Copy Markdown
Member

@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