Decoding instructions should handle ENTER_EXECUTOR

[gvanrossum]

There's a variety of places where we walk over an array of instructions, using some variant of

for (int i = 0; i < num_instructions;) {
    int opcode = instructions[i].op.code;
    // <decode EXTENDED_ARG>
    opcode = _PyOpcode_Deopt[opcode];  // or _Py_GetBaseOpcode(code, i)
    // <handle opcode>
    i += 1 + _PyOpcode_Caches[opcode];  // or _PyInstruction_GetLength(code, i)
}

All these make the mistake that if opcode is ENTER_EXECUTOR, it may obscure an underlying JUMP_BACKWARD instruction, which has a cache entry.

I fixed this in gh-107256 for _PyInstruction_GetLength() and it was first reported in gh-107082, but there are a number of other occurrences. Basically every time we consult _PyOpcode_Caches we should ensure that the index is not ENTER_EXECUTOR.

CC: @markshannon

---

Places where I found this:

• _PyInstruction_GetLength
• code_richcompare
• code_hash
• _Py_GetBaseOpcode [should not be "fixed", it would break callers that need the oparg]
• mark_stacks
• _PyFrame_OpAlreadyRan
• de_instrument?
• initialize_tools?
• remove_tools?
• _PyCode_Quicken?

There are possibly others.

Linked PRs

• gh-107265: Fix code_richcompare for ENTER_EXECUTOR case #108165
• gh-107265: Fix code_hash for ENTER_EXECUTOR case #108188
• gh-107265: Ensure de_instrument does not handle ENTER_EXECUTOR #108366
• gh-107265: Ensure _PyCode_Quicken does not handle ENTER_EXECUTOR #108460
• gh-107265: Fix initialize/remove_tools for ENTER_EXECUTOR case #108482
• Revert "gh-107265: Ensure _PyCode_Quicken does not handle ENTER_EXECU… #108485
• gh-107265: Remove all ENTER_EXECUTOR when execute _Py_Instrument #108539
• gh-107265: Add deopt for opcode from executor for future proof #109420
• gh-107265: Fix _PyFrame_OpAlreadyRan for ENTER_EXECUTOR case #111645

[gvanrossum]

Open question. Should ENTER_EXECUTOR be replaced by _Py_GetBaseOpcode() or is that its caller's responsibility? In deopt_code() the caller handles it, but in other cases the caller doesn't, but (probably) should. But you could also argue that the fix is to switch to using _PyInstruction_GetLength() instead of 1 + _PyOpcode_Caches[opcode].

@markshannon Do you have an opinion?

[corona10]

I am working on code_hash too

[markshannon]

Ideally, everything would use the same set of functions to get instruction lengths, opcodes and opargs.

The interaction between instrumentation and ENTER_EXECUTOR is more than just the ENTER_EXECUTOR itself, as we need to invalidate any executor if any of the instructions that it covers is instrumented.

[markshannon]

I think we can remove _PyFrame_OpAlreadyRan altogether. If not now, definitely when PEP 667 is implemented.