FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ cpython Public

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
ConversationCommits (1)ChecksFiles changed
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 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

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 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 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 15, 2023

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 removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 15, 2023
arhadthedev added 3.12 only security fixes and removed needs backport to 3.11 only security fixes labels Apr 15, 2023
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 mentioned this pull request Apr 15, 2023
Closed
4 tasks
arhadthedev requested a review from markshannon April 15, 2023 13:32
arhadthedev marked this pull request as ready for review April 15, 2023 13:32

arhadthedev commented Apr 19, 2023

Copy link
Copy Markdown
Member Author

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

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

markshannon commented Apr 21, 2023

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

markshannon merged commit a4967d9 into python:main Apr 21, 2023
arhadthedev deleted the fix-instrumentation-disable-missing branch April 21, 2023 13:27
arhadthedev mentioned this pull request Apr 21, 2023
Merged
2 tasks
encukou mentioned this pull request Apr 24, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

orsenthil orsenthil left review comments
kumaraditya303 kumaraditya303 approved these changes
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

Back | FazBrowse Home | New Git URL