classification
Title: PyCompile_OpcodeStackEffect() and dis.stack_effect() are not particularly useful
Type: enhancement Stage: resolved
Components: Interpreter Core, Library (Lib) Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: 24340 Superseder:
Assigned To: serhiy.storchaka Nosy List: benjamin.peterson, eric.snow, godaygo, larry, ncoghlan, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2017-12-30 14:05 by serhiy.storchaka, last changed 2018-09-18 07:00 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 6610 merged serhiy.storchaka, 2018-04-26 11:34
Messages (10)
msg309232 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-12-30 14:05
The information provided by PyCompile_OpcodeStackEffect() and dis.stack_effect() (added in issue19722) is not enough for practical use.

1. Some opcodes (like JUMP_IF_TRUE_OR_POP or FOR_ITER) have different stack effect when when execution is continued from the following opcode or jumped to the address specified by oparg. Thus there are two different values for stack effect of these opcodes.

2. Some opcodes (RETURN_VALUE, RAISE_VARARGS) stops execution. Their stack effect doesn't have meaning, and the following opcodes should be ignored.

3. Some opcodes (JUMP_ABSOLUTE, JUMP_RELATIVE) always jump. The opcodes following them should be ignored.

4. Some opcodes (like SETUP_FINALLY or SETUP_WITH) need additional space on the stack for raised exceptions. Their stack effect is virtual. Its value can't be used for calculating the actual position on the stack. This isn't documented.

The possible solution is to add a boolean flag for distinguishing the stack effects in case of consequent execution and jumping. Return a special sentinel or raise an exception if the opcode doesn't pass execution in this direction.
msg309427 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-01-03 18:19
After resolving issue24340 I'm going to add PyCompile_OpcodeStackEffect() that takes an additional integer argument, and add an optional tristate argument to dis.stack_effect(). If it is True, return the effect when jump, if it is false, return the effect when not jump, when omitted or None, return the maximal effect.
msg309448 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2018-01-03 23:31
What was the rationale for exposing these interfaces in the first place? If they're not useful, I'd rather deprecate and remove them.
msg309450 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2018-01-04 00:14
The rationale: without this information, it is impossible for anybody else to write a bytecode compiler / assembler, because when you create a code object you have to specify its stack depth.  I used this information for my "maynard" bytecode assembler / disassembler.

That said, I'm not sure who needs these super-fancy versions Serhiy is proposing.  To calculate stack depth, all you really need is the *maximum* stack depth per instruction.  You might slightly over-allocate but it shouldn't really be much of a problem.

Serhiy: what's your use case for all these complicated new features?
msg309452 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-01-04 00:57
1. This will actually simplify the code for calculating the stack size. Instead of supporting special cases for jumping instructions in stackdepth_walk() you could just write something like

new_depth = depth + PyCompile_OpcodeStackEffectEx(opcode, oparg, 0);
if (new_depth > maxdepth)
    maxdepth = new_depth;
if (isjump) {
    target_depth = depth + PyCompile_OpcodeStackEffectEx(opcode, oparg, 1);
    maxdepth = stackdepth_walk(c, instr->i_target, target_depth, maxdepth);
}
depth = new_depth;

After adding new opcodes or changing existing opcodes you would need to change the code only in one place.

2. This will simplify third-party compilers (like https://github.com/vstinner/bytecode). They wouldn't need to duplicate the complicated code.
msg309453 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2018-01-04 01:31
> 1. This will actually simplify the code for calculating the stack size.

Simplify whose code, the caller or the callee?  It seems like it's simplifying the life of the callee (PyCompile_OpcodeStackEffectEx) at the expense of pushing complexity into the caller.


> 2. This will simplify third-party compilers (like
> https://github.com/vstinner/bytecode). They wouldn't need to
> duplicate the complicated code.

What complicated code?  Compilers simply call PyCompile_OpcodeStackEffect(), passing in an opcode + oparg pair, and it simply returns the maximum stack effect.  No crazy "is it a jump" logic needed.  Again: this may mean overallocating the stack.  But CPython has behaved like this for twenty years and I'm not convinced it's a problem that needs solving.


Victor, do you want to use Serhiy's proposed API in your library?
msg315786 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-04-26 11:45
In 3.7 the stack effect is calculated more accurately by the compiler. PR 6610 exposes this feature to users. It add jump parameter to dis.stack_effect() and to PyCompile_OpcodeStackEffect(). By default the maximal value is returned. Passing jump=True for a non-jumping code or passing jump=False to RETURN_VALUE or JUMP_ABSOLUTE don't raise an exception and don't return a special value. It is up to the caller to pass reliable arguments.

This will make the workaround for stack_effect() in the bytecode project [1] not needed in 3.8.

[1] https://github.com/vstinner/bytecode/pull/29
msg315812 - (view) Author: Kirill Balunov (godaygo) Date: 2018-04-26 19:06
Sorry if this doesn't fit this issue and needs a separate one.

Since Python switched to 2 byte wordcode, all opcodes which do not imply an argument, technically have it - augmented with 0. So it is convenient to iterate over bytecode like:

op, arg = instruction.

But there is a check in stack_effect that the second argument for this opcodes must be None. 

file::_opcode.c

else if (oparg != Py_None) {
        PyErr_SetString(PyExc_ValueError,
                "stack_effect: opcode does not permit oparg but oparg was specified");
        return -1;
    }


So you need to perform a somewhat _redundant_ check before calling:

arg = arg if op >= opcode.HAVE_ARGUMENT else None.
st = stack_effect(op, arg)

Maybe it's normal to relax this condition - be None or 0 for opcode < opcode.HAVE_ARGUMENT?
msg315814 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-04-26 19:30
I concur with Kirill and was going to propose this change in a separate issue.
msg325619 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-18 06:54
New changeset 7bdf28265aa371b39f82dfc6562635801aff15a5 by Serhiy Storchaka in branch 'master':
bpo-32455: Add jump parameter to dis.stack_effect(). (GH-6610)
https://github.com/python/cpython/commit/7bdf28265aa371b39f82dfc6562635801aff15a5
History
Date User Action Args
2018-09-18 07:00:26serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-09-18 06:54:29serhiy.storchakasetmessages: + msg325619
2018-04-26 19:30:00serhiy.storchakasetmessages: + msg315814
2018-04-26 19:06:50godaygosetnosy: + godaygo
messages: + msg315812
2018-04-26 11:46:01serhiy.storchakasetversions: + Python 3.8, - Python 3.7
2018-04-26 11:45:49serhiy.storchakasetmessages: + msg315786
2018-04-26 11:34:57serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request6306
2018-01-04 01:31:26larrysetmessages: + msg309453
2018-01-04 01:24:02larrysetnosy: + vstinner
2018-01-04 00:57:30serhiy.storchakasetmessages: + msg309452
2018-01-04 00:14:04larrysetmessages: + msg309450
2018-01-03 23:31:52benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg309448
2018-01-03 18:19:49serhiy.storchakasetassignee: serhiy.storchaka
2018-01-03 18:19:40serhiy.storchakasetdependencies: + co_stacksize estimate can be highly off
messages: + msg309427
2017-12-30 14:05:46serhiy.storchakacreate