GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-103082: Fix shifted field initialization in `instrumentation.c` by arhadthedev · Pull Request #103561 · python/cpython

Conversation

@arhadthedev

@arhadthedev arhadthedev commented Apr 15, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.

  • Issue: Implement and document PEP 669. #103082

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@arhadthedev
Fix shifted field initialization in instrumentation.c
286fdc9
@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
@bedevere-bot bedevere-bot added the awaiting review label 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
Implement and document PEP 669. #103082
Closed
3 of 4 tasks
@arhadthedev arhadthedev added the skip news label 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

arhadthedev commented Apr 19, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@arhadthedev arhadthedev mentioned this pull request Apr 19, 2023
Interpreter crashes with --with-trace-refs on #103621
Closed
kumaraditya303
kumaraditya303 approved these changes Apr 19, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Apr 19, 2023
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

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markshannon

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markshannon
markshannon merged commit a4967d9 into python:main Apr 21, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label 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
gh-103509: PEP 697 -- Limited C API for Extending Opaque Types #103511
Merged
2 tasks done
@encukou encukou mentioned this pull request Apr 24, 2023
gh-102950: Implement PEP 706 – Filter for tarfile.extractall #102953
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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@arhadthedev @bedevere-bot @markshannon @orsenthil @kumaraditya303
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.