From a67909f638dd9eedea88bf3d64a8c317ab1fa2dc Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Wed, 6 Sep 2023 16:44:34 -0700 Subject: [PATCH 1/4] Make sure instrumentation line returns the correct opcode even if trace is turned off in the trace function --- Python/instrumentation.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Python/instrumentation.c b/Python/instrumentation.c index 36459687be7936..ddd3654670fcc1 100644 --- a/Python/instrumentation.c +++ b/Python/instrumentation.c @@ -1190,6 +1190,14 @@ _Py_call_instrumentation_line(PyThreadState *tstate, _PyInterpreterFrame* frame, if (err) { return -1; } + // If trace is disabled in trace function, we need to recalculate the original opcode + // of the current instruction because it might be INSTRUMENTED_INSTRUCTION before the + // trace function. + if (tstate->interp->sys_tracing_threads == 0) { + if (original_opcode == INSTRUMENTED_INSTRUCTION) { + original_opcode = instr->op.code; + } + } } } tools &= (255 - (1 << PY_MONITORING_SYS_TRACE_ID)); From 0bc904e355cf1a8beaa08e0536be8343d724ae62 Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Wed, 6 Sep 2023 17:17:45 -0700 Subject: [PATCH 2/4] Fix it for both sys.monitoring and sys.settrace --- Python/instrumentation.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/Python/instrumentation.c b/Python/instrumentation.c index ddd3654670fcc1..b15845a356c9b8 100644 --- a/Python/instrumentation.c +++ b/Python/instrumentation.c @@ -1190,14 +1190,6 @@ _Py_call_instrumentation_line(PyThreadState *tstate, _PyInterpreterFrame* frame, if (err) { return -1; } - // If trace is disabled in trace function, we need to recalculate the original opcode - // of the current instruction because it might be INSTRUMENTED_INSTRUCTION before the - // trace function. - if (tstate->interp->sys_tracing_threads == 0) { - if (original_opcode == INSTRUMENTED_INSTRUCTION) { - original_opcode = instr->op.code; - } - } } } tools &= (255 - (1 << PY_MONITORING_SYS_TRACE_ID)); @@ -1231,8 +1223,19 @@ _Py_call_instrumentation_line(PyThreadState *tstate, _PyInterpreterFrame* frame, remove_line_tools(code, i, 1 << tool); } } while (tools); + Py_DECREF(line_obj); done: + // original_opcode is acquired before trace function and the callbacks. It's + // possible that the trace function or the callbacks turn off the instruction + // monitoring. If the instruction instrumentation is stripped, using the + // opcode could crash the interpreter. We should use the original opcode + // instead. + if (monitoring->active_monitors.tools[PY_MONITORING_EVENT_INSTRUCTION] == 0 && + original_opcode == INSTRUMENTED_INSTRUCTION) { + original_opcode = instr->op.code; + } + assert(original_opcode != 0); assert(original_opcode != INSTRUMENTED_LINE); assert(_PyOpcode_Deopt[original_opcode] == original_opcode); From 7ee7d1dd5131862c568909128fc8f0a4d0bb7970 Mon Sep 17 00:00:00 2001 From: Tian Gao Date: Wed, 6 Sep 2023 17:18:20 -0700 Subject: [PATCH 3/4] Add regression tests --- Lib/test/test_monitoring.py | 9 +++++++++ Lib/test/test_pdb.py | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/Lib/test/test_monitoring.py b/Lib/test/test_monitoring.py index 845185be737eb2..99b9f6d937c13c 100644 --- a/Lib/test/test_monitoring.py +++ b/Lib/test/test_monitoring.py @@ -1152,6 +1152,15 @@ def func1(): ('instruction', 'func1', 14), ('line', 'get_events', 11)]) + def test_gh108976(self): + sys.monitoring.use_tool_id(0, "test") + sys.monitoring.set_events(0, 0) + sys.monitoring.register_callback(0, E.LINE, lambda *args: sys.monitoring.set_events(0, 0)) + sys.monitoring.register_callback(0, E.INSTRUCTION, lambda *args: 0) + sys.monitoring.set_events(0, E.LINE | E.INSTRUCTION) + sys.monitoring.set_events(0, 0) + + class TestInstallIncrementallly(MonitoringTestBase, unittest.TestCase): def check_events(self, func, must_include, tool=TEST_TOOL, recorders=(ExceptionRecorder,)): diff --git a/Lib/test/test_pdb.py b/Lib/test/test_pdb.py index 734b5c83cdff7d..a0c6b330a3a520 100644 --- a/Lib/test/test_pdb.py +++ b/Lib/test/test_pdb.py @@ -2198,6 +2198,27 @@ def test_pdb_issue_gh_101517(): (Pdb) continue """ +def test_pdb_issue_gh_108976(): + """See GH-108976 + + Make sure setting f_trace_opcodes = True won't crash pdb + + >>> def test_function(): + ... import sys + ... sys._getframe().f_trace_opcodes = True + ... import pdb; pdb.Pdb(nosigint=True, readrc=False).set_trace() + ... a = 1 + + >>> with PdbTestInput([ # doctest: +NORMALIZE_WHITESPACE + ... 'continue' + ... ]): + ... test_function() + bdb.Bdb.dispatch: unknown debugging event: 'opcode' + > (5)test_function() + -> a = 1 + (Pdb) continue + """ + def test_pdb_ambiguous_statements(): """See GH-104301 From c2b32e445313e4c417ea3ab5b51a210af5bb8ac1 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Thu, 7 Sep 2023 00:22:56 +0000 Subject: [PATCH 4/4] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2023-09-07-00-22-55.gh-issue-108976.RivVn6.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2023-09-07-00-22-55.gh-issue-108976.RivVn6.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-09-07-00-22-55.gh-issue-108976.RivVn6.rst b/Misc/NEWS.d/next/Core and Builtins/2023-09-07-00-22-55.gh-issue-108976.RivVn6.rst new file mode 100644 index 00000000000000..7155949965f13a --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2023-09-07-00-22-55.gh-issue-108976.RivVn6.rst @@ -0,0 +1 @@ +Make sure instrumentation line returns the correct opcode when instruction instrumentation is stripped