gh-143820: ENTER_EXECUTOR can be inserted after BINARY_OP_INPLACE_ADD_UNICODE

[Fidget-Spinner]

• Issue: JIT: Assertion next_instr->op.code == STORE_FAST failed in _PyEval_EvalFrameDefault #143820

[markshannon]

Provided that _BINARY_OP_INPLACE_ADD_UNICODE leaves the VM in a valid state (and I believe it does), then it doesn't matter if the following STORE_FAST is replaced by ENTER_EXECUTOR or instrumented.

It is the assert that needs changing. You'll need to use _Py_GetBaseCodeUnit

[bedevere-app]

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

[Fidget-Spinner]

It is the assert that needs changing. You'll need to use _Py_GetBaseCodeUnit

It matters because we get the next instruction's oparg and check it next_oparg = next_instr->op.arg;. ENTER_EXECUTOR overrides the oparg. If we use _Py_GetBaseCodeUnit, it will significantly slow down the current code, so I choose to deopt instead.

[markshannon]

We need to check for ENTER_EXECUTOR either way, so we might as well handle that case without deopting and losing the big-O improvement.

next_oparg = next_instr->op.arg;
if (next_instr->op.code == ENTER_EXECUTOR) {
    _PyExecutorObject *exec = _PyFrame_GetCode(frame)->co_executors->executors[next_oparg];
    next_oparg = exec->vm_data.oparg;
}
// Updated assert goes here

[markshannon]

The test case seems to have a lot of extra stuff in it, but no check that it is testing what it is supposed to be testing.

Can we tidy it up and add a check that it it working?

[markshannon]

There seems like a lot of extraneous stuff in this test, and no check that the STORE_FAST is overwritten by an ENTER_EXECUTOR.

[markshannon]

This is wrong as it will fail if the code is instrumented. You'll need to use _Py_GetBaseCodeUnit()