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

Bugs of the new AST compiler #42508

Closed
arigo mannequin opened this issue Oct 21, 2005 · 27 comments
Closed

Bugs of the new AST compiler #42508

arigo mannequin opened this issue Oct 21, 2005 · 27 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@arigo
Copy link
Mannequin

arigo mannequin commented Oct 21, 2005

BPO 1333982
Nosy @mwhudson, @brettcannon, @arigo, @nascheme, @rhettinger
Files
  • load-pop.diff: fix load const/pop top in compile.c
  • LOAD_CONST_POP_TOP.diff
  • unary_minus_fix.txt
  • 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 = None
    closed_at = <Date 2006-08-08.09:22:52.000>
    created_at = <Date 2005-10-21.10:08:25.000>
    labels = ['interpreter-core']
    title = 'Bugs of the new AST compiler'
    updated_at = <Date 2006-08-08.09:22:52.000>
    user = 'https://github.com/arigo'

    bugs.python.org fields:

    activity = <Date 2006-08-08.09:22:52.000>
    actor = 'arigo'
    assignee = 'nnorwitz'
    closed = True
    closed_date = None
    closer = None
    components = ['Interpreter Core']
    creation = <Date 2005-10-21.10:08:25.000>
    creator = 'arigo'
    dependencies = []
    files = ['1822', '1823', '1824']
    hgrepos = []
    issue_num = 1333982
    keywords = []
    message_count = 27.0
    messages = ['26655', '26656', '26657', '26658', '26659', '26660', '26661', '26662', '26663', '26664', '26665', '26666', '26667', '26668', '26669', '26670', '26671', '26672', '26673', '26674', '26675', '26676', '26677', '26678', '26679', '26680', '26681']
    nosy_count = 6.0
    nosy_names = ['mwh', 'nnorwitz', 'brett.cannon', 'arigo', 'nascheme', 'rhettinger']
    pr_nums = []
    priority = 'critical'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue1333982'
    versions = []

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Oct 21, 2005

    The newly merged AST branch is likely to expose
    a number of small problems before it stabilizes,
    so here is a tentative bug tracker entry to
    collect such small problems.

    @arigo arigo mannequin closed this as completed Oct 21, 2005
    @arigo arigo mannequin assigned nnorwitz Oct 21, 2005
    @arigo arigo mannequin added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Oct 21, 2005
    @arigo arigo mannequin closed this as completed Oct 21, 2005
    @arigo arigo mannequin assigned nnorwitz Oct 21, 2005
    @arigo arigo mannequin added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Oct 21, 2005
    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Oct 21, 2005

    Logged In: YES
    user_id=4771

    A scoping problem (comparing with the old compiler):

    class X:
        print hello

    The variable 'hello' is incorrectly looked up with
    LOAD_GLOBAL instead of LOAD_NAME. It causes a crash
    in PyPy in a case where the name 'hello' is stored
    into the class implicitely (via locals()). It can
    probably be discussed if the bug is in PyPy, but it
    is a difference in behavior.

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Oct 21, 2005

    Logged In: YES
    user_id=4771

    import a as b, c

    the 'c' part gets completely forgotten and there is
    no 'IMPORT_NAME c' in the bytecode.

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Oct 21, 2005

    Logged In: YES
    user_id=4771

    any reason why lambda functions have a __name__ of
    'lambda' now instead of '<lambda>' ?

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Oct 21, 2005

    Logged In: YES
    user_id=4771

    For reference, an optimization that got lost:

        def f():
            'a'
            'b'

    'a' is the docstring, but the 'b' previously did not show
    up anywhere in the code object. Now there is the
    LOAD_CONST/POP_TOP pair.

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Oct 21, 2005

    Logged In: YES
    user_id=4771

    The Python rules about which names get mangled are a bit
    insane. I share mwh's view that mangling should never have
    been invented in the first place, but well:

    >>> def f():
    ...   __x = 5
    ...   class X:
    ...     def g(self):
    ...       return __x
    ...   return X
    ... 
    Fatal Python error: unknown scope for _X__x in X(135832776)
    in <stdin>

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Oct 21, 2005

    Logged In: YES
    user_id=4771

    I would suspect the following one to be due to incorrect
    handling of EXTENDED_ARG -- it's from a PyPy test about that:

    longexpr = 'x = x or ' + '-x' * 2500
    code = '''
    def f(x):
        %s
        %s
        %s
        %s
        %s
        %s
        %s
        %s
        %s
        %s
        while x:
            x -= 1
            # EXTENDED_ARG/JUMP_ABSOLUTE here
        return x
    ''' % ((longexpr,)*10)

    exec code
    f(5)
    SystemError: unknown opcode

    dis.dis() shows that the target of both the SETUP_LOOP and
    the JUMP_IF_FALSE at the start of the loop are wrong.

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Oct 21, 2005

    Logged In: YES
    user_id=4771

    The following (similarly strange-looking) code snippets
    compiled successfully before, now they give SyntaxErrors:

    --------------------

    def f():
        class g:
            exec "hi"
            x
    --------------------
    def f(x):
        class g:
            exec "hi"
            x
    --------------------
    def f():
        class g:
            from a import *
            x
    --------------------
    def f(x):
        class g:
            from a import *
            x

    @nnorwitz
    Copy link
    Mannequin

    nnorwitz mannequin commented Oct 22, 2005

    Logged In: YES
    user_id=33168

    I assigned to Jeremy and Neil in the hopes they will see
    this message and know about these problems.

    @rhettinger
    Copy link
    Contributor

    Logged In: YES
    user_id=80475

    FWIW, here are the warnings issued by my compiler:

    Python-ast.c
    C:\py25\Python\Python-ast.c(1995) : warning C4101: 'i' :
    unreferenced local variable
    C:\py25\Python\Python-ast.c(2070) : warning C4101: 'i' :
    unreferenced local variable
    C:\py25\Python\Python-ast.c(2085) : warning C4101: 'i' :
    unreferenced local variable
    C:\py25\Python\Python-ast.c(2130) : warning C4101: 'i' :
    unreferenced local variable
    C:\py25\Python\Python-ast.c(2151) : warning C4101: 'i' :
    unreferenced local variable
    C:\py25\Python\Python-ast.c(2261) : warning C4101: 'i' :
    unreferenced local variable
    C:\py25\Python\Python-ast.c(2270) : warning C4101: 'i' :
    unreferenced local variable

    compile.c
    C:\py25\Python\compile.c(3782) : warning C4305: '=' : truncation
    from 'const int ' to 'char '
    C:\py25\Python\compile.c(3802) : warning C4305: '=' : truncation
    from 'const int ' to 'char '
    C:\py25\Python\compile.c(3806) : warning C4305: '=' : truncation
    from 'const int ' to 'char '

    @nnorwitz
    Copy link
    Mannequin

    nnorwitz mannequin commented Oct 23, 2005

    Logged In: YES
    user_id=33168

    Checkpoint of outstanding issues. I think all the others
    mentioned so far have been fixed:

    • Raymond's warnings in compile.c (unused locals are fixed)
    • EXTENDED_ARG problem
    • LOAD_CONST/POP_TOP (note we can fix this in the optimizer
      generally which would get rid of other useless code too)

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Dec 4, 2005

    Logged In: YES
    user_id=4771

    At rev 41584, the following code snippet triggers an assert
    if --with-pydebug is enabled:
    Python/compile.c:3843: assemble_lnotab: Assertion 'd_lineno >= 0' failed

    -----------------------
    assert 1, ([s for s in x] +
    [s for s in x])
    pass
    -----------------------

    @brettcannon
    Copy link
    Member

    Logged In: YES
    user_id=357491

    I just checked Armin's problem code on rev. 41638 and it
    worked for me in the interpreter. You still having
    problems, Armin?

    @mwhudson
    Copy link

    Logged In: YES
    user_id=6656

    You have to include those lines in a source file. It still crashes for me.

    @nnorwitz
    Copy link
    Mannequin

    nnorwitz mannequin commented Dec 18, 2005

    Logged In: YES
    user_id=33168

    EXTENDED_ARG problem was fixed a while ago.
    The assert/pass problem was fixed with: 41756.

    That leaves the LOAD_CONST/POP_TOP optimization that was
    lost and one compiler warning: marshal_write_mod() not being
    used.

    @mwhudson
    Copy link

    mwhudson commented Feb 9, 2006

    Logged In: YES
    user_id=6656

    We found another one. Something is wrong in the compilation of augmented
    assignment to subscriptions containing tuples; running this code:

    class C:
        def __setitem__(self, i, v):
            print i, v
        def __getitem__(self, i):
            print i
            return 0
    
    c = C()
    c[4,5] += 1

    gives a spurious exception:

    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: object does not support item assignment

    By contrast, "c[(4,5)] += 1" works fine.

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Feb 12, 2006

    Logged In: YES
    user_id=4771

    Subscripting is generally a bit sloppy: e.g. the AST model has
    no way to distinguish between a single value and a one-element
    tuple value! See:

    >>> d = {}
    >>> d[1,] = 6
    >>> d
    {1: 6}        # !

    I suggest we fix the model to turn the 'subs' of the 'Subscript' node
    from a list of nodes to a single, mandatory 'sub' node. If tupling is
    necessary, it can be explicitly represented with a 'sub' containing a
    'Tuple' node.

    @nnorwitz
    Copy link
    Mannequin

    nnorwitz mannequin commented Feb 18, 2006

    Logged In: YES
    user_id=33168

    Jeremy, there's no need to read anything before my last
    comment at 2005-12-17 23:10. The last two by Armin,
    Michael, then my last comment are the only important ones.
    Everything that occurred before my 2005-12-17 comment was
    taken care of AFAIK.

    @nnorwitz
    Copy link
    Mannequin

    nnorwitz mannequin commented Apr 3, 2006

    Logged In: YES
    user_id=33168

    The tuple store problem is fixed. The only outstanding item
    is the LOAD_CONST/POP_TOP. I will fix that soon.

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Apr 11, 2006

    Logged In: YES
    user_id=4771

    Another one: the literal -2147483648 (i.e. the value of
    -sys.maxint-1) gives a long in 2.5, but an int in <= 2.4.

    @mwhudson
    Copy link

    Logged In: YES
    user_id=6656

    Good morning Armin!

    I've reported that bug already: http://python.org/sf/1441486
    There's a patch which purports to fix it: http://python.org/sf/1446922
    but I haven't gotten around to testing it.

    (this is running the pypy/module/array tests or something, isn't it?)

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Jul 2, 2006

    Logged In: YES
    user_id=4771

    Attached a patch for the LOAD_CONST POP_TOP optimization
    (modified from Georg Brandl on python-dev).

    @nascheme
    Copy link
    Member

    nascheme commented Jul 9, 2006

    Logged In: YES
    user_id=35752

    Regarding the -2147483648 bug: the old compiler tries to
    fold unary +, - and ~ if the RHS is a constant. I think
    only the minus case is important since the others are just
    optimizations, right? Attaching what seems to be a minimum fix.

    @nascheme
    Copy link
    Member

    nascheme commented Jul 9, 2006

    Logged In: YES
    user_id=35752

    Unary minus bug has been fixed in SVN rev 50495.

    @nnorwitz
    Copy link
    Mannequin

    nnorwitz mannequin commented Aug 3, 2006

    Logged In: YES
    user_id=33168

    Fix pending. need tests and code to be thawed.

    @nnorwitz
    Copy link
    Mannequin

    nnorwitz mannequin commented Aug 4, 2006

    Logged In: YES
    user_id=33168

    I believe this addresses all the bugs. If you find new
    bugs, please open a new report so it's easier to track.

    Committed revision 51081.

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Aug 8, 2006

    Logged In: YES
    user_id=4771

    Yes, I re-checked and all the bugs listed here are fixed.

    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants