Sep 9, 2023 · Sep 7, 2023 · Sep 7, 2023 · Sep 8, 2023 · Sep 8, 2023 · Sep 8, 2023
diff --git a/Lib/test/test_code.py b/Lib/test/test_code.py
        self.assertNotEqual(c, c1)
        self.assertNotEqual(hash(c), hash(c1))

    @cpython_only
    def test_code_equal_with_instrumentation(self):
        """ GH-109052

        Make sure the instrumentation doesn't affect the code equality
        The validity of this test relies on the fact that "x is x" and
        "x in x" have only one different instruction and the instructions
        have the same argument.

        """
        code1 = compile("x is x", "example.py", "eval")
        code2 = compile("x in x", "example.py", "eval")
        sys._getframe().f_trace_opcodes = True
        sys.settrace(lambda *args: None)
        exec(code1, {'x': []})
        exec(code2, {'x': []})
        self.assertNotEqual(code1, code2)
        sys.settrace(None)


 def isinterned(s):
    return s is sys.intern(('_' + s + '_')[1:-1])
diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-09-07-18-49-01.gh-issue-109052.TBU4nC.rst b/Misc/NEWS.d/next/Core and Builtins/2023-09-07-18-49-01.gh-issue-109052.TBU4nC.rst
 Use the base opcode when comparing code objects to avoid interference from instrumentation
diff --git a/Objects/codeobject.c b/Objects/codeobject.c
    for (int i = 0; i < Py_SIZE(co); i++) {
        _Py_CODEUNIT co_instr = _PyCode_CODE(co)[i];
        _Py_CODEUNIT cp_instr = _PyCode_CODE(cp)[i];
        uint8_t co_code =co_instr.op.code;
        uint8_t co_code =_Py_GetBaseOpcode(co, i);
        uint8_t co_arg = co_instr.op.arg;
        uint8_t cp_code =cp_instr.op.code;
        uint8_t cp_code =_Py_GetBaseOpcode(cp, i);
        uint8_t cp_arg = cp_instr.op.arg;

        if (co_code == ENTER_EXECUTOR) {
            const int exec_index = co_arg;
            _PyExecutorObject *exec = co->co_executors->executors[exec_index];
            co_code = exec->vm_data.opcode;
            co_code =_PyOpcode_Deopt[exec->vm_data.opcode];
            co_arg = exec->vm_data.oparg;
        }
        assert(co_code != ENTER_EXECUTOR);
        co_code = _PyOpcode_Deopt[co_code];

        if (cp_code == ENTER_EXECUTOR) {
            const int exec_index = cp_arg;
            _PyExecutorObject *exec = cp->co_executors->executors[exec_index];
            cp_code = exec->vm_data.opcode;
            cp_code =_PyOpcode_Deopt[exec->vm_data.opcode];
            cp_arg = exec->vm_data.oparg;
        }
        assert(cp_code != ENTER_EXECUTOR);
        cp_code = _PyOpcode_Deopt[cp_code];

        if (co_code != cp_code || co_arg != cp_arg) {
            goto unequal;
Original file line number	Diff line number	Diff line change
Expand Up		@@ -505,6 +505,25 @@ def test_code_hash_uses_bytecode(self):
		self.assertNotEqual(c, c1)
		self.assertNotEqual(hash(c), hash(c1))

		@cpython_only
		def test_code_equal_with_instrumentation(self):
		""" GH-109052

		Make sure the instrumentation doesn't affect the code equality
		The validity of this test relies on the fact that "x is x" and
		"x in x" have only one different instruction and the instructions
		have the same argument.

		"""
		code1 = compile("x is x", "example.py", "eval")
		code2 = compile("x in x", "example.py", "eval")
		sys._getframe().f_trace_opcodes = True
		sys.settrace(lambda *args: None)
		exec(code1, {'x': []})
		exec(code2, {'x': []})
		self.assertNotEqual(code1, code2)
		sys.settrace(None)


		def isinterned(s):
		return s is sys.intern(('_' + s + '_')[1:-1])
Expand Down
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Use the base opcode when comparing code objects to avoid interference from instrumentation
Original file line number	Diff line number	Diff line change
Expand Up		@@ -1797,28 +1797,26 @@ code_richcompare(PyObject self, PyObject other, int op)
		for (int i = 0; i < Py_SIZE(co); i++) {
		_Py_CODEUNIT co_instr = _PyCode_CODE(co)[i];
		_Py_CODEUNIT cp_instr = _PyCode_CODE(cp)[i];
		uint8_t co_code =co_instr.op.code;
		uint8_t co_code =_Py_GetBaseOpcode(co, i);
Copy link Member corona10Sep 8, 2023• edited Loading Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. I am not sure that calling this API at this moment is correct. Because it will call _PyOpcode_Deopt twice in the end. See L1812, what will happen Copy link Member corona10Sep 8, 2023• edited Loading Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. Or move co_code or cp_code = _PyOpcode_Deopt[cocode or cp_code]; only if cp_code or co_code is ENTER_EXECUTOR at L1805 or L1814 Copy link Member gvanrossumSep 8, 2023 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. `_PyOpcode_Deopt[]` is idempotent, so it is harmless. I think we should change this code to be exactly the same as what`deopt_code()` does, since that's what we're after -- we want to compare`co.co_code` to`cp.co_code` without creating those objects. I also think that`_Py_GetBaseOpcode()` should map`ENTER_EXECUTOR` so we wouldn't have to worry about it here at all. Copy link Member corona10Sep 9, 2023• edited Loading Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. FYI, If we decide to fix`_Py_GetBaseOpcode()`, we can update#107265 too cc@brandtbucher Copy link Member corona10Sep 9, 2023 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. And maybe we can close#107265 in one short. Copy link Member gvanrossumSep 9, 2023 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. IIRC there are also cases where we absolutely want the actual opcode, so in those cases we still need to handle ENTER_EXECUTOR explicitly. But yeah, fixing this in`_Py_GetBaseOpcode()` might reduce the special cases elsewhere. In a new PR, please. :-) Copy link Member gvanrossumSep 11, 2023 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. IMPORTANT! Do not attempt to "fix"`_Py_GetBaseOpcode()` by adding`ENTER_EXECUTOR` handling. This will leave the`oparg` incorrect. That's why that bullet ingh-107265 uses~~strikethrough~~. corona10 reacted with thumbs up emoji
		uint8_t co_arg = co_instr.op.arg;
		uint8_t cp_code =cp_instr.op.code;
		uint8_t cp_code =_Py_GetBaseOpcode(cp, i);
Copy link Member corona10Sep 8, 2023 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. ditto
		uint8_t cp_arg = cp_instr.op.arg;

		if (co_code == ENTER_EXECUTOR) {
		const int exec_index = co_arg;
		_PyExecutorObject *exec = co->co_executors->executors[exec_index];
		co_code = exec->vm_data.opcode;
		co_code =_PyOpcode_Deopt[exec->vm_data.opcode];
Copy link Member brandtbucherSep 8, 2023 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. Nice catch. I think`deopt_code` should be doing this as well (it can't happen yet because these are all`JUMP_BACKWARD`s, which don't have specializations, but it could later when we start sticking executors in arbitrary locations). I agree with@gvanrossum though that`_Py_GetBaseOpcode` should just be the one place where all of this logic is kept.@gaogaotiantian, would you be interested in teaching`_Py_GetBaseOpcode` about`ENTER_EXECUTOR` and cleaning up all of the places where we're using that and`_PyOpcode_Deopt` to do the right thing? I can do it if not. Copy link MemberAuthor gaogaotiantianSep 9, 2023 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. Yes, I can work on that. Copy link MemberAuthor gaogaotiantianSep 12, 2023 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. So@gvanrossum mentioned we should NOT handle`ENTER_EXECUTOR` in`_Py_GetBaseOpcode` because of`oparg`. For this matter, it there any work to do?`_Py_GetBaseOpcode` already handles instrumentation and`_PyOpcode_Deopt`. Copy link Member gvanrossumSep 12, 2023 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others.Learn more. See the list of bullets ingh-107265.
		co_arg = exec->vm_data.oparg;
		}
		assert(co_code != ENTER_EXECUTOR);
		co_code = _PyOpcode_Deopt[co_code];

		if (cp_code == ENTER_EXECUTOR) {
		const int exec_index = cp_arg;
		_PyExecutorObject *exec = cp->co_executors->executors[exec_index];
		cp_code = exec->vm_data.opcode;
		cp_code =_PyOpcode_Deopt[exec->vm_data.opcode];
		cp_arg = exec->vm_data.oparg;
		}
		assert(cp_code != ENTER_EXECUTOR);
		cp_code = _PyOpcode_Deopt[cp_code];

		if (co_code != cp_code \|\| co_arg != cp_arg) {
		goto unequal;
Expand Down