Use the Py_SETREF macro

[serhiy-storchaka]

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']

[serhiy-storchaka]

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.

[pitrou]

Something similar was proposed in bpo-3081.

[serhiy-storchaka]

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.

[brettcannon]

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).

[rhettinger]

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

[serhiy-storchaka]

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"

[loewis]

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.

[serhiy-storchaka]

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).

[kristjanvalur]

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.

[serhiy-storchaka]

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