classification
Title: Replace two Py_XDECREFs with Py_DECREFs in do_raise
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: python-dev, rhettinger, serhiy.storchaka, xiang.zhang, ztane
Priority: normal Keywords: patch

Created on 2016-08-07 12:51 by xiang.zhang, last changed 2016-09-27 08:39 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
do_raise.patch xiang.zhang, 2016-08-07 12:51 review
do_raise_v2.patch xiang.zhang, 2016-08-08 04:47 add asserts review
Messages (8)
msg272119 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-07 12:51
In do_raise, the two Py_XDECREFs at the final of the success branch seem doing unnecessary NULL checks.
msg272134 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-08-07 22:28
Have you checked all the possibly code paths to be sure?   "Seems to be unnecessary" is insufficient reason for undoing Guido's code that has stood since 1997.   


Looking at the snarl of possible code paths, I not finding it obvious why some of the paths require Py_XDECREF and others don't.   Even if you are sure, then the patch needs to include assertions at key checkpoints (i.e. after a given assignment to "type" or "value" can we reliably assert the value is non-null) and/or comments showing what the reasoning is.  Otherwise, even if the patch happens to be correct, it increases the risk that future maintenance will introduce a bug when the current code would have allowed the maintenance in a safer context.
msg272143 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-08 03:05
Thanks for your reply Raymond. You always provide thoughtful feedbacks, though it makes me somewhat more nervous. ;) (I am nervous about making noise here and waste others' time.)

> Have you checked all the possibly code paths to be sure?

Yes. I did check all the code paths. I don't want to be a noise maker so I did check it carefully.

>  "Seems to be unnecessary" is insufficient reason for undoing Guido's code that has stood since 1997.

I use the word "seems" because even if I think my opinion is right, I am only 99% sure. Just as you said, the code is legacy and changing it has to be careful. I can miss something and be wrong. But now I think I am right.

> Looking at the snarl of possible code paths, I not finding it obvious why some of the paths require Py_XDECREF and others don't.

When we reach the two Py_XDECREFs, can type and value be NULL?

> then the patch needs to include assertions at key checkpoints (i.e. after a given assignment to "type" or "value" can we reliably assert the value is non-null) and/or comments showing what the reasoning is

Sorry, I didn't realize that. This is definitely good practice I know. But this is not documented and not a must then so I didn't do that. But I'll remember this in later patches.
msg272145 - (view) Author: Antti Haapala (ztane) * Date: 2016-08-08 06:20
Normally I wouldn't recommend changing working code. However those asserts would be OK; if either of them is NULL, then the previous if would have had undefined behaviour already. Thus the `XDECREF` wrongly signals that it'd be OK if they were NULLs until this point, which is not true.

I'd rather see more asserts in the code; would be a big aid in possible refactoring; now for example `PyErr_SetObject` checks twice and thrice if either of the arguments is NULL; would be nice to go see the call site and see asserts in place there, showing that the arguments never were NULL to begin with.
msg272148 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-08 06:50
Thanks for the comment, Antti.

>  would be nice to go see the call site and see asserts in place there, showing that the arguments never were NULL to begin with.

So is it still not enough getting the two asserts? IMHO I think the two asserts have already meet your expectations since the cause block is impossible to make type or value NULL.
msg272213 - (view) Author: Antti Haapala (ztane) * Date: 2016-08-09 04:05
No, I was just trying to explain why your change could be considered beneficial.
msg272226 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-09 09:40
do_raise_v2.patch (with asserts) LGTM. There are two ways that lead to this point (and third way leads to raising an exception), and additional assertions make the code clearer, because a reader shouldn't follow all these patch for reading the code after assertions.
msg277506 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-27 08:38
New changeset 9e59cb403efa by Serhiy Storchaka in branch 'default':
Issue #27703: Got rid of unnecessary NULL checks in do_raise() in release mode.
https://hg.python.org/cpython/rev/9e59cb403efa
History
Date User Action Args
2016-09-27 08:39:00serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: resolved
2016-09-27 08:38:23python-devsetnosy: + python-dev
messages: + msg277506
2016-08-09 09:40:35serhiy.storchakasetmessages: + msg272226
2016-08-09 04:05:03ztanesetmessages: + msg272213
2016-08-08 06:50:14xiang.zhangsetmessages: + msg272148
2016-08-08 06:20:08ztanesetnosy: + ztane
messages: + msg272145
2016-08-08 04:47:09xiang.zhangsetfiles: + do_raise_v2.patch
2016-08-08 03:05:07xiang.zhangsetmessages: + msg272143
2016-08-07 22:28:31rhettingersetnosy: + rhettinger
messages: + msg272134
2016-08-07 12:51:57xiang.zhangcreate