Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
serhiy-storchaka opened this issue Dec 30, 2017 · 10 comments
Assignees
Labels
3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

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
    Copy link
    Member Author

    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 serhiy-storchaka added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Dec 30, 2017
    @serhiy-storchaka
    Copy link
    Member Author

    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.

    @serhiy-storchaka serhiy-storchaka self-assigned this Jan 3, 2018
    @benjaminp
    Copy link
    Contributor

    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
    Copy link
    Contributor

    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
    Copy link
    Member Author

    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.

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

    @larryhastings
    Copy link
    Contributor

    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.

    1. 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
    Copy link
    Member Author

    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

    @serhiy-storchaka serhiy-storchaka added 3.8 only security fixes and removed 3.7 (EOL) end of life labels Apr 26, 2018
    @godaygo
    Copy link
    Mannequin

    godaygo mannequin commented Apr 26, 2018

    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
    Copy link
    Member Author

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

    @serhiy-storchaka
    Copy link
    Member Author

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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants