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?

I am working on code_hash too

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.

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

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

But PEP 667 seems in limbo (and rather ambitious, so I wouldn't want to wait for it if we can drop _PyFrame_OpAlreadyRan).

I don't understand at all what _PyFrame_OpAlreadyRan does or why, or if there's a test for it. I added

assert(check_opcode != ENTER_EXECUTOR);

to it and am now running the test suite with -Xuops always on; we'll see in about an hour how that goes. (Hum, well, if at least one standard CI job runs runs with assertions enabled.)

So the entire test suite run with -Xuops hard-coded and a hard-coded check in _PyFrame_OpAlreadyRan that check_opcode != ENTER_EXECUTOR found nothing, so maybe there's some good reason why that function isn't a problem. In any case it's not a priority.

Guido, IIUC, we don't need to fix de_instrument.
Because de_instrument is only called from removetools and it requires that opcode should have event.
I will send a PR to add one line of assert statement for the context.

Please let me know if I miss something.

IIUC, we don't need to handle ENTER_EXECUTOR from _PyCode_Quicken too.
AFAIK, the role of _PyCode_Quicken is marking the initialization stat for specialization.
From the execution view, now the ENTER_EXECUTOR will be decoded from the tier2 interpreter execution only,
so we don't have to validate accurate stat for specialization from tier 2.

Please let me know if I miss something :)

Yeah, _PyCode_Quicken is called when initializing a fresh code object, there won't be any ENTER_EXECUTOR instructions there. Maybe just add an assert that opcode != ENTER_EXECUTOR?

Maybe just add an assert that opcode != ENTER_EXECUTOR?

Sent #108460 and merged :)

initialize_tools needs actual fix.
JUMP_BACKWARD should be handled appropriately from the following code line, but it looks not.

I was able to reproduce the situation by executing the following command.

./python.exe -Xuops -m test test_monitoring

Assertion failed: (opcode != ENTER_EXECUTOR), function initialize_tools, file instrumentation.c, line 1298.
Fatal Python error: Aborted

And plus
#107265 (comment) was wrong,
since the remove_tools should decode ENTER_EXECUTOR to JUMP_BACKWARD, there needs some fix too

Working from the other end, in main the following two commands crash:

(.venv) ~/cpython$  ./python.exe -Xuops -m test test_import
0:00:00 load avg: 3.18 Run tests sequentially
0:00:00 load avg: 3.18 [1/1] test_import
Assertion failed: (opcode != ENTER_EXECUTOR), function _PyCode_Quicken, file specialize.c, line 304.
Fatal Python error: Aborted

Current thread 0x00007ff851463640 (most recent call first):
  <no Python frame>

Extension modules: _testinternalcapi, _testmultiphase, _xxsubinterpreters, _testcapi (total: 4)
Abort trap: 6
(.venv) ~/cpython$ 

and

(.venv) ~/cpython$  ./python.exe -Xuops -m test test_bdb
0:00:00 load avg: 3.42 Run tests sequentially
0:00:00 load avg: 3.42 [1/1] test_bdb
Assertion failed: (is_version_up_to_date(code, tstate->interp)), function _Py_call_instrumentation_line, file instrumentation.c, line 1117.
Fatal Python error: Aborted
[...]

I think both are related to ENTER_EXECUTOR.

About _PyFrame_OpAlreadyRan we don't have to do anything if it is removed: faster-cpython/ideas#623 (comment)

PEP 667 landed for 3.13, and a code search indicates _PyFrame_OpAlreadyRan is gone, so is there anything left to resolve for this issue?

(This issue came up while doing a search for "PEP 667" to check for issues that could be closed now that has been implemented)