Use the Py_SETREF macro

@loewis

BPO	20440
Nosy	@loewis, @brettcannon, @birkenfeld, @rhettinger, @pitrou, @kristjanvalur, @vstinner, @taleinat, @larryhastings, @benjaminp, @JimJJewett, @serhiy-storchaka
Files	py_replace.spatch py_replace-3.4.patch py_replace-3.3.patch py_replace-2.7.patch py_setref.patch py_setref.cocci py_setref_extra.patch py_setref_extra2.patch py_setref_extra3.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-01-05.19:34:57.395>
created_at = <Date 2014-01-29.18:14:18.675>
labels = ['extension-modules', 'interpreter-core', 'type-crash']
title = 'Use the Py_SETREF macro'
updated_at = <Date 2016-01-05.19:34:57.328>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2016-01-05.19:34:57.328>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-01-05.19:34:57.395>
closer = 'serhiy.storchaka'
components = ['Extension Modules', 'Interpreter Core']
creation = <Date 2014-01-29.18:14:18.675>
creator = 'serhiy.storchaka'
dependencies = []
files = ['33800', '33801', '33802', '33803', '41380', '41401', '41402', '41430', '41505']
hgrepos = []
issue_num = 20440
keywords = ['patch']
message_count = 35.0
messages = ['209663', '209666', '209669', '209670', '209701', '209713', '209894', '209984', '210447', '210567', '212217', '212219', '212223', '212230', '212246', '212278', '212279', '212283', '212346', '212347', '213980', '213983', '213986', '213989', '213993', '256802', '256956', '256957', '256958', '257069', '257075', '257164', '257527', '257539', '257540']
nosy_count = 15.0
nosy_names = ['loewis', 'brett.cannon', 'georg.brandl', 'rhettinger', 'pitrou', 'kristjan.jonsson', 'vstinner', 'taleinat', 'larry', 'benjamin.peterson', 'Arfrever', 'python-dev', 'Jim.Jewett', 'serhiy.storchaka', 'abusalimov']
pr_nums = []
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue20440'
versions = ['Python 3.6']

Proposed patches replaced idiomatic code

    Py_DECREF(ptr);
    ptr = new_value;

to "Py_REPLACE(ptr, new_value);" which is expanded to

    {
        PyObject *__tmp__ = ptr;
        ptr = new_value;
        Py_DECREF(__tmp__);
    }

(and same for Py_XDECREF -> Py_XREPLACE).

Victor proposed large patch for bpo-16447, but this issue was closed after fixing particular bug. Here are updated patches, which Py_REPLACE/Py_XREPLACE macros for cleaner code. They are also generated automatically by the Coccinelle tool (http://coccinelle.lip6.fr/):

spatch --in-place --sp-file py_replace.spatch --dir .

Patch for every version contains about 50 replaces in about 21-24 files.

Something similar was proposed in bpo-3081.

I do not understand why that issue was closed. Probably Py_SETREF is not the best name but this can be discussed. Adverse idea about Py_INCREF also looked questionable. But many people supported the introduction of a macro for safe replacement.

And now we see that sources contain 50 potential bugs which can be fixed either by using special macros or by manually inlining them.

It all seems like a good idea to me and I like the Py_REPLACE naming (I was going to suggest Py_SWAP, but Py_XSWAP looks too weird to me).

Unless some known bugs are being fixed, this should be 3.4 only.

Unless some known bugs are being fixed, this should be 3.4 only.

For example here is a bug very similar to a bug fixed in bpo-16447.

class Nasty(str):
    def __del__(self):
        C.__qualname__ = "other"

class C:
    pass

C.__qualname__ = Nasty("abc")
C.__qualname__ = "normal"
C.__qualname__ = "normal"

I think Raymond's original concern still applies: The macros do add to the learning curve. I would personally expect that Py_REPLACE(op, op2) does an INCREF on op2, but it does not.

Explicit is better than implicit.

I think Raymond's original concern still applies: The macros do add to the learning curve.

I agree. But alternative solution is Victor's original patch which replaces potential bugs by inlined body of Py_REPLACE/Py_XREPLACE. This is explicit, but more verbose (2 lines are replaced by 5 lines with one new variable, with macros it would be one line), less clear and more errorprone.

I believe that given the popularity of such a code and the possibility of mistakes, it is worth introducing special macros. Here apply the same reasoning as for Py_CLEAR.

Of course these macros shouldn't be a part of stable API in 2.7 and 3.3 (and may be even in 3.4).

These macros work as assignment with builtin decref,
i.e. a smart replacement for =
We could resolve this by calling them Py_ASSIGN Py_XASSIGN
and having complementary macros Py_STORE/Py_XSTORE that will incref the new value.

However, with an added incref, does the X apply to the source or the target?
I wonder if we need the X variants in these macros. Once you are doing things like this, why not just use X implicitly? An extra pointer test or two is unlikely to be a performance problem in the places you might use them.

Anyway, I'll be adding this to the internal api of stackless because it is tremendously useful.

Py_ASSIGN was proposed by Paul Pogonyshev in msg70798, and this also looks good to me.

According to the results of recent poll [1], Py_SETREF is the most popular candidate. It was also one of leaders in previous poll [2], and this is a name used in original Antoine's proposition [3].

[1] http://comments.gmane.org/gmane.comp.python.devel/155654
[2] http://comments.gmane.org/gmane.comp.python.devel/145974
[3] https://mail.python.org/pipermail/python-dev/2008-May/079862.html

Hence there is a patch that introduces and uses the Py_SETREF macro.

New changeset 23296440b654 by Serhiy Storchaka in branch '2.7':
Issue bpo-20440: Massive replacing unsafe attribute setting code with special
https://hg.python.org/cpython/rev/23296440b654

New changeset fd36d72f6030 by Serhiy Storchaka in branch '3.5':
Issue bpo-20440: Massive replacing unsafe attribute setting code with special
https://hg.python.org/cpython/rev/fd36d72f6030

New changeset c4e8751ce637 by Serhiy Storchaka in branch 'default':
Issue bpo-20440: Massive replacing unsafe attribute setting code with special
https://hg.python.org/cpython/rev/c4e8751ce637

Committed patches were generated with attached Coccinelle script.

Following patch is manually crafted and covers the rest cases. It also replaces existing correct attribute replacing using a temporary variable with more compact call of the macro.

New changeset 9fb57c0209ea by Serhiy Storchaka in branch '3.5':
Issue bpo-20440: Applied yet one patch for using Py_SETREF.
https://hg.python.org/cpython/rev/9fb57c0209ea

New changeset bc7c56a225de by Serhiy Storchaka in branch 'default':
Issue bpo-20440: Applied yet one patch for using Py_SETREF.
https://hg.python.org/cpython/rev/bc7c56a225de

New changeset e6502bf289ab by Serhiy Storchaka in branch '2.7':
Issue bpo-20440: Applied yet one patch for using Py_SETREF.
https://hg.python.org/cpython/rev/e6502bf289ab

New changeset 4bfbb2714ae9 by Serhiy Storchaka in branch '3.5':
Issue bpo-20440: More use of Py_SETREF.
https://hg.python.org/cpython/rev/4bfbb2714ae9

New changeset 11670e4be1a9 by Serhiy Storchaka in branch '2.7':
Issue bpo-20440: More use of Py_SETREF.
https://hg.python.org/cpython/rev/11670e4be1a9

New changeset 539ba7267701 by Serhiy Storchaka in branch 'default':
Issue bpo-20440: More use of Py_SETREF.
https://hg.python.org/cpython/rev/539ba7267701

New changeset b02d256b8827 by Serhiy Storchaka in branch 'default':
Issue bpo-20440: Cleaning up the code by using Py_SETREF and Py_CLEAR.
https://hg.python.org/cpython/rev/b02d256b8827

New changeset e04cd497aa41 by Zachary Ware in branch 'default':
Issue bpo-25972, bpo-20440: Fix compilation on Windows
https://hg.python.org/cpython/rev/e04cd497aa41

The final patch replaces the code that equivalent to the Py_SETREF macro by using the Py_SETREF macro. There are no bugs, the patch only makes the correct code cleaner. If I'll not got a review, I'll just close this issue.

New changeset 1118dfcbcc35 by Serhiy Storchaka in branch 'default':
Issue bpo-20440: Cleaning up the code by using Py_SETREF.
https://hg.python.org/cpython/rev/1118dfcbcc35

The commit doesn't include changes for dictobject.c, setobject.c and _sqlite/cache.c (I had forgot to exclude them from the patch before uploading). Dict and set code is performance critical, and using Py_XDECREF instead of Py_DECREF can affect performance. The code in _sqlite would use Py_SETREF in less obvious way, it is better to left it as is.

Navigation Menu

Repository navigation

Use the Py_SETREF macro #64639

16 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Use the Py_SETREF macro #64639

Description

Activity

serhiy-storchaka commented on Jan 29, 2014

pitrou commented on Jan 29, 2014

serhiy-storchaka commented on Jan 29, 2014

brettcannon commented on Jan 29, 2014

rhettinger commented on Jan 30, 2014

serhiy-storchaka commented on Jan 30, 2014

loewis commented on Feb 1, 2014

serhiy-storchaka commented on Feb 2, 2014

kristjanvalur commented on Feb 7, 2014

serhiy-storchaka commented on Feb 7, 2014

16 remaining items

serhiy-storchaka commented on Dec 21, 2015

python-dev commented on Dec 24, 2015

serhiy-storchaka commented on Dec 24, 2015

serhiy-storchaka commented on Dec 24, 2015

python-dev commented on Dec 27, 2015

python-dev commented on Dec 27, 2015

python-dev commented on Dec 29, 2015

serhiy-storchaka commented on Jan 5, 2016

python-dev commented on Jan 5, 2016

serhiy-storchaka commented on Jan 5, 2016

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions