frame.setlineno has serious flaws.

[markshannon]

The frame_setlineno function works in in stages:

• Determine a set of possible bytecode offsets as targets from the line number.
• Compute the stack state for these targets and the current position
• Determine a best target. That is, the first one that has a compatible stack.
• Pop values form the stack and jump.

The first steps is faulty (I think, I haven't demonstrated this) as it might be possible to jump to an instruction involved in frame creation. This should be easy to fix using the new _co_firsttraceable field.

The second step has (at least) three flaws:

• It does not account for NULLs on the stack, making it possible to jump from a stack with NULLs to one that cannot handle NULLs.
• It does not skip over caches, so could produce incorrect stacks by misinterpreting cache entries as normal instructions.
• It is out of date. For example it thinks that PUSH_EXC_INFO pushes three values. It only pushes one.

Setting the line number of a frame is only possible in the debugger, so this isn't as terrible as might appear, but it definitely needs fixing.

Linked PRs

• gh-94438: Add additional cases to mark_stacks with tests #111237
• [3.12] GH-94438: Restore ability to jump over None tests (GH-111237) #111243
• [3.11] GH-94438: Restore ability to jump over None tests (GH-111237) #111338
• GH-94438: Fix RuntimeWarning for jump tests in test_sys_settrace #111341
• [3.12] GH-94438: Fix RuntimeWarning for jump tests in test_sys_settrace (GH-111341) #111369

[pablogsal]

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 :)

[iritkatriel]

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.

[iritkatriel]

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?

[sweeneyde]

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?

[iritkatriel]

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

[sweeneyde]

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:

[brandtbucher]

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).

[iritkatriel]

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

[brandtbucher]

Yep. See my above comment. :)

[iritkatriel]

ah yeah :)

[gvanrossum]

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

[markshannon]

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