Py_XDECREF() expands its argument multiple times

[davidmalcolm]

BPO	17206
Nosy	@malemburg, @brettcannon, @jcea, @amauryfa, @mdickinson, @vstinner, @larryhastings, @benjaminp, @jkloth, @asvetlov, @davidmalcolm, @markshannon
PRs	bpo-46329: Split calls into precall and call instructions. #30855
Files	17206.diff: Patch for 17206 perf.log: Benchmark comparison for 17206 patch perf-linux.log: Benchmark comparison for 17206 in Ubuntu + GCC asmdiff_17206.zip: Preprocessed, object and disassembled files of ceval module with and without patch. 17206-2.diff: Updated patch for 17206 17206-3.diff object.patch: Patch reusing _py_tmp object-2.patch project.diff

^{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 = None
closed_at = <Date 2016-09-25.15:52:53.332>
created_at = <Date 2013-02-14.16:07:37.057>
labels = ['interpreter-core', 'type-bug']
title = 'Py_XDECREF() expands its argument multiple times'
updated_at = <Date 2022-01-30.17:51:28.343>
user = 'https://github.com/davidmalcolm'

bugs.python.org fields:

activity = <Date 2022-01-30.17:51:28.343>
actor = 'Mark.Shannon'
assignee = 'none'
closed = True
closed_date = <Date 2016-09-25.15:52:53.332>
closer = 'SilentGhost'
components = ['Interpreter Core']
creation = <Date 2013-02-14.16:07:37.057>
creator = 'dmalcolm'
dependencies = []
files = ['29439', '29440', '29523', '29524', '29617', '29660', '30625', '30690', '30750']
hgrepos = []
issue_num = 17206
keywords = ['patch']
message_count = 48.0
messages = ['182107', '182111', '182209', '184430', '184431', '184432', '184433', '184488', '184492', '184495', '184833', '184834', '184855', '185396', '185614', '185623', '185625', '185627', '185643', '185689', '185889', '186386', '189362', '189379', '190156', '190623', '190628', '191364', '191731', '191752', '191780', '191785', '191786', '191791', '191792', '191877', '191878', '191879', '191880', '191881', '191883', '191888', '191919', '192167', '192204', '192205', '192206', '192507']
nosy_count = 15.0
nosy_names = ['lemburg', 'brett.cannon', 'jcea', 'amaury.forgeotdarc', 'mark.dickinson', 'vstinner', 'larry', 'benjamin.peterson', 'jkloth', 'Arfrever', 'asvetlov', 'dmalcolm', 'Mark.Shannon', 'python-dev', 'ilblackdragon']
pr_nums = ['30855']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue17206'
versions = ['Python 3.4']

[davidmalcolm]

When running my refcount static analyzer [1] on a large corpus of real-world C extension code for Python, I ran into the following construct in various places:

    Py_XDECREF(PyObject_CallObject(callable, args));

This is bogus code: Py_XDECREF expands its argument multiple times, so after this goes through the C preprocessor the callable is actually called invoked up to 4 times, leaking refs to 3 of the results - assuming that none of the calls fail, and a non pydebug build of CPython (which would expand the argument more times).

The raw preprocessor output for an optimized Python 2.7 looks like this:
do { if ((PyObject_CallObject(callable, args)) == ((void *)0)) ; else do { if ( --((PyObject*)(PyObject_CallObject(callable, args)))->ob_refcnt != 0) ; else ( ((((PyObject)((PyObject *)(PyObject_CallObject(callable, args))))->ob_type)->tp_dealloc)((PyObject *)((PyObject *)(PyObject_CallObject(callable, args))))); } while (0); } while (0);

Cleaned up (and removing some of the casts for clarity) it looks like this:
  do { 
    if ((PyObject_CallObject(callable, args)) == ((void *)0))
      ; 
    else 
      do {
        if (--(PyObject_CallObject(callable, args)->ob_refcnt) != 0)
          ;
        else
          (*(PyObject_CallObject(callable, args)->ob_type)->tp_dealloc)
            PyObject_CallObject(callable, args);
      } while (0);
  } while (0);
which is clearly not what the extension author was expecting.

Should we:
(A) update the macro so that it expands its argument only once, or
(B) warn people not to write code like the above?

Similar considerations apply to the other macros, I guess, but I've seen the above used "in the wild", I haven't yet seen it for the other macros).

Seen in the wild in:
python-alsa-1.0.25-1.fc17:
pyalsa/alsamixer.c:179
00174 | for (i = 0; i < count; i++) {
00175 | t = PyTuple_New(2);
00176 | if (t) {
00177 | PyTuple_SET_ITEM(t, 0, PyInt_FromLong(pfd[i].fd));
00178 | PyTuple_SET_ITEM(t, 1, PyInt_FromLong(pfd[i].events));
00179>| Py_XDECREF(PyObject_CallObject(reg, t));
00180 | Py_DECREF(t);
00181 | }
00182 | }
00183 |
00184 | Py_XDECREF(reg);

  pyalsa/alsahcontrol.c:190
00185 | 	for (i = 0; i < count; i++) { 
00186 | 		t = PyTuple_New(2); 
00187 | 		if (t) { 
00188 | 			PyTuple_SET_ITEM(t, 0, PyInt_FromLong(pfd[i].fd)); 
00189 | 			PyTuple_SET_ITEM(t, 1, PyInt_FromLong(pfd[i].events)); 
00190>| 			Py_XDECREF(PyObject_CallObject(reg, t)); 
00191 | 			Py_DECREF(t); 
00192 | 		} 
00193 | 	}	 
00194 |  
00195 | 	Py_XDECREF(reg); 

  pyalsa/alsaseq.c:3277
03272 |     for (i = 0; i < count; i++) { 
03273 |         t = PyTuple_New(2); 
03274 |         if (t) { 
03275 |             PyTuple_SET_ITEM(t, 0, PyInt_FromLong(pfd[i].fd)); 
03276 |             PyTuple_SET_ITEM(t, 1, PyInt_FromLong(pfd[i].events)); 
03277>|             Py_XDECREF(PyObject_CallObject(reg, t)); 
03278 |             Py_DECREF(t); 
03279 |         } 
03280 |     } 
03281 |  
03282 |     Py_XDECREF(reg); 

python-4Suite-XML-1.0.2-14.fc17:
Ft/Xml/src/domlette/refcounts.c:80
00075 | /* refcount = this */
00076 | expected = 1;
00077 | }
00078 | else {
00079 | sprintf(buf, "Unexpected object type '%.200s'" ,node->ob_type->tp_name);
00080>| Py_XDECREF(PyObject_CallMethod(tester, "error", "s", buf));
00081 | return 0;
00082 | }
00083 |
00084 | sprintf(buf, "%.200s refcounts", node->ob_type->tp_name);
00085 | return do_test(tester, buf, expected, node->ob_refcnt);

[Note to self: I actually saw this because it uncovered a traceback in cpychecker, which I fixed as:
http://git.fedorahosted.org/cgit/gcc-python-plugin.git/commit/?id=99fa820487e380b74c2eda1d97facdf2ee6d2f3a ]

[1] https://gcc-python-plugin.readthedocs.org/en/latest/cpychecker.html

[brettcannon]

On Thu, Feb 14, 2013 at 11:07 AM, Dave Malcolm <report@bugs.python.org>wrote:

New submission from Dave Malcolm:

When running my refcount static analyzer [1] on a large corpus of
real-world C extension code for Python, I ran into the following construct
in various places:

Py_XDECREF(PyObject_CallObject(callable, args));

Eww.

This is bogus code: Py_XDECREF expands its argument multiple times, so
after this goes through the C preprocessor the callable is actually called
invoked up to 4 times, leaking refs to 3 of the results - assuming that
none of the calls fail, and a non pydebug build of CPython (which would
expand the argument more times).

The raw preprocessor output for an optimized Python 2.7 looks like this:
do { if ((PyObject_CallObject(callable, args)) == ((void *)0)) ; else
do { if ( --((PyObject*)(PyObject_CallObject(callable, args)))->ob_refcnt
!= 0) ; else ( ((((PyObject)((PyObject *)(PyObject_CallObject(callable,
args))))->ob_type)->tp_dealloc)((PyObject *)((PyObject
*)(PyObject_CallObject(callable, args))))); } while (0); } while (0);

Cleaned up (and removing some of the casts for clarity) it looks like this:
do {
if ((PyObject_CallObject(callable, args)) == ((void *)0))
;
else
do {
if (--(PyObject_CallObject(callable, args)->ob_refcnt) != 0)
;
else
(*(PyObject_CallObject(callable, args)->ob_type)->tp_dealloc)
PyObject_CallObject(callable, args);
} while (0);
} while (0);
which is clearly not what the extension author was expecting.

Should we:
(A) update the macro so that it expands its argument only once, or

How expensive is that going to be? Since there is a 'do' statement using a
temp variable would be easy to introduce, but is a compiler going to be
smart enough to inline it all so it doesn't have to waste an allocation,
etc.?

(B) warn people not to write code like the above?

Only if (A) is too costly. I would modify a checkout for ALL refcount
macros and run the benchmark suite to see if there is a noticeable
difference. If there isn't then I say change all the macros (can't make one
safe and the rest not as that is just asking for inconsistent usage and
trouble).

-Brett

Similar considerations apply to the other macros, I guess, but I've seen
the above used "in the wild", I haven't yet seen it for the other macros).

Seen in the wild in:
python-alsa-1.0.25-1.fc17:
pyalsa/alsamixer.c:179
00174 | for (i = 0; i < count; i++) {
00175 | t = PyTuple_New(2);
00176 | if (t) {
00177 | PyTuple_SET_ITEM(t, 0,
PyInt_FromLong(pfd[i].fd));
00178 | PyTuple_SET_ITEM(t, 1,
PyInt_FromLong(pfd[i].events));
00179>| Py_XDECREF(PyObject_CallObject(reg, t));
00180 | Py_DECREF(t);
00181 | }
00182 | }
00183 |
00184 | Py_XDECREF(reg);

  pyalsa/alsahcontrol.c:190
00185 |     for (i = 0; i \< count; i++) {
00186 |             t = PyTuple_New(2);
00187 |             if (t) {
00188 |                     PyTuple_SET_ITEM(t, 0,

PyInt_FromLong(pfd[i].fd));
00189 | PyTuple_SET_ITEM(t, 1,
PyInt_FromLong(pfd[i].events));
00190>| Py_XDECREF(PyObject_CallObject(reg, t));
00191 | Py_DECREF(t);
00192 | }
00193 | }
00194 |
00195 | Py_XDECREF(reg);

  pyalsa/alsaseq.c:3277
03272 |     for (i = 0; i \< count; i++) {
03273 |         t = PyTuple_New(2);
03274 |         if (t) {
03275 |             PyTuple_SET_ITEM(t, 0, PyInt_FromLong(pfd[i].fd));
03276 |             PyTuple_SET_ITEM(t, 1,

PyInt_FromLong(pfd[i].events));
03277>| Py_XDECREF(PyObject_CallObject(reg, t));
03278 | Py_DECREF(t);
03279 | }
03280 | }
03281 |
03282 | Py_XDECREF(reg);

python-4Suite-XML-1.0.2-14.fc17:
Ft/Xml/src/domlette/refcounts.c:80
00075 | /* refcount = this */
00076 | expected = 1;
00077 | }
00078 | else {
00079 | sprintf(buf, "Unexpected object type '%.200s'"
,node->ob_type->tp_name);
00080>| Py_XDECREF(PyObject_CallMethod(tester, "error", "s", buf));
00081 | return 0;
00082 | }
00083 |
00084 | sprintf(buf, "%.200s refcounts", node->ob_type->tp_name);
00085 | return do_test(tester, buf, expected, node->ob_refcnt);

[Note to self: I actually saw this because it uncovered a traceback in
cpychecker, which I fixed as:

http://git.fedorahosted.org/cgit/gcc-python-plugin.git/commit/?id=99fa820487e380b74c2eda1d97facdf2ee6d2f3a]

[1] https://gcc-python-plugin.readthedocs.org/en/latest/cpychecker.html

----------
components: Interpreter Core
messages: 182107
nosy: dmalcolm
priority: normal
severity: normal
status: open
title: Py_XDECREF() expands its argument multiple times
versions: Python 2.7, Python 3.1, Python 3.2, Python 3.3, Python 3.4,
Python 3.5

---

Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue17206\>

---

---

New-bugs-announce mailing list
New-bugs-announce@python.org
http://mail.python.org/mailman/listinfo/new-bugs-announce

[benjaminp]

I wish we could use inline functions...

[ilblackdragon]

I've worked on this with Dave Malcolm @PyCon2013 sprints.

This patch is work in progress to make Py_XDECREF() and Py_XINCREF() expands their arguments once instead of multiple times.

Because patch is work in progress, it contains old version for ease of benchmarking. To enable the old version - define OLD_17206.

Tested so far only with: Clang on Mac OS X 10.7 x64.

[ilblackdragon]

Additionally, in macros Py_XINCREF and Py_XDECREF we've took an opportunity to increase readability by changing expression:

if (item == NULL) ; else action(item);
to more natural inverted condition:
if (item != NULL) action(item);

There is a chance that there may be a reason for this form of expression, so let me know if this is an issue.

[ilblackdragon]

Benchmark run on Clang Mac OS X 10.7 attached of comparison with and without patch 17206.diff.

[ilblackdragon]

Command used for benchmarking was:

python perf.py -b 2n3 -f ../cpython/baseline-clang/python.exe ../cpython/experiment-clang/python.exe | tee perf.log

[mdickinson]

I'd suggest less generic names for the temporary variables, just to minimise the chance of accidental name collisions. _py_xincref_tmp and _py_xdecref_tmp, perhaps?

[ilblackdragon]

That names were my first idea - but then I saw Py_CLEAR uses _py_tmp variable I used it.

Should I replace in Py_CLEAR to _py_clear_tmp as well?

[mdickinson]

Ignore me; I don't know what I was thinking. _py_tmp is fine. There's no issue with name clashes.

[ilblackdragon]

Compiled baseline and patched version with GCC in Ubuntu 12.10 (running in VMWare).

Benchmarking results are attached:
./perf.py -b 2n3 -f ../cpython/baseline/python ../cpython/experiment/python | tee perf-linux.log

[ilblackdragon]

Additionally, disassembled ceval.o and compared baseline with experiment (with applied patch): no actual differences found.

Attached archive contains ceval from both baseline and experiment builds:

• cevalb.cc - processed file of baseline build
• ceval.cc - processed file of experiment build
  diff ceval.cc cevalb.cc - shows differences introduced by patch.
• cevalb.o - object file of baseline build
• ceval.o - object file of experiment build
• cevalb.asm - disassembly of baseline object file
• ceval.asm - disassembly of experiment object file
  diff ceval.asm cevalb.asm - doesn't show any differences in generated assembly code.

[amauryfa]

• The patch contains a "#ifdef OLD_17206" that should be removed.

• I know that these macros are already used everywhere, but a test for the new feature would be nice (in _testcapimodule.c)
  For example, Py_DECREF(PyLong_FromLong(0)) does not fail or leak when tests are run with -R.

[vstinner]

I'm not sure that it is a good idea to patch such major function (macro) in a minor version (ex: Python 2.7.x). I changed the Versions field to only select Python 3.4.