Skip to content

Commit

Permalink
Revert "gh-98724: Fix Py_CLEAR() macro side effects" (#99737)
Browse files Browse the repository at this point in the history 
Revert "gh-98724: Fix Py_CLEAR() macro side effects (#99100)"

This reverts commit c03e05c.
  • Loading branch information
@vstinner
vstinner authored Nov 24, 2022
1 parent 0da7283 commit 3a803bc
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 172 deletions.
46 changes: 2 additions & 44 deletions Doc/c-api/refcounting.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
Reference Counting
******************

The functions and macros in this section are used for managing reference counts
of Python objects.
The macros in this section are used for managing reference counts of Python
objects.


.. c:function:: Py_ssize_t Py_REFCNT(PyObject *o)
Expand Down Expand Up @@ -129,11 +129,6 @@ of Python objects.
It is a good idea to use this macro whenever decrementing the reference
count of an object that might be traversed during garbage collection.
.. versionchanged:: 3.12
The macro argument is now only evaluated once. If the argument has side
effects, these are no longer duplicated.
.. c:function:: void Py_IncRef(PyObject *o)
Increment the reference count for object *o*. A function version of :c:func:`Py_XINCREF`.
Expand All @@ -144,40 +139,3 @@ of Python objects.
Decrement the reference count for object *o*. A function version of :c:func:`Py_XDECREF`.
It can be used for runtime dynamic embedding of Python.
.. c:macro:: Py_SETREF(dst, src)
Macro safely decrementing the `dst` reference count and setting `dst` to
`src`.
As in case of :c:func:`Py_CLEAR`, "the obvious" code can be deadly::
Py_DECREF(dst);
dst = src;
The safe way is::
Py_SETREF(dst, src);
That arranges to set `dst` to `src` _before_ decrementing reference count of
*dst* old value, so that any code triggered as a side-effect of `dst`
getting torn down no longer believes `dst` points to a valid object.
.. versionadded:: 3.6
.. versionchanged:: 3.12
The macro arguments are now only evaluated once. If an argument has side
effects, these are no longer duplicated.
.. c:macro:: Py_XSETREF(dst, src)
Variant of :c:macro:`Py_SETREF` macro that uses :c:func:`Py_XDECREF` instead
of :c:func:`Py_DECREF`.
.. versionadded:: 3.6
.. versionchanged:: 3.12
The macro arguments are now only evaluated once. If an argument has side
effects, these are no longer duplicated.
5 changes: 0 additions & 5 deletions Doc/whatsnew/3.12.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -822,11 +822,6 @@ Porting to Python 3.12
:class:`bytes` type is accepted for bytes strings.
(Contributed by Victor Stinner in :gh:`98393`.)

* The :c:macro:`Py_CLEAR`, :c:macro:`Py_SETREF` and :c:macro:`Py_XSETREF`
macros now only evaluate their argument once. If the argument has side
effects, these side effects are no longer duplicated.
(Contributed by Victor Stinner in :gh:`98724`.)

Deprecated
----------

Expand Down
44 changes: 20 additions & 24 deletions Include/cpython/object.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,41 +305,37 @@ _PyObject_GenericSetAttrWithDict(PyObject *, PyObject *,

PyAPI_FUNC(PyObject *) _PyObject_FunctionStr(PyObject *);

/* Safely decref `dst` and set `dst` to `src`.
/* Safely decref `op` and set `op` to `op2`.
*
* As in case of Py_CLEAR "the obvious" code can be deadly:
*
* Py_DECREF(dst);
* dst = src;
* Py_DECREF(op);
* op = op2;
*
* The safe way is:
*
* Py_SETREF(dst, src);
* Py_SETREF(op, op2);
*
* That arranges to set `dst` to `src` _before_ decref'ing, so that any code
* triggered as a side-effect of `dst` getting torn down no longer believes
* `dst` points to a valid object.
* That arranges to set `op` to `op2` _before_ decref'ing, so that any code
* triggered as a side-effect of `op` getting torn down no longer believes
* `op` points to a valid object.
*
* gh-98724: Use the _tmp_dst_ptr variable to evaluate the 'dst' macro argument
* exactly once, to prevent the duplication of side effects in this macro.
* Py_XSETREF is a variant of Py_SETREF that uses Py_XDECREF instead of
* Py_DECREF.
*/
#define Py_SETREF(dst, src) \
do { \
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
PyObject *_tmp_dst = (*_tmp_dst_ptr); \
*_tmp_dst_ptr = _PyObject_CAST(src); \
Py_DECREF(_tmp_dst); \

#define Py_SETREF(op, op2) \
do { \
PyObject *_py_tmp = _PyObject_CAST(op); \
(op) = (op2); \
Py_DECREF(_py_tmp); \
} while (0)

/* Py_XSETREF() is a variant of Py_SETREF() that uses Py_XDECREF() instead of
* Py_DECREF().
*/
#define Py_XSETREF(dst, src) \
do { \
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
PyObject *_tmp_dst = (*_tmp_dst_ptr); \
*_tmp_dst_ptr = _PyObject_CAST(src); \
Py_XDECREF(_tmp_dst); \
#define Py_XSETREF(op, op2) \
do { \
PyObject *_py_tmp = _PyObject_CAST(op); \
(op) = (op2); \
Py_XDECREF(_py_tmp); \
} while (0)


Expand Down
19 changes: 7 additions & 12 deletions Include/object.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -598,21 +598,16 @@ static inline void Py_DECREF(PyObject *op)
* one of those can't cause problems -- but in part that relies on that
* Python integers aren't currently weakly referencable. Best practice is
* to use Py_CLEAR() even if you can't think of a reason for why you need to.
*
* gh-98724: Use the _py_tmp_ptr variable to evaluate the macro argument
* exactly once, to prevent the duplication of side effects in this macro.
*/
#define Py_CLEAR(op) \
do { \
PyObject **_py_tmp_ptr = _Py_CAST(PyObject**, &(op)); \
if (*_py_tmp_ptr != NULL) { \
PyObject* _py_tmp = (*_py_tmp_ptr); \
*_py_tmp_ptr = NULL; \
Py_DECREF(_py_tmp); \
} \
#define Py_CLEAR(op) \
do { \
PyObject *_py_tmp = _PyObject_CAST(op); \
if (_py_tmp != NULL) { \
(op) = NULL; \
Py_DECREF(_py_tmp); \
} \
} while (0)


/* Function to use in case the object pointer can be NULL: */
static inline void Py_XINCREF(PyObject *op)
{
Expand Down
87 changes: 0 additions & 87 deletions Modules/_testcapimodule.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2589,91 +2589,6 @@ test_set_type_size(PyObject *self, PyObject *Py_UNUSED(ignored))
}


// Test Py_CLEAR() macro
static PyObject*
test_py_clear(PyObject *self, PyObject *Py_UNUSED(ignored))
{
// simple case with a variable
PyObject *obj = PyList_New(0);
if (obj == NULL) {
return NULL;
}
Py_CLEAR(obj);
assert(obj == NULL);

// gh-98724: complex case, Py_CLEAR() argument has a side effect
PyObject* array[1];
array[0] = PyList_New(0);
if (array[0] == NULL) {
return NULL;
}

PyObject **p = array;
Py_CLEAR(*p++);
assert(array[0] == NULL);
assert(p == array + 1);

Py_RETURN_NONE;
}


// Test Py_SETREF() and Py_XSETREF() macros, similar to test_py_clear()
static PyObject*
test_py_setref(PyObject *self, PyObject *Py_UNUSED(ignored))
{
// Py_SETREF() simple case with a variable
PyObject *obj = PyList_New(0);
if (obj == NULL) {
return NULL;
}
Py_SETREF(obj, NULL);
assert(obj == NULL);

// Py_XSETREF() simple case with a variable
PyObject *obj2 = PyList_New(0);
if (obj2 == NULL) {
return NULL;
}
Py_XSETREF(obj2, NULL);
assert(obj2 == NULL);
// test Py_XSETREF() when the argument is NULL
Py_XSETREF(obj2, NULL);
assert(obj2 == NULL);

// gh-98724: complex case, Py_SETREF() argument has a side effect
PyObject* array[1];
array[0] = PyList_New(0);
if (array[0] == NULL) {
return NULL;
}

PyObject **p = array;
Py_SETREF(*p++, NULL);
assert(array[0] == NULL);
assert(p == array + 1);

// gh-98724: complex case, Py_XSETREF() argument has a side effect
PyObject* array2[1];
array2[0] = PyList_New(0);
if (array2[0] == NULL) {
return NULL;
}

PyObject **p2 = array2;
Py_XSETREF(*p2++, NULL);
assert(array2[0] == NULL);
assert(p2 == array2 + 1);

// test Py_XSETREF() when the argument is NULL
p2 = array2;
Py_XSETREF(*p2++, NULL);
assert(array2[0] == NULL);
assert(p2 == array2 + 1);

Py_RETURN_NONE;
}


#define TEST_REFCOUNT() \
do { \
PyObject *obj = PyList_New(0); \
Expand Down Expand Up @@ -3337,8 +3252,6 @@ static PyMethodDef TestMethods[] = {
{"pynumber_tobase", pynumber_tobase, METH_VARARGS},
{"without_gc", without_gc, METH_O},
{"test_set_type_size", test_set_type_size, METH_NOARGS},
{"test_py_clear", test_py_clear, METH_NOARGS},
{"test_py_setref", test_py_setref, METH_NOARGS},
{"test_refcount_macros", test_refcount_macros, METH_NOARGS},
{"test_refcount_funcs", test_refcount_funcs, METH_NOARGS},
{"test_py_is_macros", test_py_is_macros, METH_NOARGS},
Expand Down

0 comments on commit 3a803bc

Please sign in to comment.