From 14d01262dad02579b3dffe5965f640ce21c38896 Mon Sep 17 00:00:00 2001 From: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> Date: Sat, 17 Jun 2023 17:00:16 +0100 Subject: gh-105481: remove HAS_ARG, HAS_CONST, IS_JUMP_OPCODE, IS_PSEUDO_OPCODE and replace by their new versions (#105865) --- Doc/library/dis.rst | 6 +++++ Include/internal/pycore_opcode.h | 13 ---------- Include/internal/pycore_opcode_utils.h | 26 +------------------ Include/opcode.h | 19 -------------- Lib/test/test__opcode.py | 10 ++------ .../2023-06-17-12-13-57.gh-issue-105481.KgBH5w.rst | 4 +++ Modules/_opcode.c | 22 +++++----------- Python/assemble.c | 3 +-- Python/ceval.c | 5 ++-- Python/compile.c | 17 ++++-------- Python/flowgraph.c | 21 ++++++++------- Tools/build/generate_opcode_h.py | 30 ---------------------- 12 files changed, 38 insertions(+), 138 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2023-06-17-12-13-57.gh-issue-105481.KgBH5w.rst diff --git a/Doc/library/dis.rst b/Doc/library/dis.rst index 1caf3c0..39d4357 100644 --- a/Doc/library/dis.rst +++ b/Doc/library/dis.rst @@ -318,6 +318,12 @@ operation is being performed, so the intermediate analysis object isn't useful: .. versionchanged:: 3.8 Added *jump* parameter. + .. versionchanged:: 3.13 + If ``oparg`` is omitted (or ``None``), the stack effect is now returned + for ``oparg=0``. Previously this was an error for opcodes that use their + arg. It is also no longer an error to pass an integer ``oparg`` when + the ``opcode`` does not use it; the ``oparg`` in this case is ignored. + .. _bytecodes: diff --git a/Include/internal/pycore_opcode.h b/Include/internal/pycore_opcode.h index d680039..5935823 100644 --- a/Include/internal/pycore_opcode.h +++ b/Include/internal/pycore_opcode.h @@ -12,24 +12,11 @@ extern "C" { #include "opcode.h" -extern const uint32_t _PyOpcode_Jump[9]; - extern const uint8_t _PyOpcode_Caches[256]; extern const uint8_t _PyOpcode_Deopt[256]; #ifdef NEED_OPCODE_TABLES -const uint32_t _PyOpcode_Jump[9] = { - 0U, - 0U, - 536870912U, - 135020544U, - 4163U, - 0U, - 0U, - 0U, - 48U, -}; const uint8_t _PyOpcode_Caches[256] = { [BINARY_SUBSCR] = 1, diff --git a/Include/internal/pycore_opcode_utils.h b/Include/internal/pycore_opcode_utils.h index d80b3c1..50ff2af 100644 --- a/Include/internal/pycore_opcode_utils.h +++ b/Include/internal/pycore_opcode_utils.h @@ -15,10 +15,7 @@ extern "C" { #define IS_WITHIN_OPCODE_RANGE(opcode) \ (((opcode) >= 0 && (opcode) <= MAX_REAL_OPCODE) || \ - IS_PSEUDO_OPCODE(opcode)) - -#define IS_JUMP_OPCODE(opcode) \ - is_bit_set_in_table(_PyOpcode_Jump, opcode) + IS_PSEUDO_INSTR(opcode)) #define IS_BLOCK_PUSH_OPCODE(opcode) \ ((opcode) == SETUP_FINALLY || \ @@ -55,27 +52,6 @@ extern "C" { (opcode) == RAISE_VARARGS || \ (opcode) == RERAISE) -#define LOG_BITS_PER_INT 5 -#define MASK_LOW_LOG_BITS 31 - -static inline int -is_bit_set_in_table(const uint32_t *table, int bitindex) { - /* Is the relevant bit set in the relevant word? */ - /* 512 bits fit into 9 32-bits words. - * Word is indexed by (bitindex>>ln(size of int in bits)). - * Bit within word is the low bits of bitindex. - */ - if (bitindex >= 0 && bitindex < 512) { - uint32_t word = table[bitindex >> LOG_BITS_PER_INT]; - return (word >> (bitindex & MASK_LOW_LOG_BITS)) & 1; - } - else { - return 0; - } -} - -#undef LOG_BITS_PER_INT -#undef MASK_LOW_LOG_BITS /* Flags used in the oparg for MAKE_FUNCTION */ #define MAKE_FUNCTION_DEFAULTS 0x01 diff --git a/Include/opcode.h b/Include/opcode.h index c6ffaee..43a1806 100644 --- a/Include/opcode.h +++ b/Include/opcode.h @@ -50,7 +50,6 @@ extern "C" { #define SETUP_ANNOTATIONS 85 #define LOAD_LOCALS 87 #define POP_EXCEPT 89 -#define HAVE_ARGUMENT 90 #define STORE_NAME 90 #define DELETE_NAME 91 #define UNPACK_SEQUENCE 92 @@ -219,22 +218,6 @@ extern "C" { #define UNPACK_SEQUENCE_TWO_TUPLE 160 #define SEND_GEN 161 -#define HAS_ARG(op) ((((op) >= HAVE_ARGUMENT) && (!IS_PSEUDO_OPCODE(op)))\ - || ((op) == JUMP) \ - || ((op) == JUMP_NO_INTERRUPT) \ - || ((op) == LOAD_METHOD) \ - || ((op) == LOAD_SUPER_METHOD) \ - || ((op) == LOAD_ZERO_SUPER_METHOD) \ - || ((op) == LOAD_ZERO_SUPER_ATTR) \ - || ((op) == STORE_FAST_MAYBE_NULL) \ - ) - -#define HAS_CONST(op) (false\ - || ((op) == LOAD_CONST) \ - || ((op) == RETURN_CONST) \ - || ((op) == KW_NAMES) \ - ) - #define NB_ADD 0 #define NB_AND 1 #define NB_FLOOR_DIVIDE 2 @@ -265,8 +248,6 @@ extern "C" { /* Defined in Lib/opcode.py */ #define ENABLE_SPECIALIZATION 1 -#define IS_PSEUDO_OPCODE(op) (((op) >= MIN_PSEUDO_OPCODE) && ((op) <= MAX_PSEUDO_OPCODE)) - #ifdef __cplusplus } #endif diff --git a/Lib/test/test__opcode.py b/Lib/test/test__opcode.py index 7640c6f..3e08492 100644 --- a/Lib/test/test__opcode.py +++ b/Lib/test/test__opcode.py @@ -15,20 +15,14 @@ class OpcodeTests(unittest.TestCase): self.assertEqual(stack_effect(dis.opmap['BUILD_SLICE'], 1), -1) self.assertEqual(stack_effect(dis.opmap['BUILD_SLICE'], 3), -2) self.assertRaises(ValueError, stack_effect, 30000) - self.assertRaises(ValueError, stack_effect, dis.opmap['BUILD_SLICE']) - self.assertRaises(ValueError, stack_effect, dis.opmap['POP_TOP'], 0) # All defined opcodes has_arg = dis.hasarg for name, code in filter(lambda item: item[0] not in dis.deoptmap, dis.opmap.items()): if code >= opcode.MIN_INSTRUMENTED_OPCODE: continue with self.subTest(opname=name): - if code not in has_arg: - stack_effect(code) - self.assertRaises(ValueError, stack_effect, code, 0) - else: - stack_effect(code, 0) - self.assertRaises(ValueError, stack_effect, code) + stack_effect(code) + stack_effect(code, 0) # All not defined opcodes for code in set(range(256)) - set(dis.opmap.values()): with self.subTest(opcode=code): diff --git a/Misc/NEWS.d/next/Library/2023-06-17-12-13-57.gh-issue-105481.KgBH5w.rst b/Misc/NEWS.d/next/Library/2023-06-17-12-13-57.gh-issue-105481.KgBH5w.rst new file mode 100644 index 0000000..11084ef --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-06-17-12-13-57.gh-issue-105481.KgBH5w.rst @@ -0,0 +1,4 @@ +:func:`~dis.stack_effect` no longer raises an exception if an ``oparg`` is +provided for an ``opcode`` that doesn't use its arg, or when it is not +provided for an ``opcode`` that does use it. In the latter case, the stack +effect is returned for ``oparg=0``. diff --git a/Modules/_opcode.c b/Modules/_opcode.c index b70d426..3c0fce4 100644 --- a/Modules/_opcode.c +++ b/Modules/_opcode.c @@ -27,25 +27,16 @@ _opcode_stack_effect_impl(PyObject *module, int opcode, PyObject *oparg, PyObject *jump) /*[clinic end generated code: output=64a18f2ead954dbb input=461c9d4a44851898]*/ { - int effect; int oparg_int = 0; int jump_int; - if (HAS_ARG(opcode)) { - if (oparg == Py_None) { - PyErr_SetString(PyExc_ValueError, - "stack_effect: opcode requires oparg but oparg was not specified"); - return -1; - } + + if (oparg != Py_None) { oparg_int = (int)PyLong_AsLong(oparg); if ((oparg_int == -1) && PyErr_Occurred()) { return -1; } } - else if (oparg != Py_None) { - PyErr_SetString(PyExc_ValueError, - "stack_effect: opcode does not permit oparg but oparg was specified"); - return -1; - } + if (jump == Py_None) { jump_int = -1; } @@ -60,11 +51,10 @@ _opcode_stack_effect_impl(PyObject *module, int opcode, PyObject *oparg, "stack_effect: jump must be False, True or None"); return -1; } - effect = PyCompile_OpcodeStackEffectWithJump(opcode, oparg_int, jump_int); + int effect = PyCompile_OpcodeStackEffectWithJump(opcode, oparg_int, jump_int); if (effect == PY_INVALID_STACK_EFFECT) { - PyErr_SetString(PyExc_ValueError, - "invalid opcode or oparg"); - return -1; + PyErr_SetString(PyExc_ValueError, "invalid opcode or oparg"); + return -1; } return effect; } diff --git a/Python/assemble.c b/Python/assemble.c index 85c6fe7..d6213f8 100644 --- a/Python/assemble.c +++ b/Python/assemble.c @@ -339,10 +339,9 @@ static void write_instr(_Py_CODEUNIT *codestr, instruction *instr, int ilen) { int opcode = instr->i_opcode; - assert(IS_PSEUDO_OPCODE(opcode) == IS_PSEUDO_INSTR(opcode)); assert(!IS_PSEUDO_INSTR(opcode)); int oparg = instr->i_oparg; - assert(HAS_ARG(opcode) || oparg == 0); + assert(OPCODE_HAS_ARG(opcode) || oparg == 0); int caches = _PyOpcode_Caches[opcode]; switch (ilen - caches) { case 4: diff --git a/Python/ceval.c b/Python/ceval.c index b628190..b1ac4de 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -29,6 +29,7 @@ #include "pycore_frame.h" #include "frameobject.h" // _PyInterpreterFrame_GetLine #include "opcode.h" +#include "opcode_metadata.h" #include "pydtrace.h" #include "setobject.h" #include "structmember.h" // struct PyMemberDef, T_OFFSET_EX @@ -141,7 +142,7 @@ lltrace_instruction(_PyInterpreterFrame *frame, const char *opname = _PyOpcode_OpName[opcode]; assert(opname != NULL); int offset = (int)(next_instr - _PyCode_CODE(_PyFrame_GetCode(frame))); - if (HAS_ARG((int)_PyOpcode_Deopt[opcode])) { + if (OPCODE_HAS_ARG((int)_PyOpcode_Deopt[opcode])) { printf("%d: %s %d\n", offset * 2, opname, oparg); } else { @@ -882,7 +883,7 @@ handle_eval_breaker: #if USE_COMPUTED_GOTOS _unknown_opcode: #else - EXTRA_CASES // From opcode.h, a 'case' for each unused opcode + EXTRA_CASES // From pycore_opcode.h, a 'case' for each unused opcode #endif /* Tell C compilers not to hold the opcode variable in the loop. next_instr points the current instruction without TARGET(). */ diff --git a/Python/compile.c b/Python/compile.c index afb7b7d..cfa945b 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -134,9 +134,8 @@ enum { int _PyCompile_InstrSize(int opcode, int oparg) { - assert(IS_PSEUDO_OPCODE(opcode) == IS_PSEUDO_INSTR(opcode)); assert(!IS_PSEUDO_INSTR(opcode)); - assert(HAS_ARG(opcode) || oparg == 0); + assert(OPCODE_HAS_ARG(opcode) || oparg == 0); int extended_args = (0xFFFFFF < oparg) + (0xFFFF < oparg) + (0xFF < oparg); int caches = _PyOpcode_Caches[opcode]; return extended_args + 1 + caches; @@ -248,15 +247,9 @@ instr_sequence_use_label(instr_sequence *seq, int lbl) { static int instr_sequence_addop(instr_sequence *seq, int opcode, int oparg, location loc) { - /* compare old and new opcode macros - use ! to compare as bools. */ - assert(!HAS_ARG(opcode) == !OPCODE_HAS_ARG(opcode)); - assert(!HAS_CONST(opcode) == !OPCODE_HAS_CONST(opcode)); - assert(!OPCODE_HAS_JUMP(opcode) == !OPCODE_HAS_JUMP(opcode)); - assert(0 <= opcode && opcode <= MAX_OPCODE); - assert(IS_PSEUDO_OPCODE(opcode) == IS_PSEUDO_INSTR(opcode)); assert(IS_WITHIN_OPCODE_RANGE(opcode)); - assert(HAS_ARG(opcode) || HAS_TARGET(opcode) || oparg == 0); + assert(OPCODE_HAS_ARG(opcode) || HAS_TARGET(opcode) || oparg == 0); assert(0 <= oparg && oparg < (1 << 30)); int idx = instr_sequence_next_inst(seq); @@ -874,7 +867,7 @@ PyCompile_OpcodeStackEffect(int opcode, int oparg) static int codegen_addop_noarg(instr_sequence *seq, int opcode, location loc) { - assert(!HAS_ARG(opcode)); + assert(!OPCODE_HAS_ARG(opcode)); assert(!IS_ASSEMBLER_OPCODE(opcode)); return instr_sequence_addop(seq, opcode, 0, loc); } @@ -1151,7 +1144,7 @@ codegen_addop_j(instr_sequence *seq, location loc, } #define ADDOP_N(C, LOC, OP, O, TYPE) { \ - assert(!HAS_CONST(OP)); /* use ADDOP_LOAD_CONST_NEW */ \ + assert(!OPCODE_HAS_CONST(OP)); /* use ADDOP_LOAD_CONST_NEW */ \ if (compiler_addop_o((C)->u, (LOC), (OP), (C)->u->u_metadata.u_ ## TYPE, (O)) < 0) { \ Py_DECREF((O)); \ return ERROR; \ @@ -7798,7 +7791,7 @@ instructions_to_instr_sequence(PyObject *instructions, instr_sequence *seq) goto error; } int oparg; - if (HAS_ARG(opcode)) { + if (OPCODE_HAS_ARG(opcode)) { oparg = PyLong_AsLong(PyTuple_GET_ITEM(item, 1)); if (PyErr_Occurred()) { goto error; diff --git a/Python/flowgraph.c b/Python/flowgraph.c index de5c5e7..39f780e 100644 --- a/Python/flowgraph.c +++ b/Python/flowgraph.c @@ -45,14 +45,13 @@ is_block_push(cfg_instr *i) static inline int is_jump(cfg_instr *i) { - assert(!OPCODE_HAS_JUMP(i->i_opcode) == !IS_JUMP_OPCODE(i->i_opcode)); return OPCODE_HAS_JUMP(i->i_opcode); } /* One arg*/ #define INSTR_SET_OP1(I, OP, ARG) \ do { \ - assert(HAS_ARG(OP)); \ + assert(OPCODE_HAS_ARG(OP)); \ _PyCfgInstruction *_instr__ptr_ = (I); \ _instr__ptr_->i_opcode = (OP); \ _instr__ptr_->i_oparg = (ARG); \ @@ -61,7 +60,7 @@ is_jump(cfg_instr *i) /* No args*/ #define INSTR_SET_OP0(I, OP) \ do { \ - assert(!HAS_ARG(OP)); \ + assert(!OPCODE_HAS_ARG(OP)); \ _PyCfgInstruction *_instr__ptr_ = (I); \ _instr__ptr_->i_opcode = (OP); \ _instr__ptr_->i_oparg = 0; \ @@ -111,7 +110,7 @@ basicblock_addop(basicblock *b, int opcode, int oparg, location loc) { assert(IS_WITHIN_OPCODE_RANGE(opcode)); assert(!IS_ASSEMBLER_OPCODE(opcode)); - assert(HAS_ARG(opcode) || HAS_TARGET(opcode) || oparg == 0); + assert(OPCODE_HAS_ARG(opcode) || HAS_TARGET(opcode) || oparg == 0); assert(0 <= oparg && oparg < (1 << 30)); int off = basicblock_next_instr(b); @@ -193,7 +192,7 @@ dump_instr(cfg_instr *i) char arg[128]; *arg = '\0'; - if (HAS_ARG(i->i_opcode)) { + if (OPCODE_HAS_ARG(i->i_opcode)) { sprintf(arg, "arg: %d ", i->i_oparg); } if (HAS_TARGET(i->i_opcode)) { @@ -1108,7 +1107,7 @@ static PyObject* get_const_value(int opcode, int oparg, PyObject *co_consts) { PyObject *constant = NULL; - assert(HAS_CONST(opcode)); + assert(OPCODE_HAS_CONST(opcode)); if (opcode == LOAD_CONST) { constant = PyList_GET_ITEM(co_consts, oparg); } @@ -1139,7 +1138,7 @@ fold_tuple_on_constants(PyObject *const_cache, assert(inst[n].i_oparg == n); for (int i = 0; i < n; i++) { - if (!HAS_CONST(inst[i].i_opcode)) { + if (!OPCODE_HAS_CONST(inst[i].i_opcode)) { return SUCCESS; } } @@ -1542,8 +1541,8 @@ optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts) } break; default: - /* All HAS_CONST opcodes should be handled with LOAD_CONST */ - assert (!HAS_CONST(inst->i_opcode)); + /* All OPCODE_HAS_CONST opcodes should be handled with LOAD_CONST */ + assert (!OPCODE_HAS_CONST(inst->i_opcode)); } } @@ -1793,7 +1792,7 @@ remove_unused_consts(basicblock *entryblock, PyObject *consts) /* mark used consts */ for (basicblock *b = entryblock; b != NULL; b = b->b_next) { for (int i = 0; i < b->b_iused; i++) { - if (HAS_CONST(b->b_instr[i].i_opcode)) { + if (OPCODE_HAS_CONST(b->b_instr[i].i_opcode)) { int index = b->b_instr[i].i_oparg; index_map[index] = index; } @@ -1846,7 +1845,7 @@ remove_unused_consts(basicblock *entryblock, PyObject *consts) for (basicblock *b = entryblock; b != NULL; b = b->b_next) { for (int i = 0; i < b->b_iused; i++) { - if (HAS_CONST(b->b_instr[i].i_opcode)) { + if (OPCODE_HAS_CONST(b->b_instr[i].i_opcode)) { int index = b->b_instr[i].i_oparg; assert(reverse_index_map[index] >= 0); assert(reverse_index_map[index] < n_used_consts); diff --git a/Tools/build/generate_opcode_h.py b/Tools/build/generate_opcode_h.py index 5be9810..ec9fdae 100644 --- a/Tools/build/generate_opcode_h.py +++ b/Tools/build/generate_opcode_h.py @@ -21,8 +21,6 @@ extern "C" {{ footer = """ -#define IS_PSEUDO_OPCODE(op) (((op) >= MIN_PSEUDO_OPCODE) && ((op) <= MAX_PSEUDO_OPCODE)) - #ifdef __cplusplus } #endif @@ -68,16 +66,6 @@ DEFINE = "#define {:<38} {:>3}\n" UINT32_MASK = (1<<32)-1 -def write_int_array_from_ops(name, ops, out): - bits = 0 - for op in ops: - bits |= 1<>= 32 - assert bits == 0 - out.write(f"}};\n") def main(opcode_py, outfile='Include/opcode.h', internaloutfile='Include/internal/pycore_opcode.h', @@ -100,7 +88,6 @@ def main(opcode_py, outfile='Include/opcode.h', _pseudo_ops = opcode['_pseudo_ops'] ENABLE_SPECIALIZATION = opcode["ENABLE_SPECIALIZATION"] - HAVE_ARGUMENT = opcode["HAVE_ARGUMENT"] MIN_PSEUDO_OPCODE = opcode["MIN_PSEUDO_OPCODE"] MAX_PSEUDO_OPCODE = opcode["MAX_PSEUDO_OPCODE"] MIN_INSTRUMENTED_OPCODE = opcode["MIN_INSTRUMENTED_OPCODE"] @@ -130,8 +117,6 @@ def main(opcode_py, outfile='Include/opcode.h', for name in opname: if name in opmap: op = opmap[name] - if op == HAVE_ARGUMENT: - fobj.write(DEFINE.format("HAVE_ARGUMENT", HAVE_ARGUMENT)) if op == MIN_PSEUDO_OPCODE: fobj.write(DEFINE.format("MIN_PSEUDO_OPCODE", MIN_PSEUDO_OPCODE)) if op == MIN_INSTRUMENTED_OPCODE: @@ -146,11 +131,9 @@ def main(opcode_py, outfile='Include/opcode.h', for name, op in specialized_opmap.items(): fobj.write(DEFINE.format(name, op)) - iobj.write("\nextern const uint32_t _PyOpcode_Jump[9];\n") iobj.write("\nextern const uint8_t _PyOpcode_Caches[256];\n") iobj.write("\nextern const uint8_t _PyOpcode_Deopt[256];\n") iobj.write("\n#ifdef NEED_OPCODE_TABLES\n") - write_int_array_from_ops("_PyOpcode_Jump", opcode['hasjrel'] + opcode['hasjabs'], iobj) iobj.write("\nconst uint8_t _PyOpcode_Caches[256] = {\n") for i, entries in enumerate(opcode["_inline_cache_entries"]): @@ -172,19 +155,6 @@ def main(opcode_py, outfile='Include/opcode.h', iobj.write("#endif // NEED_OPCODE_TABLES\n") fobj.write("\n") - fobj.write("#define HAS_ARG(op) ((((op) >= HAVE_ARGUMENT) && (!IS_PSEUDO_OPCODE(op)))\\") - for op in _pseudo_ops: - if opmap[op] in hasarg: - fobj.write(f"\n || ((op) == {op}) \\") - fobj.write("\n )\n") - - fobj.write("\n") - fobj.write("#define HAS_CONST(op) (false\\") - for op in hasconst: - fobj.write(f"\n || ((op) == {opname[op]}) \\") - fobj.write("\n )\n") - - fobj.write("\n") for i, (op, _) in enumerate(opcode["_nb_ops"]): fobj.write(DEFINE.format(op, i)) -- cgit v0.12