Repository: python/cpython
Pull request: gh-112519: Make it possible to specify instruction flags for pseudo instructions in bytecodes.c
Fixes #112519.

<!-- gh-issue-number: gh-112519 -->
* Issue: gh-112519
<!-- /gh-issue-number -->


Problem statement:
Make it possible to specify flags for pseudo instructions defined in bytecodes.c

The instruction metadata is incorrect for some of the pseudo instructions. For instance, these map to ``NOP``, and take their metadata from it, but they should have the ``HAS_ARG`` flag set.

```
  [SETUP_FINALLY] = { true, 0, 0 },
  [SETUP_CLEANUP] = { true, 0, 0 },
  [SETUP_WITH] = { true, 0, 0 },
```

We need to be able to specify flags for pseudo instructions in bytecodes.c (and relax assertions that it is identical to the target flags).



<!-- gh-linked-prs -->
### Linked PRs
* gh-112520
<!-- /gh-linked-prs -->

Hints from issue discussion:

Commit message: remove obsolete comment

Patch to review:
diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h
index 4e45725d393479..4ae15e71e8d318 100644
--- a/Include/internal/pycore_opcode_metadata.h
+++ b/Include/internal/pycore_opcode_metadata.h
@@ -1661,7 +1661,7 @@ const struct opcode_metadata _PyOpcode_opcode_metadata[OPCODE_METADATA_SIZE] = {
     [IMPORT_FROM] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_NAME_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG },
     [JUMP_FORWARD] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_JUMP_FLAG },
     [JUMP_BACKWARD] = { true, INSTR_FMT_IBC, HAS_ARG_FLAG | HAS_JUMP_FLAG | HAS_EVAL_BREAK_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG },
-    [JUMP] = { true, 0, HAS_ARG_FLAG | HAS_JUMP_FLAG },
+    [JUMP] = { true, 0, HAS_ARG_FLAG | HAS_JUMP_FLAG | HAS_EVAL_BREAK_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG },
     [JUMP_NO_INTERRUPT] = { true, 0, HAS_ARG_FLAG | HAS_JUMP_FLAG },
     [ENTER_EXECUTOR] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_JUMP_FLAG | HAS_EVAL_BREAK_FLAG | HAS_ERROR_FLAG },
     [_POP_JUMP_IF_FALSE] = { true, INSTR_FMT_IBC, HAS_ARG_FLAG | HAS_JUMP_FLAG },
@@ -1703,9 +1703,9 @@ const struct opcode_metadata _PyOpcode_opcode_metadata[OPCODE_METADATA_SIZE] = {
     [BEFORE_ASYNC_WITH] = { true, INSTR_FMT_IX, HAS_ERROR_FLAG | HAS_ESCAPES_FLAG },
     [BEFORE_WITH] = { true, INSTR_FMT_IX, HAS_ERROR_FLAG | HAS_ESCAPES_FLAG },
     [WITH_EXCEPT_START] = { true, INSTR_FMT_IX, HAS_ERROR_FLAG | HAS_ESCAPES_FLAG },
-    [SETUP_FINALLY] = { true, 0, 0 },
-    [SETUP_CLEANUP] = { true, 0, 0 },
-    [SETUP_WITH] = { true, 0, 0 },
+    [SETUP_FINALLY] = { true, 0, HAS_ARG_FLAG },
+    [SETUP_CLEANUP] = { true, 0, HAS_ARG_FLAG },
+    [SETUP_WITH] = { true, 0, HAS_ARG_FLAG },
     [POP_BLOCK] = { true, 0, 0 },
     [PUSH_EXC_INFO] = { true, INSTR_FMT_IX, 0 },
     [_GUARD_DORV_VALUES_INST_ATTR_FROM_DICT] = { true, INSTR_FMT_IX, HAS_DEOPT_FLAG },
diff --git a/Lib/test/test_generated_cases.py b/Lib/test/test_generated_cases.py
index 98a8fff4268746..de96a8764594ba 100644
--- a/Lib/test/test_generated_cases.py
+++ b/Lib/test/test_generated_cases.py
@@ -466,6 +466,44 @@ def test_macro_instruction(self):
     """
         self.run_cases_test(input, output)
 
+    def test_pseudo_instruction_no_flags(self):
+        input = """
+        pseudo(OP) = {
+            OP1,
+        };
+
+        inst(OP1, (--)) {
+        }
+    """
+        output = """
+        TARGET(OP1) {
+            frame->instr_ptr = next_instr;
+            next_instr += 1;
+            INSTRUCTION_STATS(OP1);
+            DISPATCH();
+        }
+    """
+        self.run_cases_test(input, output)
+
+    def test_pseudo_instruction_with_flags(self):
+        input = """
+        pseudo(OP, (HAS_ARG, HAS_JUMP)) = {
+            OP1,
+        };
+
+        inst(OP1, (--)) {
+        }
+    """
+        output = """
+        TARGET(OP1) {
+            frame->instr_ptr = next_instr;
+            next_instr += 1;
+            INSTRUCTION_STATS(OP1);
+            DISPATCH();
+        }
+    """
+        self.run_cases_test(input, output)
+
     def test_array_input(self):
         input = """
         inst(OP, (below, values[oparg*2], above --)) {
diff --git a/Python/bytecodes.c b/Python/bytecodes.c
index a1ca66e457e47d..21d53aeb97de19 100644
--- a/Python/bytecodes.c
+++ b/Python/bytecodes.c
@@ -2831,15 +2831,15 @@ dummy_func(
             ERROR_IF(res == NULL, error);
         }
 
-        pseudo(SETUP_FINALLY) = {
+        pseudo(SETUP_FINALLY, (HAS_ARG)) = {
             NOP,
         };
 
-        pseudo(SETUP_CLEANUP) = {
+        pseudo(SETUP_CLEANUP, (HAS_ARG)) = {
             NOP,
         };
 
-        pseudo(SETUP_WITH) = {
+        pseudo(SETUP_WITH, (HAS_ARG)) = {
             NOP,
         };
 
diff --git a/Python/flowgraph.c b/Python/flowgraph.c
index 87401e14f97f02..4c69af301505c6 100644
--- a/Python/flowgraph.c
+++ b/Python/flowgraph.c
@@ -2239,7 +2239,7 @@ convert_pseudo_ops(basicblock *entryblock)
         for (int i = 0; i < b->b_iused; i++) {
             cfg_instr *instr = &b->b_instr[i];
             if (is_block_push(instr) || instr->i_opcode == POP_BLOCK) {
-                assert(SAME_OPCODE_METADATA(instr->i_opcode, NOP));
+                assert(OPCODE_HAS_ARG(instr->i_opcode) || instr->i_opcode == POP_BLOCK);
                 INSTR_SET_OP0(instr, NOP);
             }
             else if (instr->i_opcode == LOAD_CLOSURE) {
diff --git a/Tools/cases_generator/analysis.py b/Tools/cases_generator/analysis.py
index 603b15596f16de..26d92c13cd82ab 100644
--- a/Tools/cases_generator/analysis.py
+++ b/Tools/cases_generator/analysis.py
@@ -390,9 +390,14 @@ def analyze_pseudo(self, pseudo: parsing.Pseudo) -> PseudoInstruction:
             else:
                 targets.append(self.macro_instrs[target_name])
         assert targets
-        ignored_flags = {"HAS_EVAL_BREAK_FLAG", "HAS_DEOPT_FLAG", "HAS_ERROR_FLAG", "HAS_ESCAPES_FLAG"}
+        ignored_flags = {"HAS_EVAL_BREAK_FLAG", "HAS_DEOPT_FLAG", "HAS_ERROR_FLAG",
+                          "HAS_ESCAPES_FLAG"}
         assert len({t.instr_flags.bitmap(ignore=ignored_flags) for t in targets}) == 1
-        return PseudoInstruction(pseudo.name, targets, targets[0].instr_flags)
+
+        flags = InstructionFlags(**{f"{f}_FLAG" : True for f in pseudo.flags})
+        for t in targets:
+            flags.add(t.instr_flags)
+        return PseudoInstruction(pseudo.name, targets, flags)
 
     def analyze_instruction(
         self, instr: Instruction, offset: int
diff --git a/Tools/cases_generator/parsing.py b/Tools/cases_generator/parsing.py
index d36bd52b022ea9..d0503673bd5790 100644
--- a/Tools/cases_generator/parsing.py
+++ b/Tools/cases_generator/parsing.py
@@ -138,6 +138,7 @@ class Family(Node):
 @dataclass
 class Pseudo(Node):
     name: str
+    flags: list[str]    # instr flags to set on the pseudo instruction
     targets: list[str]  # opcodes this can be replaced by
 
 
@@ -374,19 +375,39 @@ def family_def(self) -> Family | None:
                                     )
         return None
 
+    def flags(self) -> list[str] | None:
+        here = self.getpos()
+        if self.expect(lx.LPAREN):
+            if tkn := self.expect(lx.IDENTIFIER):
+                flags = [tkn.text]
+                while self.expect(lx.COMMA):
+                    if tkn := self.expect(lx.IDENTIFIER):
+                        flags.append(tkn.text)
+                    else:
+                        break
+                if not self.expect(lx.RPAREN):
+                    raise self.make_syntax_error("Expected comma or right paren")
+                return flags
+        self.setpos(here)
+        return None
+
     @contextual
     def pseudo_def(self) -> Pseudo | None:
         if (tkn := self.expect(lx.IDENTIFIER)) and tkn.text == "pseudo":
             size = None
             if self.expect(lx.LPAREN):
                 if tkn := self.expect(lx.IDENTIFIER):
+                    if self.expect(lx.COMMA):
+                        flags = self.flags()
+                    else:
+                        flags = []
                     if self.expect(lx.RPAREN):
                         if self.expect(lx.EQUALS):
                             if not self.expect(lx.LBRACE):
                                 raise self.make_syntax_error("Expected {")
                             if members := self.members():
                                 if self.expect(lx.RBRACE) and self.expect(lx.SEMI):
-                                    return Pseudo(tkn.text, members)
+                                    return Pseudo(tkn.text, flags, members)
         return None
 
     def members(self) -> list[str] | None:


Code review comments:
diff_hunk: @@ -2239,7 +2239,7 @@ convert_pseudo_ops(basicblock *entryblock)
         for (int i = 0; i < b->b_iused; i++) {
             cfg_instr *instr = &b->b_instr[i];
             if (is_block_push(instr) || instr->i_opcode == POP_BLOCK) {
-                assert(SAME_OPCODE_METADATA(instr->i_opcode, NOP));
+                assert(OPCODE_HAS_ARG(instr->i_opcode) || instr->i_opcode == POP_BLOCK);
path: Python/flowgraph.c
text: @user1:
Would it make sense to drop this assert and assert `OPCODE_HAS_ARG(instr->i_opcode)` in `is_block_push()`.
It seems a bit out of place here.