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 — Add Py_SETREF/Py_XSETREF to old limited C API versions by vstinner · Pull Request #187 · python/pythoncapi-compat

Conversation

@vstinner

@vstinner vstinner commented Oct 7, 2026

Copy link
Copy Markdown
Member

No description provided.

Sorry, something went wrong.

Uh oh!

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

prathamhole14 reacted with rocket emoji
All reactions
  • 🚀 1 reaction
@vstinner vstinner mentioned this pull request Oct 7, 2026
Support the limited API? #183
Closed
@vstinner

vstinner commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

@ngoldbaum: Would you mind to review this change?

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner mentioned this pull request Oct 7, 2026
ENH: Limited API fallbacks for Py_SETREF and pythoncapi-compat numpy/numpy#32864
Closed
@vstinner

vstinner commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

cc @prathamhole14

All reactions

Sorry, something went wrong.

Uh oh!

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

@ngoldbaum

ngoldbaum commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

I asked an AI model to look this over and it spotted an issue (CPython's fallback also has this problem):

The memcpy() protects the write, but the preceding read still violates strict aliasing:

PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst));
PyObject *_tmp_old_dst = (*_tmp_dst_ptr);

If dst is a PyTypeObject *, dereferencing _tmp_dst_ptr accesses a PyTypeObject * object through a PyObject * lvalue. GCC can therefore assume that this read does not observe a write made through PyTypeObject **.

Here is a standalone reproducer using the exact Py_XSETREF() implementation from this PR:

#include <Python.h>

#include <cstdio>
#include <cstring>

#ifndef _Py_CAST
#  define _Py_CAST(type, expr) ((type)(expr))
#endif
#ifndef _PyObject_CAST
#  define _PyObject_CAST(obj) _Py_CAST(PyObject *, (obj))
#endif

#define REPRO_XSETREF(dst, src) \
    do { \
        PyObject **_tmp_dst_ptr = _Py_CAST(PyObject **, &(dst)); \
        PyObject *_tmp_old_dst = (*_tmp_dst_ptr); \
        PyObject *_tmp_src = _PyObject_CAST(src); \
        memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject *)); \
        Py_XDECREF(_tmp_old_dst); \
    } while (0)

__attribute__((noinline))
static void
replace(PyTypeObject **a, PyTypeObject **b,
        PyTypeObject *first, PyTypeObject *second)
{
    *a = first;
    REPRO_XSETREF(*b, second);
}

int
main()
{
    Py_Initialize();

    PyType_Slot slots[] = {{0, NULL}};
    PyType_Spec spec = {
        "repro.T", sizeof(PyObject), 0, Py_TPFLAGS_DEFAULT, slots
    };
    PyObject *first = PyType_FromSpec(&spec);
    PyObject *second = PyType_FromSpec(&spec);
    if (first == NULL || second == NULL) {
        PyErr_Print();
        return 1;
    }

    Py_ssize_t before = Py_REFCNT(first);
    PyTypeObject *slot = NULL;

    // Both pointer arguments alias. The assignment makes first the old value
    // that REPRO_XSETREF() must release before storing second.
    replace(&slot, &slot,
            reinterpret_cast<PyTypeObject *>(Py_NewRef(first)),
            reinterpret_cast<PyTypeObject *>(Py_NewRef(second)));

    std::printf("first refcount: %zd; expected: %zd\n",
                Py_REFCNT(first), before);
    return 0;
}

On CPython 3.10 with GCC 15.2.0:

$ g++ -std=c++11 $(python3.10-config --cflags) \
      -DPy_LIMITED_API=0x030a0000 -O2 -fstrict-aliasing \
      repro.cpp $(python3.10-config --embed --ldflags) -o repro
$ ./repro
first refcount: 3; expected: 2

The reference acquired by Py_NewRef(first) was not released. Compiling the same source with -fno-strict-aliasing produces the expected refcount of 2.

With Py_SETREF() instead of Py_XSETREF(), the stale value can be NULL, causing a segmentation fault when it is passed to Py_DECREF().

prathamhole14 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

The memcpy() protects the write, but the preceding read still violates strict aliasing

This issue is super annoying. What do you recommend to fix the issue? Copy/paste the whole Py_SETREF/XSETREF implementation to use typeof() / __typeof__() and auto on C++?

All reactions

Sorry, something went wrong.

Uh oh!

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

@ngoldbaum

ngoldbaum commented Oct 7, 2026 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

The AI model's suggestion is two memcpys:

#ifndef Py_SETREF
#define Py_SETREF(dst, src) \
    do { \
        PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
        PyObject *_tmp_old_dst; \
        memcpy(&_tmp_old_dst, _tmp_dst_ptr, sizeof(PyObject*)); \
        PyObject *_tmp_src = _PyObject_CAST(src); \
        memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
        Py_DECREF(_tmp_old_dst); \
    } while (0)
#endif
#ifndef Py_XSETREF
#define Py_XSETREF(dst, src) \
    do { \
        PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
        PyObject *_tmp_old_dst; \
        memcpy(&_tmp_old_dst, _tmp_dst_ptr, sizeof(PyObject*)); \
        PyObject *_tmp_src = _PyObject_CAST(src); \
        memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
        Py_XDECREF(_tmp_old_dst); \
    } while (0)
#endif

I don't think this is a performance concern, the compiler should be able to replace the memcpys with a load or store.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

If we change the memcpy() implementation of Py_SETREF(), the fix should happen in Python first.

All reactions

Sorry, something went wrong.

Uh oh!

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

@ngoldbaum

ngoldbaum commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Agreed, sorry for missing this over there. I didn't get a chance to look this week.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

No problem. I will prepare a fix for Python later.

prathamhole14 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@prathamhole14 prathamhole14 mentioned this pull request Oct 8, 2026
ENH: Update pythoncapi-compat and add Limited API fallbacks for Py_SETREF numpy/numpy#32871
Merged
@mattip

mattip commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Is this python/cpython#159143 ?

All reactions

Sorry, something went wrong.

Uh oh!

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

@mattip

mattip commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

Did the CPython work happen?

All reactions

Sorry, something went wrong.

Uh oh!

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

@prathamhole14

prathamhole14 commented Oct 18, 2026 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

I just checked the aliasing problem is not yet fixed in cpython, should I open the PR @vstinner ?

All reactions

Sorry, something went wrong.

Uh oh!

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

@ngoldbaum

ngoldbaum commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

should I open the PR

Yes, please do open a CPython PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

@ngoldbaum ngoldbaum mentioned this pull request Oct 29, 2026
BUG: fix incorrect reference setting in reduction loop tests numpy/numpy#33175
Closed
@vstinner vstinner mentioned this pull request Nov 2, 2026
[C API] Py_CLEAR(), Py_SETREF() and Py_XSETREF() memcpy() implementation is not safe with strict aliasing python/cpython#161185
Closed
@vstinner

vstinner commented Nov 2, 2026

Copy link
Copy Markdown
Member Author

@ngoldbaum:

The memcpy() protects the write, but the preceding read still violates strict aliasing

That's bad. I created python/cpython#161185 to fix the issue in Python upstream.

@mattip:

Is this python/cpython#159143 ?

Py_SETREF() macros family had multiple issues, python/cpython#159143 was the previous bugfix for C++ compatibility.

prathamhole14 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner mentioned this pull request Nov 4, 2026
gh-161185: Fix strict aliasing issue in Py_SETREF() macros family python/cpython#161186
Merged
@vstinner
Add Py_SETREF/Py_XSETREF to old limited C API versions
6f43d14
@vstinner
vstinner force-pushed the setref_limited branch from a569133 to 6f43d14 Compare November 4, 2026 22:00
@vstinner
vstinner merged commit d3ebabf into python:main Nov 4, 2026
46 checks passed

Uh oh!

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

@vstinner

vstinner commented Nov 4, 2026

Copy link
Copy Markdown
Member Author

Aaaand, it's now merged! @ngoldbaum: Thanks for spotting the strict aliasing issue!

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner
vstinner deleted the setref_limited branch November 4, 2026 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

4 participants

@vstinner @ngoldbaum @mattip @prathamhole14
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.