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.

[taleinat]

While we're bikeshedding, how about the more verbose PY_DECREF_AND_ASSIGN? That makes it clearer that an INCREF is not done.

Regarding Kristján's suggestion of PY_ASSIGN and a complementary PY_STORE, IMO these names are too similar and the difference between them is not clear.

[serhiy-storchaka]

While we're bikeshedding, how about the more verbose PY_DECREF_AND_ASSIGN? That makes it clearer that an INCREF is not done.

Py_ASSIGN_AND_DECREF would be more correct. And Py_CLEAR can be renamed to Py_CLEAR_AND_XDECREF or Py_ASSIGN_NULL_AND_XDECREF.

[taleinat]

PY_ASSIGN_AND_DECREF could seem to imply that the assigned value is DECREF-ed. I think PY_DECREF_AND_ASSIGN makes it clearer that the original value is DECREF-ed.

I like PY_ASSIGN_NULL_AND_DECREF, though for the same reason as above, I'd name it PY_DECREF_AND_ASSIGN_NULL.

[kristjanvalur]

Better yet, embrace c++ and smart pointers :;-)

[serhiy-storchaka]

Poll: http://comments.gmane.org/gmane.comp.python.devel/145974

[kristjanvalur]

Barring c++, are we using any C compilers that don't support inlines?
Imho these macros should be functions proper. Then we could do
Py_Assign(&target, Py_IncRef(obj))

It's 2014 already.

[skrah]

Barring c++, are we using any C compilers that don't support inlines?

Not that I know of. libmpdec is C99, which seems to be supported by all
obscure commercial compilers on snakebite.

Also there have been no 3.x bug reports due to compilers choking on inline
functions.

[larryhastings]

Barring c++, are we using any C compilers that don't support inlines?

CPython advertises itself as C89 compliant, and C89 doesn't have inlines. You need to go to C99 to get inlines.

And before you ask--yes, we support a compiler that is not C99 compliant: Microsoft Visual C++. I'm pretty sure it does have inline support though.

It's possible that every platform officially supported by CPython has a C compiler that supports inlines. I'm pretty sure people compile Python on unsupported platforms whose compilers don't have inlines (e.g. OS/2). Anyway, you'd have to get Guido to agree to breaking C89 compatibility, it's not something you could do locally on this patch without (most likely) a big drawn-out discussion on python-dev.

[kristjanvalur]

Are you referring to the Py_LOCAL_INLINE macro?
I see that we have no Py_INLINE. Py_LOCAL_INLINE includes the "static" qualifier, and in fact, if there is no "USE_INLINE" defined, then all that it does is to add "static".

Would having a "Py_INLINE(type)" macro, that is the same, but without the static (except when USE_INLINE is false) make a difference? It would be a bit odd to have Py_LOCAL_INLINE() functions defined in the headers.

I'm not sure that there is any practical difference between "static inline" and "inline". But there is a difference between "static" and "inline".

It would be great if we could start writing stuff like the Py_INCREF() and Py_DECREF() as functions rather than macros, but for this to happen we must be able to trust that they are really inlined.

[kristjanvalur]

Well, Larry, I certainly am in no mood to start wrangling on python-dev. A 25 year old C standard is likely to be very mature and reliable by now. Why take risks? :)

#Py_LOCAL_INLINE exists and demonstrates that we can make use of them when possible.
We could create a #Py_INLINE macro that would work the same, only not necessarily yield inline on some of the older compilers.

It would really be healthy for the pyton code base, for quality, for semantics, and for the learning curve, if we could start to rely less on macros in the core.

Ah well, perhaps I'll throw this out there...

[jimjjewett]

I am changing this from "High" to "Release blocker", because I think 3.3 should make an explicit decision about whether to remove itself from the Affected Versions list.

3.4 probably should too, since it is now in bug-fix mode.

Then this issue can go back to whatever level is otherwise appropriate.

[vstinner]

"I am changing this from "High" to "Release blocker", because I think 3.3 should make an explicit decision about whether to remove itself from the Affected Versions list."

I don't understand. Which release does it block? There is no scheduled release in short term. Python 3.3 now only accept security changes, and so it's too late for such change.

[pitrou]

The patch adds new public APIs (C macros), I don't think it should be committed to the maintenance branches.

[larryhastings]

Yeah, I'm not accepting this for 3.4 at this point, and I bet the other RMs feel the same way.

[birkenfeld]

Yes, this is new feature territory.

[serhiy-storchaka]

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.

[python-dev]

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

[serhiy-storchaka]

Committed patches were generated with attached Coccinelle script.

[serhiy-storchaka]

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.

[python-dev]

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

[python-dev]

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

[python-dev]

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

[serhiy-storchaka]

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.

[python-dev]

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

[serhiy-storchaka]

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.