homepage

This issue tracker has been migrated to GitHub , and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

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 2022年04月11日 14:57 by admin. 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
2022年04月11日 14:57:57adminsetgithub: 64639
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

AltStyle によって変換されたページ (->オリジナル) /