Moving this back to release blocker because apparently, this could end in many changes.

I am missing some context here on what this is affecting so I changed it from deferred blocker to release blocker if we think we can delay this to 3.12, please, say so :)

It is out of date. For example it thinks that PUSH_EXC_INFO pushes three values. It only pushes one.

After failing to write a test that will crash on this, I analysed the code and realised that the "exception handling opcode" cases of the switch are no longer reachable - there is nothing that will initialise their stack[] entries, so they get skipped in the continue; before the switch.

I created a PR to use my favourite macro in these cases: #94582

We could backport it, but we don't have to.

It does not skip over caches, so could produce incorrect stacks by misinterpreting cache entries as normal instructions.

I'm assuming this refers to the mark_stacks loop again, which is where stacks are produced.

The good news is that it looks like it just happens to work by accident. There is no case in the switch for the CACHE opcode, so it goes to the default case. Since the stack_effect of CACHE is 0, it just copies stack[i] to stack[i+1] unchanged. So the stack that is supposed to be on the next instruction will propagate as is until the first non-cache entry.

We could make it more explicit like this:

diff --git a/Objects/frameobject.c b/Objects/frameobject.c
index 34a6c46c6b..ccdf7de990 100644
--- a/Objects/frameobject.c
+++ b/Objects/frameobject.c
@@ -220,6 +220,11 @@ mark_stacks(PyCodeObject *code_obj, int len)
             }
             opcode = _Py_OPCODE(code[i]);
             switch (opcode) {
+                case CACHE:
+                {
+                    stacks[i+1] = stacks[i];
+                    break;
+                }
                 case JUMP_IF_FALSE_OR_POP:
                 case JUMP_IF_TRUE_OR_POP:
                 case POP_JUMP_FORWARD_IF_FALSE:

thoughts?

There is no case in the switch for the CACHE opcode, so it goes to the default case.

Don't cache entries get populated at specialization time with data that overwrites the CACHE opcode? I don't think we're keeping around 0s in the bytecode that aren't subject to change. If I'm understanding right, the problem comes from misinterpreting that specialized cache data as if it's some bytecode that has a stack effect.

It looks like the dis module looks up _inline_cache_entries[deop] to determine how many cache entries to skip over. Would that work here?

Ah I see! Would you like to try? (I'm signing off for the night soon.)

I don't think I'm going to have time in the next couple of days to come up with a decent test case and PR and everything, but I am thinking that something vaguely like this could work:

@@ -213,12 +213,15 @@ mark_stacks(PyCodeObject *code_obj, int len)
     int todo = 1;
     while (todo) {
         todo = 0;
-        for (i = 0; i < len; i++) {
+        int ncaches;
+        for (i = 0; i < len; i += (1 + ncaches)) {
+            ncaches = 0;
             int64_t next_stack = stacks[i];
             if (next_stack == UNINITIALIZED) {
                 continue;
             }
-            opcode = _Py_OPCODE(code[i]);
+            opcode = _PyOpcode_Deopt[_Py_OPCODE(code[i])];
+            ncaches = _PyOpcode_Caches[opcode];
             switch (opcode) {
                 case JUMP_IF_FALSE_OR_POP:
                 case JUMP_IF_TRUE_OR_POP:

It does not skip over caches, so could produce incorrect stacks by misinterpreting cache entries as normal instructions

@iritkatriel is correct... nothing needs to be done about this. mark_stacks uses _PyCode_GetCode, which produces a "deoptimized" byte string equivalent to .co_code attribute access. Otherwise, we would need cases in this switch for all of the adaptive forms of these instructions.

So I think this issue can be closed?

Does it make sense to add some test that exercises this function on highly specialised/warmed up code? I think all the tests just run some function a small number of times.

Ah, my mistake, I missed the _PyCode_GetCode.

Well, the function is designed to totally ignore warmed-up code and just use the deoptimized string. So new tests would keep us from accidentally using it in the future, but I'm pretty sure other stuff here would start breaking much sooner (tons of instructions would be hitting the default case and would end up having a stack effect of PY_INVALID_STACK_EFFECT, which is INT_MAX).

Perhaps it would just be better to assert(delta != PY_INVALID_STACK_EFFECT)?

Let’s close then.

Reopening because extended arguments aren't handled correctly. :(

Removing release blocker status since all known crashes are fixed.

I'm still going to leave this open, thoough, since the four "is None" jumps aren't handled yet (which just means that we aren't able to jump to some valid locations).

I just noticed that the switch in mark_stacks doesn't have cases for POP_JUMP_IF_NONE/POP_JUMP_IF_NOT_NONE.

Yep. See my above comment. :)

ah yeah :)

Shall we nevertheless close this, and open a more specific issue?

Yes.
Most, if not all, of the flaws I listed have been fixed.