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

ceval traces code differently with USE_COMPUTED_GOTOS #85836

Closed
nedbat opened this issue Aug 30, 2020 · 5 comments
Closed

ceval traces code differently with USE_COMPUTED_GOTOS #85836

nedbat opened this issue Aug 30, 2020 · 5 comments
Labels
3.8 only security fixes 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@nedbat
Copy link
Member

nedbat commented Aug 30, 2020

BPO 41670
Nosy @nedbat, @markshannon, @ammaraskar, @pablogsal
PRs
  • bpo-41670: Remove outdated predict macro invocation. #22026
  • 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 2021-04-02.15:50:43.892>
    created_at = <Date 2020-08-30.22:49:31.082>
    labels = ['interpreter-core', 'type-bug', '3.8', '3.9']
    title = 'ceval traces code differently with USE_COMPUTED_GOTOS'
    updated_at = <Date 2021-04-02.15:50:43.892>
    user = 'https://github.com/nedbat'

    bugs.python.org fields:

    activity = <Date 2021-04-02.15:50:43.892>
    actor = 'ammar2'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-04-02.15:50:43.892>
    closer = 'ammar2'
    components = ['Interpreter Core']
    creation = <Date 2020-08-30.22:49:31.082>
    creator = 'nedbat'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 41670
    keywords = ['patch', '3.8regression']
    message_count = 5.0
    messages = ['376135', '376137', '376140', '376146', '377659']
    nosy_count = 4.0
    nosy_names = ['nedbat', 'Mark.Shannon', 'ammar2', 'pablogsal']
    pr_nums = ['22026']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue41670'
    versions = ['Python 3.8', 'Python 3.9']

    @nedbat
    Copy link
    Member Author

    nedbat commented Aug 30, 2020

    Coverage.py bug reports nedbat/coveragepy#1022 and nedbat/coveragepy#959 demonstrate the same Python code, with the same disassembly, executing differently.

    In https://discuss.python.org/t/same-python-version-different-optimizations-on-different-os/5098, Ammar Askar said:

    For any core developer who wants to look into this, based on my preliminary research this seems to be related to opcode prediction and computed GOTOS.

    If you put #define USE_COMPUTED_GOTOS 0 above https://github.com/python/cpython/blob/master/Python/ceval.c#L1033 then this issue is re-creatable on Linux/Mac.

    It seems to be an issue relating to how f_lasti is updated.

    @nedbat nedbat added 3.8 only security fixes 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Aug 30, 2020
    @ammaraskar
    Copy link
    Member

    minimal reproducer without coverage:

    import sys
    
    def f():
        try:
            for i in []: pass
            return 1
        except:
            return 2
    
    def tracer(frame, event, _):
        if event == 'line':
            print("executing line {}".format(frame.f_lineno))
        return tracer
    
    sys.settrace(tracer)
    f()

    With computed gotos this results in:

    executing line 4
    executing line 5
    executing line 6

    but without:

    executing line 4
    executing line 5

    @ammaraskar
    Copy link
    Member

    So I think this is a weird edge case with a set of opcode predictions (GET_ITER -> FOR_ITER -> POP_BLOCK) going outside of a line boundary.

    The disassembly of the reproducer above is:

    4 0 SETUP_FINALLY 16 (to 18)

    5 2 LOAD_CONST 1 (())
    4 GET_ITER
    >> 6 FOR_ITER 4 (to 12)
    8 STORE_FAST 0 (i)
    10 JUMP_ABSOLUTE 6

    6 >> 12 POP_BLOCK
    14 LOAD_CONST 2 (1)
    16 RETURN_VALUE

    When computed gotos are disabled and there is a tracing function, instructions 0, 2, 4, 14 and 16 hit the fast_next_opcode label and have a chance to be traced as line hits. Note that maybe_call_line_trace will only cause a line hit if the instruction that starts a line (POP_BLOCK in this case) is being executed.

    When computed gotos are enabled, DISPATCH is a no-op and there is a special case for when tracing is enabled that causes every opcode to go through fast_next_opcode:

    cpython/Python/ceval.c

    Lines 1054 to 1059 in c3a651a

    if (!_Py_TracingPossible(ceval2) && !PyDTrace_LINE_ENABLED()) { \
    f->f_lasti = INSTR_OFFSET(); \
    NEXTOPARG(); \
    goto *opcode_targets[opcode]; \
    } \
    goto fast_next_opcode; \

    When computed gotos are not enabled, there is no similar check for PREDICT (and might be too costly to add) causing this issue:

    cpython/Python/ceval.c

    Lines 1131 to 1141 in c3a651a

    #define PREDICT(op) \
    do { \
    _Py_CODEUNIT word = *next_instr; \
    opcode = _Py_OPCODE(word); \
    if (opcode == op) { \
    oparg = _Py_OPARG(word); \
    next_instr++; \
    goto PREDICT_ID(op); \
    } \
    } while(0)
    #endif

    @ammaraskar ammaraskar changed the title Windows and Linux execute the same code differently ceval traces code differently based with USE_COMPUTED_GOTOS Aug 31, 2020
    @ammaraskar ammaraskar changed the title Windows and Linux execute the same code differently ceval traces code differently based with USE_COMPUTED_GOTOS Aug 31, 2020
    @ammaraskar ammaraskar changed the title ceval traces code differently based with USE_COMPUTED_GOTOS ceval traces code differently with USE_COMPUTED_GOTOS Aug 31, 2020
    @ammaraskar ammaraskar changed the title ceval traces code differently based with USE_COMPUTED_GOTOS ceval traces code differently with USE_COMPUTED_GOTOS Aug 31, 2020
    @markshannon
    Copy link
    Member

    A couple of things to fix here.

    Firstly, the PREDICTion of POP_BLOCK in FOR_ITER shouldn't be there. POP_BLOCK doesn't normally occur after a loop and hasn't since we removed "pseudo exceptions" from the interpreter a couple of years ago.

    Secondly, there is the issue of PREDICTs skipping tracing.
    Either we can make sure that no PREDICTs cross a line boundary, which seems error prone, or we add the check for tracing into the PREDICT macro, which seems more robust.

    @markshannon
    Copy link
    Member

    New changeset 17b5be0 by Mark Shannon in branch 'master':
    bpo-41670: Remove outdated predict macro invocation. (GH-22026)
    17b5be0

    @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 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants