classification
Title: Use the Py_SETREF macro
Type: crash Stage: resolved
Components: Extension Modules, Interpreter Core Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Arfrever, Jim.Jewett, abusalimov, benjamin.peterson, brett.cannon, georg.brandl, kristjan.jonsson, larry, loewis, pitrou, python-dev, rhettinger, serhiy.storchaka, taleinat, vstinner
Priority: high Keywords: patch

Created on 2014-01-29 18:14 by serhiy.storchaka, last changed 2016-01-05 19:34 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
py_replace.spatch serhiy.storchaka, 2014-01-29 18:14
py_replace-3.4.patch serhiy.storchaka, 2014-01-29 18:14 review
py_replace-3.3.patch serhiy.storchaka, 2014-01-29 18:15 review
py_replace-2.7.patch serhiy.storchaka, 2014-01-29 18:16 review
py_setref.patch serhiy.storchaka, 2015-12-21 14:54 review
py_setref.cocci serhiy.storchaka, 2015-12-24 08:58
py_setref_extra.patch serhiy.storchaka, 2015-12-24 09:04 review
py_setref_extra2.patch serhiy.storchaka, 2015-12-27 11:42 review
py_setref_extra3.patch serhiy.storchaka, 2016-01-05 11:29 review
Messages (35)
msg209663 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-29 18:14
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 issue16447, 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.
msg209666 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-01-29 18:35
Something similar was proposed in issue3081.
msg209669 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-29 19:00
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.
msg209670 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-01-29 20:00
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).
msg209701 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2014-01-30 04:28
Unless some known bugs are being fixed, this should be 3.4 only.
msg209713 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-30 12:58
> 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 issue16447.

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

class C:
    pass

C.__qualname__ = Nasty("abc")
C.__qualname__ = "normal"
C.__qualname__ = "normal"
msg209894 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2014-02-01 16:06
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.
msg209984 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-02 13:53
> 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).
msg210447 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2014-02-07 10:50
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.
msg210567 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-07 21:51
Py_ASSIGN was proposed by Paul Pogonyshev in msg70798, and this also looks good to me.
msg212217 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-02-25 21:44
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.
msg212219 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-25 21:52
> 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.
msg212223 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-02-25 22:00
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.
msg212230 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2014-02-26 00:06
Better yet, embrace c++ and smart pointers :;-)
msg212246 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-26 10:17
Poll: http://comments.gmane.org/gmane.comp.python.devel/145974
msg212278 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2014-02-26 17:29
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.
msg212279 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-02-26 17:52
> 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.
msg212283 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-26 18:23
> 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.
msg212346 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2014-02-27 09:56
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.
msg212347 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2014-02-27 10:07
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...
msg213980 - (view) Author: Jim Jewett (Jim.Jewett) (Python triager) Date: 2014-03-18 15:29
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.
msg213983 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-03-18 15:48
"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.
msg213986 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-03-18 16:39
The patch adds new public APIs (C macros), I don't think it should be committed to the maintenance branches.
msg213989 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-03-18 16:50
Yeah, I'm not accepting this for 3.4 at this point, and I bet the other RMs feel the same way.
msg213993 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-03-18 17:42
Yes, this is new feature territory.
msg256802 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-21 14:54
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.
msg256956 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-12-24 08:40
New changeset 23296440b654 by Serhiy Storchaka in branch '2.7':
Issue #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 #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 #20440: Massive replacing unsafe attribute setting code with special
https://hg.python.org/cpython/rev/c4e8751ce637
msg256957 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-24 08:58
Committed patches were generated with attached Coccinelle script.
msg256958 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-24 09:04
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.
msg257069 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-12-27 10:44
New changeset 9fb57c0209ea by Serhiy Storchaka in branch '3.5':
Issue #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 #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 #20440: Applied yet one patch for using Py_SETREF.
https://hg.python.org/cpython/rev/e6502bf289ab
msg257075 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-12-27 13:52
New changeset 4bfbb2714ae9 by Serhiy Storchaka in branch '3.5':
Issue #20440: More use of Py_SETREF.
https://hg.python.org/cpython/rev/4bfbb2714ae9

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

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

New changeset b02d256b8827 by Serhiy Storchaka in branch 'default':
Issue #20440: Cleaning up the code by using Py_SETREF and Py_CLEAR.
https://hg.python.org/cpython/rev/b02d256b8827
msg257164 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-12-29 05:51
New changeset e04cd497aa41 by Zachary Ware in branch 'default':
Issue #25972, #20440: Fix compilation on Windows
https://hg.python.org/cpython/rev/e04cd497aa41
msg257527 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-01-05 11:29
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.
msg257539 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-01-05 19:28
New changeset 1118dfcbcc35 by Serhiy Storchaka in branch 'default':
Issue #20440: Cleaning up the code by using Py_SETREF.
https://hg.python.org/cpython/rev/1118dfcbcc35
msg257540 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-01-05 19:34
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.
History
Date User Action Args
2016-01-05 19:34:57serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg257540

stage: patch review -> resolved
2016-01-05 19:28:47python-devsetmessages: + msg257539
2016-01-05 11:29:55serhiy.storchakasetfiles: + py_setref_extra3.patch

messages: + msg257527
versions: - Python 2.7, Python 3.5
2015-12-29 05:51:22python-devsetmessages: + msg257164
2015-12-27 13:52:08python-devsetmessages: + msg257075
2015-12-27 11:42:51serhiy.storchakasetfiles: + py_setref_extra2.patch
2015-12-27 10:44:06python-devsetmessages: + msg257069
2015-12-24 09:54:27serhiy.storchakaunlinkissue24103 dependencies
2015-12-24 09:04:11serhiy.storchakasetfiles: + py_setref_extra.patch

messages: + msg256958
2015-12-24 08:58:26serhiy.storchakasetfiles: + py_setref.cocci

messages: + msg256957
2015-12-24 08:40:36python-devsetnosy: + python-dev
messages: + msg256956
2015-12-24 07:17:39serhiy.storchakasettitle: Use Py_REPLACE/Py_XREPLACE macros -> Use the Py_SETREF macro
2015-12-21 14:54:46serhiy.storchakasetfiles: + py_setref.patch
assignee: serhiy.storchaka
messages: + msg256802

versions: + Python 2.7, Python 3.6
2015-12-16 13:20:14serhiy.storchakalinkissue24103 dependencies
2014-10-30 10:40:23abusalimovsetnosy: + abusalimov
2014-10-14 15:17:11skrahsetnosy: - skrah
2014-03-18 17:42:14georg.brandlsetpriority: release blocker -> high

messages: + msg213993
versions: + Python 3.5, - Python 2.7, Python 3.4
2014-03-18 16:50:10larrysetmessages: + msg213989
2014-03-18 16:39:23pitrousetmessages: + msg213986
2014-03-18 15:48:15vstinnersetmessages: + msg213983
versions: - Python 3.3
2014-03-18 15:29:20Jim.Jewettsetpriority: high -> release blocker
nosy: + Jim.Jewett
messages: + msg213980

2014-02-27 10:07:45kristjan.jonssonsetmessages: + msg212347
2014-02-27 09:56:22kristjan.jonssonsetmessages: + msg212346
2014-02-26 18:23:40larrysetmessages: + msg212283
2014-02-26 17:52:38skrahsetmessages: + msg212279
2014-02-26 17:29:12kristjan.jonssonsetmessages: + msg212278
2014-02-26 10:39:43Arfreversetnosy: + Arfrever
2014-02-26 10:17:30serhiy.storchakasetmessages: + msg212246
2014-02-26 00:06:10kristjan.jonssonsetmessages: + msg212230
2014-02-25 22:00:54taleinatsetmessages: + msg212223
2014-02-25 21:52:08serhiy.storchakasetmessages: + msg212219
2014-02-25 21:44:05taleinatsetnosy: + taleinat
messages: + msg212217
2014-02-07 21:51:39serhiy.storchakasetmessages: + msg210567
2014-02-07 10:50:56kristjan.jonssonsetnosy: + kristjan.jonsson
messages: + msg210447
2014-02-02 13:53:28serhiy.storchakasetmessages: + msg209984
2014-02-01 16:06:27loewissetnosy: + loewis
messages: + msg209894
2014-01-30 13:07:43serhiy.storchakasetpriority: normal -> high
type: behavior -> crash
2014-01-30 12:58:59serhiy.storchakasetmessages: + msg209713
2014-01-30 04:28:20rhettingersetnosy: + rhettinger
messages: + msg209701
2014-01-29 20:00:40brett.cannonsetnosy: + brett.cannon
messages: + msg209670
2014-01-29 19:00:04serhiy.storchakasetmessages: + msg209669
2014-01-29 18:35:17pitrousetnosy: + pitrou
messages: + msg209666
2014-01-29 18:16:00serhiy.storchakasetfiles: + py_replace-2.7.patch
2014-01-29 18:15:22serhiy.storchakasetfiles: + py_replace-3.3.patch
2014-01-29 18:14:50serhiy.storchakasetfiles: + py_replace-3.4.patch
keywords: + patch
2014-01-29 18:14:18serhiy.storchakacreate