[issue17206] Py_XDECREF() expands its argument multiple times

Change by Mark Shannon : -- pull_requests: +29209 pull_request: https://github.com/python/cpython/pull/30855 ___ Python tracker ___

[issue17206] Py_XDECREF() expands its argument multiple times

Changes by SilentGhost : -- resolution: -> fixed status: open -> closed type: -> behavior ___ Python tracker ___ ___ Python-bugs-lis

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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 Linu

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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 f

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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 t

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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(

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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 underl

[issue17206] Py_XDECREF() expands its argument multiple times

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 ___ _

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

Benjamin Peterson added the comment: Could you please wrap your new macros in the do .. while boilerplate? -- ___ Python tracker ___ _

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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 ___ _

[issue17206] Py_XDECREF() expands its argument multiple times

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 refactori

[issue17206] Py_XDECREF() expands its argument multiple times

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 ref

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

Amaury Forgeot d'Arc added the comment: Hum, a stack overflow? -- ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe

[issue17206] Py_XDECREF() expands its argument multiple times

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 main_in_temp_cwd()

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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 ___

[issue17206] Py_XDECREF() expands its argument multiple times

Changes by Arfrever Frehtes Taifersar Arahesis : -- nosy: +Arfrever ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscri

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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)

[issue17206] Py_XDECREF() expands its argument multiple times

Changes by Andrew Svetlov : -- nosy: +asvetlov ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.pyth

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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.

[issue17206] Py_XDECREF() expands its argument multiple times

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'r

[issue17206] Py_XDECREF() expands its argument multiple times

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 exa

[issue17206] Py_XDECREF() expands its argument multiple times

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_does

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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 -

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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 ___

[issue17206] Py_XDECREF() expands its argument multiple times

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 __

[issue17206] Py_XDECREF() expands its argument multiple times

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

[issue17206] Py_XDECREF() expands its argument multiple times

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 ___

[issue17206] Py_XDECREF() expands its argument multiple times

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 __

[issue17206] Py_XDECREF() expands its argument multiple times

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 t

[issue17206] Py_XDECREF() expands its argument multiple times

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 ben

[issue17206] Py_XDECREF() expands its argument multiple times

Benjamin Peterson added the comment: I wish we could use inline functions... -- nosy: +benjamin.peterson ___ Python tracker ___ ___ Py

[issue17206] Py_XDECREF() expands its argument multiple times

Changes by Jesús Cea Avión : -- nosy: +jcea ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.

[issue17206] Py_XDECREF() expands its argument multiple times

Brett Cannon added the comment: On Thu, Feb 14, 2013 at 11:07 AM, Dave Malcolm 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: > >

[issue17206] Py_XDECREF() expands its argument multiple times

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 argu