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

Expose stack effect calculator to Python #63921

Closed
larryhastings opened this issue Nov 22, 2013 · 11 comments
Closed

Expose stack effect calculator to Python #63921

larryhastings opened this issue Nov 22, 2013 · 11 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@larryhastings
Copy link
Contributor

BPO 19722
Nosy @brettcannon, @ncoghlan, @larryhastings, @ericsnowcurrently
Files
  • larry.expose.stack.effect.patch.1.diff
  • larry.expose.stack.effect.patch.2.diff
  • larry.expose.stack.effect.patch.3.diff
  • 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/larryhastings'
    closed_at = <Date 2013-11-23.22:50:27.780>
    created_at = <Date 2013-11-22.19:42:03.240>
    labels = ['type-feature', 'library']
    title = 'Expose stack effect calculator to Python'
    updated_at = <Date 2014-03-10.01:35:03.426>
    user = 'https://github.com/larryhastings'

    bugs.python.org fields:

    activity = <Date 2014-03-10.01:35:03.426>
    actor = 'python-dev'
    assignee = 'larry'
    closed = True
    closed_date = <Date 2013-11-23.22:50:27.780>
    closer = 'larry'
    components = ['Library (Lib)']
    creation = <Date 2013-11-22.19:42:03.240>
    creator = 'larry'
    dependencies = []
    files = ['32781', '32782', '32813']
    hgrepos = []
    issue_num = 19722
    keywords = ['patch']
    message_count = 11.0
    messages = ['203845', '203847', '203856', '203857', '203922', '203931', '203937', '204028', '204113', '204122', '213006']
    nosy_count = 5.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'larry', 'python-dev', 'eric.snow']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue19722'
    versions = ['Python 3.4']

    @larryhastings
    Copy link
    Contributor Author

    Attached is a patch exposing the old opcode_stack_effect() function to Python. The patch does the following:

    • renames opcode_stack_effect() to PyCompile_OpcodeStackEffect()
    • removes the "static" modifier from PyCompile_OpcodeStackEffect()
    • changes PyCompile_OpcodeStackEffect()'s behavior so it returns a magic
      value on failure
    • preserves existing behavior when compiling code and encountering
      an opcode/oparg pair that results in failure
    • creates a new _opcode module
    • exposes PyCompile_OpcodeStackEffect() as _opcode.stack_effect()
    • tests _opcode module with new test__opcode.py
    • imports _opcode.stack_effect() into opcode, exposing it publically
    • documents the function in dis (there is no documentation for opcode,
      and dis imports and exposes everything in opcode)

    Whew! I think it's ready to go in.

    @larryhastings larryhastings self-assigned this Nov 22, 2013
    @larryhastings larryhastings added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Nov 22, 2013
    @larryhastings
    Copy link
    Contributor Author

    (Sponging around for a reviewer ;-)

    @ericsnowcurrently
    Copy link
    Member

    FWIW, I agree with Antoine about making those PyCompile_ functions private (leading "_").

    @larryhastings
    Copy link
    Contributor Author

    New patch, incorporating Antoine's comments. Thanks, Antoine!

    @ncoghlan
    Copy link
    Contributor

    +1 from me. A stack_effect attribute on dis.Instruction would be a nice
    bonus, but doesn't need to be part of this patch.

    @ncoghlan
    Copy link
    Contributor

    Hmm, looking at dis.py, I'm -1 on exposing this as a public opcode module API at this point, although I'm still a fan of exposing it as opcode._stack_effect to allow advanced users access (ala sys._get_frames).

    I initially thought the required addition to dis.Instruction would just be:

        @property
        def stack_effect(self):
            return opcode.stack_effect(self.opcode, self.arg)

    However, that doesn't necessarily work, since self.arg may be None.

    That means stack_effect has to be at least:

        def stack_effect(opcode, oparg=None):
            if oparg is None:
                if opcode >= HAVE_ARGUMENT:
                    raise ValueError("This opcode needs an argument")
                oparg = 0
            return _opcode.stack_effect(opcode, oparg)

    However, even that's not quite accurate, since if the previous opcode was EXTENDED_ARG, you should be adding *that* arg times 65536 to oparg in order to figure out the stack effect.

    It's that need to take the previous opcode into account to correctly determine the value for "oparg" that makes this a bit tricky.

    Although, I guess the latter concern would only apply to integration into the dis module - for the opcode module, it just needs to be documented that the calculation of the passed in oparg value should take EXTENDED_ARG into account.

    @larryhastings
    Copy link
    Contributor Author

    stack_effect will never know about EXTENDED_ARG. Instead, you simply pass the already-extended arg as the second argument.

    Making the second argument support a default of None will be slightly inconvenient, but I'll do it if you think it's a big improvement.

    Considering how troublesome it was to recreate this information when I wrote my assembler, I definitely think this information should be exported out of the murky heart of Python.

    @ncoghlan
    Copy link
    Contributor

    Yeah, I argued myself into realising EXTENDED_ARG just needed to be
    mentioned in the function docs, but didn't go back and fix my opening
    paragraph.

    The fact dis uses an arg of None (rather than zero) to indicate "no
    argument" means I think the extra layer of wrapping in the pure Python
    module is needed prior to 3.4 rc1, but we can live without it for beta 1.

    @larryhastings
    Copy link
    Contributor Author

    Attached is revision 3 of the patch. I'm gonna check it in pretty soon, so as to make beta (and feature freeze). I changed the API so the oparg is optional, and it raises if it gets one it shouldn't have or didn't get one when it should.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 23, 2013

    New changeset 5fe72b9ed48e by Larry Hastings in branch 'default':
    Issue bpo-19722: Added opcode.stack_effect(), which accurately
    http://hg.python.org/cpython/rev/5fe72b9ed48e

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 10, 2014

    New changeset 4a801f8b7e2d by R David Murray in branch 'default':
    whatsnew: dis.stack_effect (bpo-19722).
    http://hg.python.org/cpython/rev/4a801f8b7e2d

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