[issue17206] Py_XDECREF() expands its argument multiple times

[Mark Shannon]

Change by Mark Shannon :


--
pull_requests: +29209
pull_request: https://github.com/python/cpython/pull/30855

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[SilentGhost]

Changes by SilentGhost :


--
resolution:  -> fixed
status: open -> closed
type:  -> behavior

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Roundup Robot]

Roundup Robot added the comment:

New changeset fcf079dece0d by Victor Stinner in branch 'default':
Issue #17206: On Windows, increase the stack size from 2 MB to 4.2 MB to fix
http://hg.python.org/cpython/rev/fcf079dece0d

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Jeremy Kloth]

Jeremy Kloth added the comment:

The /STACK:reserve[,commit] linker option uses 'reserve' as the upper limit of 
bytes to allow the executable to allocate.  The optional 'commit' value 
(default of 4096) is the amount of physical memory allocated as needed.

Therefore the reserve could be as large as needed to prevent stack overflows 
without impacting the actually memory footprint of the executable.  Maybe we 
should consider increasing the reserve to match that of Linux (8MB)?  From the 
list I posted above, the huge increase in stack use in Python-ast.c could lead 
to other overflows for certain expressions.

Attached is the patch to the project file to increase the stack reserve to 4MB. 
 The actual value doesn't impact the memory requirements of the debug build 
except in edge cases that would otherwise result in stack overflow exceptions.

--
Added file: http://bugs.python.org/file30750/project.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Jeremy Kloth]

Jeremy Kloth added the comment:

I forgot to note that with the increased stack reserve my previous patches to 
Py_DECREF() and related macros are no longer needed.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[STINNER Victor]

STINNER Victor added the comment:

Jeremy: stack for python_d.exe has been already up'ed to 210

Indeed, but not recently (changeset 97361d917d22):

 Issue 2286: bump up the stack size of the 64-bit debug python_d.exe to 
210. The default value of 20 causes a stack overflow at 1965 iterations 
of r_object() in marshal.c, 35 iterations before the 2000 limit enforced by 
MAX_MARSHAL_STACK_DEPTH. 

Oh it looks like it was the same issue: Stack overflow exception caused by 
test_marshal on Windows x64. See issue #2286.

Amaury wrote in this issue (in 2008, msg63540):
Can you try to raise the stack size on x64 builds? If 2Mb is enough for 32bit, 
4Mb should be good in your case.

Jeremy: Therefore the reserve could be as large as needed to prevent stack 
overflows without impacting the actually memory footprint of the executable.  
Maybe we should consider increasing the reserve to match that of Linux (8MB)?

Ah ok, if the memory footprint is unchanged, yes please increase the arbitrary 
limit. I would suggest 8 MB.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[STINNER Victor]

STINNER Victor added the comment:

So it seems that the best option would be to increase the stack size used when 
linking (/STACK:).  I would suggest increasing it to 3MB using /STACK:3145728.

Does Python allocate the whole stack (physical pages) at startup, or is it done 
on demand as on Linux?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Jeremy Kloth]

Jeremy Kloth added the comment:

Here is some additional analysis of recursive functions in the 'pythoncore' 
MSVC project:

Ratio  Release  Debug   Filename:Function Name
   StackStack
---
 1.000   32   32_sre.asm:_validate_inner
 1.159  352  408ast.asm:set_context
 1.167   48   56compile.asm:stackdepth_walk
 1.188  128  152memoryobject.asm:copy_rec
 1.250  160  200ast.asm:num_stmts
 1.308  104  136firstsets.asm:calcfirstset
 1.312  128  168posixmodule.asm:win32_xstat_impl_w
 1.400   40   56node.asm:freechildren
 1.667   72  120memoryobject.asm:tolist_rec
 1.750   32   56listnode.asm:list1node
 1.750   32   56parsermodule.asm:parser_compare_nodes
 1.750   32   56parsermodule.asm:validate_factor
 1.750   32   56parsermodule.asm:validate_not_test
 1.750   32   56rotatingtree.asm:RotatingTree_Enum
 1.750   32   56typeobject.asm:solid_base
 1.786  112  200bytearrayobject.asm:bytearray_setslice
 1.900   80  152compile.asm:compiler_comprehension_generator
 2.143   56  120pythonrun.asm:print_exception_recursive
 2.167   48  104arraymodule.asm:array_ass_slice
 2.250   32   72ast.asm:validate_slice
 2.250   32   72getargs.asm:skipitem
 2.250   32   72node.asm:sizeofchildren
 2.250   32   72parsermodule.asm:validate_test
 2.250   32   72setobject.asm:set_issuperset
 2.278  144  328listobject.asm:list_ass_slice
 2.417   96  232arraymodule.asm:array_ass_subscr
 2.429   56  136longobject.asm:PyLong_AsLongLong
 2.429   56  136parsermodule.asm:node2tuple
 2.429   56  136typeobject.asm:mro_subclasses
 2.500   80  200bytearrayobject.asm:bytearray_ass_subscript
 2.600   40  104errors.asm:PyErr_GivenExceptionMatches
 2.750   32   88import.asm:update_code_filenames
 2.750   32   88setobject.asm:set_richcompare
 2.750   32   88symtable.asm:symtable_visit_slice
 2.750   32   88typeobject.asm:PyType_Modified
 2.766  512 1416marshal.asm:r_object
 3.125   64  200longobject.asm:long_rshift
 3.250   32  104compile.asm:compiler_with
 3.250   32  104setobject.asm:set_issubset
 3.250   32  104typeobject.asm:assign_version_tag
 3.750   32  120abstract.asm:abstract_issubclass
 3.750   32  120typeobject.asm:PyType_Ready
 3.833   48  184ast.asm:ast_for_expr
 4.250   32  136compile.asm:compiler_visit_expr
 4.250   32  136mathmodule.asm:factorial_partial_product
 4.500   80  360genobject.asm:gen_throw
 4.692  312 1464symtable.asm:symtable_visit_stmt
 5.250   32  168_collectionsmodule.asm:deque_extend
 5.250   32  168_collectionsmodule.asm:deque_extendleft
 5.250   32  168ast.asm:alias_for_import_name
 5.750   32  184typeobject.asm:merge_class_dict
 6.250   32  200abstract.asm:PyObject_IsInstance
 6.250   32  200abstract.asm:PyObject_IsSubclass
 6.250   32  200ast.asm:validate_expr
 6.500   48  312Python-ast.asm:obj2ast_slice
 7.182   88  632parsermodule.asm:build_node_children
 7.250   32  232errors.asm:PyErr_NormalizeException
 9.167   48  440symtable.asm:symtable_visit_expr
10.250   32  328_json.asm:encoder_listencode_obj
10.344  256 2648Python-ast.asm:obj2ast_expr
15.955  176 2808Python-ast.asm:obj2ast_stmt
31.750   32 1016Python-ast.asm:ast2obj_expr

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Mark Shannon]

Mark Shannon added the comment:

I think blaming the crash in test_marshall
http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/2009/steps/test/logs/stdio
on the new macros (change aff41a6421c2) is correct.

It may be the proximate cause, but that doesn't make it the underlying one. The 
interpreter should be more resilient against minor changes to the amount of C 
stack used per call.

Can we get a full stack trace (for the C stack not the Python one) for the 
failure?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Roundup Robot]

Roundup Robot added the comment:

New changeset c31bec42e411 by Victor Stinner in branch 'default':
Issue #17206: test.regrtest and test.script_helper enable faulthandler module
http://hg.python.org/cpython/rev/c31bec42e411

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Jeremy Kloth]

Jeremy Kloth added the comment:

I'm unsure as to how to get a stack trace from Visual Studio, but I can assure 
you that the changes introduced by the change aff41a6421c2 *is* the cause of 
the failure in test_marshal.

In a nutshell, the overflow happens in a recursion on the marhsal.c:r_object() 
function for the test_marshal test of BugsTestCase.test_loads_recursion().

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Roundup Robot]

Roundup Robot added the comment:

New changeset 1a9367d5aabc by Victor Stinner in branch 'default':
Issue #17206: Fix test_cmd_line and test_faulthandler for my previous change
http://hg.python.org/cpython/rev/1a9367d5aabc

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[STINNER Victor]

STINNER Victor added the comment:

 Can we get a full stack trace (for the C stack not the Python one) for the 
 failure?

Yes, using a debugger on Windows x64. I'm waiting for a renewal of my MSDN 
account to get Visual Studio (full version, not the Express version limited to 
32-bit).

Until this, I enabled faulthandler in subprocesses created by test.regrtest and 
test.script_helper because it may help this issue, but also others future 
crashes.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[STINNER Victor]

STINNER Victor added the comment:

I enabled faulthandler in subprocesses created by test.regrtest and 
test.script_helper...

Oh, it doesn't help for this issue. On Windows, faulthandler is unable to dump 
the Python traceback on a stack overflow. On UNIX, it allocates a diffrent 
stack for its signal handler, and so the signal handler can run even on a stack 
overflow.

The Windows exit code 0xC0FD (3221225725) means Stack overflow / 
exhaustion.

[ 81/375] test_marshal
Traceback (most recent call last):
  File ../lib/test/regrtest.py, line 1618, in module
main_in_temp_cwd()
  File ../lib/test/regrtest.py, line 1593, in main_in_temp_cwd
main()
  File ../lib/test/regrtest.py, line 775, in main
raise Exception(Child error on {}: {}.format(test, result[1]))
Exception: Child error on test_marshal: Exit code 3221225725

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Jeremy Kloth]

Jeremy Kloth added the comment:

Here are some results after a detailed investigation:

- in debug, each variable declaration in a block is allocated on the stack
- in debug, r_object() uses 1416 bytes of stack 
- in release, r_object() uses 512 bytes of stack
- default stack size is 1MB
- stack for python_d.exe has been already up'ed to 210

So it seems that the best option would be to increase the stack size used when 
linking (/STACK:).  I would suggest increasing it to 3MB using /STACK:3145728.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Jeremy Kloth]

Jeremy Kloth added the comment:

At least in a debug build, the MSVC 64-bit compiler seems to allocate space for 
each unique variable declared in the function body.  Therefore, by changing the 
temporary variables to be named identically, the amount of required space is 
minimized.

The refactoring of Py_DECREF is needed to prevent an error for local variable 
defined before use when Py_DECREF is nested within another macro already 
defining _py_tmp (it would expand to roughly PyObject *_py_tmp = _py_tmp).

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Benjamin Peterson]

Benjamin Peterson added the comment:

We've fixed this debug-problem in other cases simply by skipping the test in 
question on debug builds.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Jeremy Kloth]

Jeremy Kloth added the comment:

Except, in this case, it actually crashes the interpreter.  I would hope to 
think that it isn't common practice to just hide crashers especially when there 
is a fix available.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Benjamin Peterson]

Benjamin Peterson added the comment:

Could you please wrap your new macros in the do .. while boilerplate?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Jeremy Kloth]

Jeremy Kloth added the comment:

FYI, however, the new macro is designed as an internal implementation detail 
along the lines of the other _Py_* macros.  That is, just a tiny piece of a 
larger function to be used at your own risk.

Either way, I've uploaded another version with the do { } while (0) wrapping.

--
Added file: http://bugs.python.org/file30690/object-2.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Benjamin Peterson]

Benjamin Peterson added the comment:

Yes, but in case someone else mindlessly starts using it elsewhere, let's spare 
them the macro bugz.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[STINNER Victor]

STINNER Victor added the comment:

 I've attached an updated patch that reuses the _py_tmp variable for those 
 temporary assignments thus keeping the required stack size down.

I don't understand how it would change the size of the C stack, could you 
please explain? object.patch looks like refactoring to simplify the work of the 
C preprocessor.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Jeremy Kloth]

Jeremy Kloth added the comment:

Indeed, after debugging, it is a stack overflow caused by the introduction of 
the different temporary variables in the Py_XINCREF, Py_XDECREF, and Py_DECREF 
macros.

I've attached an updated patch that reuses the _py_tmp variable for those 
temporary assignments thus keeping the required stack size down.

--
nosy: +jkloth
Added file: http://bugs.python.org/file30625/object.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[STINNER Victor]

STINNER Victor added the comment:

AMD64 Windows7 SP1 3.x buildbot is crashing in test_marshal. It looks like a 
regression introduced by the changeset aff41a6421c2:

[ 88/375] test_marshal
Traceback (most recent call last):
  File ../lib/test/regrtest.py, line 1611, in module
main_in_temp_cwd()
  File ../lib/test/regrtest.py, line 1586, in main_in_temp_cwd
main()
  File ../lib/test/regrtest.py, line 774, in main
raise Exception(Child error on {}: {}.format(test, result[1]))
Exception: Child error on test_marshal: Exit code 3221225725

http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/2009/steps/test/logs/stdio

--
resolution: fixed - 
status: closed - open

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Amaury Forgeot d'Arc]

Amaury Forgeot d'Arc added the comment:

Hum, a stack overflow?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Roundup Robot]

Roundup Robot added the comment:

New changeset aff41a6421c2 by Benjamin Peterson in branch 'default':
don't expand the operand to Py_XINCREF/XDECREF/CLEAR/DECREF multiple times 
(closes #17206)
http://hg.python.org/cpython/rev/aff41a6421c2

--
nosy: +python-dev
resolution:  - fixed
stage:  - committed/rejected
status: open - closed

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Illia Polosukhin]

Illia Polosukhin added the comment:

Amaury, 

   I didn't update Py_INCREF macro in this patch (because it doesn't expand 
it's argument multiple times) - so the examples you are showing will be working 
fine. 

   I've updated Py_XINCREF, but it can't be used as an expression anyway.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Arfrever Frehtes Taifersar Arahesis]

Changes by Arfrever Frehtes Taifersar Arahesis arfrever@gmail.com:


--
nosy: +Arfrever

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Amaury Forgeot d'Arc]

Amaury Forgeot d'Arc added the comment:

The last patch (17206-3.diff) has tests for the 4 macros, and looks good to me.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Amaury Forgeot d'Arc]

Amaury Forgeot d'Arc added the comment:

As I wrote in issue17589, there are some extension modules (pytables) that 
which assume that Py_INCREF is an expression:
  return Py_INCREF(x), x;
and Py_RETURN_NONE is also defined with a comma expression.

Oh, and Cython:
#define __Pyx_PyBool_FromLong(b) ((b) ? (Py_INCREF(Py_True), Py_True) : 
(Py_INCREF(Py_False), Py_False))

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Andrew Svetlov]

Changes by Andrew Svetlov andrew.svet...@gmail.com:


--
nosy: +asvetlov

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Illia Polosukhin]

Illia Polosukhin added the comment:

Fixed Py_DECREF and Py_CLEAR as well. 

Added tests for Py_INCREF and Py_XINCREF (if somebody has a better idea how to 
tests that INCREF doesn't leak - please, let me know).

Removed comment that Py_DECREF evaluate it's argument multiple times as not 
relevant anymore.

About considerations from performance point of view - I've made toy example 
(only this defines and main function) to test how gcc optimizer behaves in 
different cases - from what I see, if expression is like this (which is 
majority of cases in the code):
 PyObject* obj = Foo();
 Py_XDECREF(obj)

assembly code that will be produced (with -O3) is the same before and after 
patch.

--
Added file: http://bugs.python.org/file29660/17206-3.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Mark Dickinson]

Mark Dickinson added the comment:

 Amaury, why do you mention -R flag

He's talking about the -R argument to regrtest:


-R runs each test several times and examines sys.gettotalrefcount() to
see if the test appears to be leaking references.  The argument should
be of the form stab:run:fname where 'stab' is the number of times the
test is run to let gettotalrefcount settle down, 'run' is the number
of times further it is run and 'fname' is the name of the file the
reports are written to.  These parameters all have defaults (5, 4 and
reflog.txt respectively), and the minimal invocation is '-R :'.


--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Mark Dickinson]

Mark Dickinson added the comment:

Thanks for the updated patch.  Some comments:

- In the tests, your functions should have return type PyObject* rather than 
void;  you can use Py_RETURN_NONE as a convenient way to return something of 
the correct type.  E.g.:

static PyObject *
test_decref_doesnt_leak(PyObject* self)
{
Py_DECREF(PyLong_FromLong(0));
Py_RETURN_NONE;
}

- Unfortunately, the test above *will* leak:  even though your patch fixes 
Py_XDECREF, Py_DECREF will still evaluate the argument multiple times.  So we 
get, for example:

iwasawa:cpython mdickinson$ ./python.exe -m test -R:: test_capi
[1/1] test_capi
beginning 9 repetitions
123456789
.
test_capi leaked [1, 1, 1, 1] references, sum=4
1 test failed:
test_capi

- It would be great to have tests for Py_INCREF and Py_XINCREF, too.

- The patch introduces two extra blank lines before Py_CLEAR.

- Not specifically about the patch, but: there's a comment in object.h that 
reads:


*** WARNING*** The Py_DECREF macro must have a side-effect-free argument
since it may evaluate its argument multiple times.  (The alternative
would be to mace it a proper function or assign it to a global temporary
variable first, both of which are slower; and in a multi-threaded
environment the global variable trick is not safe.)


I don't understand why the author of that comment ruled out the possibility of 
a local temporary variable.  Anyone have any ideas?  A constraint of some 
flavours of pre-ANSI C, perhaps?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Mark Shannon]

Mark Shannon added the comment:

Mark, it was written 20 years ago. Who knows (or cares) why it was written that 
way?
Let's just write it correctly this time.


Py_INCREF, Py_DECREF, Py_XDECREF and Py_XINCREF should all expand their 
argument exactly once.

Py_CLEAR should expand its argument exactly twice, once as an rvalue, then once 
as an lvalue.

So could we fix Py_DECREF and Py_CLEAR as well, please?

--
nosy: +Mark.Shannon

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Larry Hastings]

Larry Hastings added the comment:

For the record: I care.  Generally speaking CPython is a lovingly
crafted source tree, and the choices its architects made are nearly
always sensible and well-reasoned.  When I see things like this, things
that seem kind of dumb on first glance, I worry that we're missing
something.

It could be that it did have a sensible reason fifteen years ago, like
some bone-headed compiler generated awful code--and the macros *are*
used everywhere, so maybe performance genuinely suffered.  Or maybe
the macros were originally intended to be usable as rvalues; it looks
like Py_INCREF still permits that, and changing it to be a statement
could be considered a breaking API change.

But we can't let ourselves become hidebound and superstitious.  I see
no reason why a local temporary variable shouldn't be fine here, and
perhaps in all the macros Mark cites.  So don't get me wrong, I'm +1.

(The documentation on Py_INCREF makes no mention of whether it's legal
as an rvalue, and all the examples use it as a statement.  Perhaps we
should document it explicitly so for 3.4.)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Marc-Andre Lemburg]

Marc-Andre Lemburg added the comment:

On 31.03.2013 21:29, Larry Hastings wrote:
 
 Larry Hastings added the comment:
 
 For the record: I care.  Generally speaking CPython is a lovingly
 crafted source tree, and the choices its architects made are nearly
 always sensible and well-reasoned.  When I see things like this, things
 that seem kind of dumb on first glance, I worry that we're missing
 something.

Did someone test the patch on a use case where the performance of
this macro matters a lot ? E.g. deallocating large lists or
dictionaries.

Todays CPUs have plenty of registers available, so it's possible
that the patch actually speeds up things, since the decref'ed
variable may well be an indirect reference. At the time Py_DECREF
was written, registers were precious golden flasks, only to be
filled with the most valuable of things ;-)

Just want to be sure we're not missing something...

--
nosy: +lemburg

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Illia Polosukhin]

Illia Polosukhin added the comment:

Updated patch - removed old code and addded tests to _testcapimoudule.c as 
Amaury suggested.

Amaury, why do you mention -R flag (from what I see it does hash 
randomization)? I would expect some flag, that would enforce memory leakage 
issues to fail test.

--
Added file: http://bugs.python.org/file29617/17206-2.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[STINNER Victor]

STINNER Victor added the comment:

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.

--
nosy: +haypo
versions:  -Python 2.7, Python 3.1, Python 3.2, Python 3.3, Python 3.5

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Amaury Forgeot d'Arc]

Amaury Forgeot d'Arc added the comment:

- 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.

--
nosy: +amaury.forgeotdarc

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Illia Polosukhin]

Illia Polosukhin added the comment:

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

--
Added file: http://bugs.python.org/file29523/perf-linux.log

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Illia Polosukhin]

Illia Polosukhin added the comment:

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.

--
Added file: http://bugs.python.org/file29524/asmdiff_17206.zip

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Illia Polosukhin]

Illia Polosukhin added the comment:

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.

--
keywords: +patch
nosy: +ilblackdragon
Added file: http://bugs.python.org/file29439/17206.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Illia Polosukhin]

Illia Polosukhin added the comment:

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.

--
nosy: +larry

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Illia Polosukhin]

Illia Polosukhin added the comment:

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

--
Added file: http://bugs.python.org/file29440/perf.log

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Illia Polosukhin]

Illia Polosukhin added the comment:

Command used for benchmarking was:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Mark Dickinson]

Mark Dickinson added the comment:

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?

--
nosy: +mark.dickinson

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Illia Polosukhin]

Illia Polosukhin added the comment:

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?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Mark Dickinson]

Mark Dickinson added the comment:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Jesús Cea Avión]

Changes by Jesús Cea Avión j...@jcea.es:


--
nosy: +jcea

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Benjamin Peterson]

Benjamin Peterson added the comment:

I wish we could use inline functions...

--
nosy: +benjamin.peterson

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Dave Malcolm]

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));

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

--
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 rep...@bugs.python.org
http://bugs.python.org/issue17206
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17206] Py_XDECREF() expands its argument multiple times

[Brett Cannon]

Brett Cannon added the comment:

On Thu, Feb 14, 2013 at 11:07 AM, Dave Malcolm rep...@bugs.python.orgwrote:


 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