PyCompile_OpcodeStackEffect() and dis.stack_effect() are not particularly useful

[serhiy-storchaka]

BPO	32455
Nosy	@ncoghlan, @vstinner, @larryhastings, @benjaminp, @ericsnowcurrently, @serhiy-storchaka, @godaygo
PRs	bpo-32455: Add jump parameter to dis.stack_effect(). #6610
Dependencies	bpo-24340: co_stacksize estimate can be highly off

^{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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2018-09-18.07:00:26.286>
created_at = <Date 2017-12-30.14:05:46.138>
labels = ['interpreter-core', '3.8', 'type-feature', 'library']
title = 'PyCompile_OpcodeStackEffect() and dis.stack_effect() are not particularly useful'
updated_at = <Date 2018-09-18.07:00:26.285>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2018-09-18.07:00:26.285>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2018-09-18.07:00:26.286>
closer = 'serhiy.storchaka'
components = ['Interpreter Core', 'Library (Lib)']
creation = <Date 2017-12-30.14:05:46.138>
creator = 'serhiy.storchaka'
dependencies = ['24340']
files = []
hgrepos = []
issue_num = 32455
keywords = ['patch']
message_count = 10.0
messages = ['309232', '309427', '309448', '309450', '309452', '309453', '315786', '315812', '315814', '325619']
nosy_count = 7.0
nosy_names = ['ncoghlan', 'vstinner', 'larry', 'benjamin.peterson', 'eric.snow', 'serhiy.storchaka', 'godaygo']
pr_nums = ['6610']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue32455'
versions = ['Python 3.8']

[serhiy-storchaka]

The information provided by PyCompile_OpcodeStackEffect() and dis.stack_effect() (added in bpo-19722) 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.

[serhiy-storchaka]

After resolving bpo-24340 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.

[benjaminp]

What was the rationale for exposing these interfaces in the first place? If they're not useful, I'd rather deprecate and remove them.

[larryhastings]

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?

[serhiy-storchaka]

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.

[larryhastings]

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?

[serhiy-storchaka]

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] MatthieuDartiailh/bytecode#29

[godaygo]

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?

[serhiy-storchaka]

I concur with Kirill and was going to propose this change in a separate issue.

[serhiy-storchaka]

New changeset 7bdf282 by Serhiy Storchaka in branch 'master':
bpo-32455: Add jump parameter to dis.stack_effect(). (GH-6610)
7bdf282