Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions Lib/test/test_free_threading/test_monitoring.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Tests monitoring, sys.settrace, and sys.setprofile in a multi-threaded
environment to verify things are thread-safe in a free-threaded build"""

import dis
import sys
import threading
import time
Expand Down Expand Up @@ -251,6 +252,62 @@ def append(self, trace):

@threading_helper.requires_working_threading()
class MonitoringMisc(MonitoringTestMixin, TestCase):
def test_disable_line_keeps_instruction_events(self):
def func(x):
a = x + 1
b = a * 2
return b

code = func.__code__
expected = [instr.offset for instr in dis.get_instructions(func)
if instr.opname != "RESUME"]
# Two threads, each with two lists of instruction offsets (one per call).
# Check that disabling LINE preserves INSTRUCTION events in every
# thread-local bytecode copy, including on subsequent calls.
records = [[[], []] for _ in range(2)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me forever to understand the structure. Let's explain a bit here - why the nested structures.

local = threading.local()
ready = Barrier(len(records) + 1)
start = threading.Event()

def worker(calls):
# Create each thread's bytecode copy before enabling monitoring.
func(1)
ready.wait()
start.wait()
for instructions in calls:
local.instructions = instructions
func(1)

def instruction(code, offset):
local.instructions.append(offset)

def line(code, lineno):
return monitoring.DISABLE

threads = [Thread(target=worker, args=(calls,)) for calls in records]
try:
with threading_helper.start_threads(threads, unlock=start.set):
ready.wait()
monitoring.register_callback(
self.tool_id, monitoring.events.INSTRUCTION, instruction)
monitoring.register_callback(
self.tool_id, monitoring.events.LINE, line)
monitoring.set_local_events(
self.tool_id, code,
monitoring.events.INSTRUCTION | monitoring.events.LINE)
start.set()
finally:
monitoring.set_local_events(self.tool_id, code, 0)
monitoring.register_callback(
self.tool_id, monitoring.events.INSTRUCTION, None)
monitoring.register_callback(
self.tool_id, monitoring.events.LINE, None)
monitoring.restart_events()

for calls in records:
for instructions in calls:
self.assertEqual(instructions, expected)

def register_callback(self, barrier):
barrier.wait()

Expand Down
74 changes: 74 additions & 0 deletions Lib/test/test_monitoring.py
Original file line number Diff line number Diff line change
Expand Up @@ -1192,6 +1192,80 @@ def __call__(self, code, offset):
class TestLineAndInstructionEvents(CheckEvents):
maxDiff = None

def test_disable_line_keeps_instruction_events(self):
for local in (False, True):
# Use the same tool or different tools for LINE and INSTRUCTION.
for line_tool, instr_tool in (
(TEST_TOOL, TEST_TOOL),
(TEST_TOOL2, TEST_TOOL),
):
with self.subTest(local=local, line_tool=line_tool,
instr_tool=instr_tool):
self.check_disable_line_keeps_instruction_events(
local, line_tool, instr_tool)

def check_disable_line_keeps_instruction_events(self, local, line_tool,
instr_tool):
def func(x):
a = x + 1
# Split the assignment so line events also move backwards from
# the expression's line to the assignment's line.
b = (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary for this function to be like this? If we need it to be a multi-line statement, we better talk about it.

a * 2
)
return b

code = func.__code__
instructions = []
lines = []
disable = False

def instruction(code_arg, offset):
if code_arg is code:
instructions.append(offset)

def line(code_arg, lineno):
if code_arg is code:
lines.append(lineno)
if disable:
return sys.monitoring.DISABLE

sys.monitoring.register_callback(instr_tool, E.INSTRUCTION, instruction)
sys.monitoring.register_callback(line_tool, E.LINE, line)
events = {instr_tool: E.INSTRUCTION}
events[line_tool] = events.get(line_tool, 0) | E.LINE
try:
for tool, mask in events.items():
if local:
sys.monitoring.set_local_events(tool, code, mask)
else:
sys.monitoring.set_events(tool, mask)
func(1)
expected_instructions = instructions[:]
expected_lines = lines[:]
self.assertEqual(expected_instructions, [
inst.offset for inst in dis.get_instructions(func)
if inst.opname != "RESUME"
])
self.assertTrue(expected_lines)
disable = True
for restart in (False, True):
if restart:
sys.monitoring.restart_events()
for call in range(2):
instructions.clear()
lines.clear()
func(1)
self.assertEqual(instructions, expected_instructions)
self.assertEqual(lines, expected_lines if call == 0 else [])
finally:
for tool in events:
sys.monitoring.set_local_events(tool, code, 0)
sys.monitoring.set_events(tool, 0)
sys.monitoring.register_callback(instr_tool, E.INSTRUCTION, None)
sys.monitoring.register_callback(line_tool, E.LINE, None)
sys.monitoring.restart_events()

def test_simple(self):

def func1():
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix missing :mod:`sys.monitoring` ``INSTRUCTION`` events when a ``LINE``
callback returns :data:`sys.monitoring.DISABLE`.
16 changes: 13 additions & 3 deletions Python/instrumentation.c
Original file line number Diff line number Diff line change
Expand Up @@ -715,9 +715,6 @@ de_instrument_line(PyCodeObject *code, _Py_CODEUNIT *bytecode, _PyCoMonitoringDa
}
_PyCoLineInstrumentationData *lines = monitoring->lines;
int original_opcode = _PyCode_GetOriginalOpcode(lines, i);
if (original_opcode == INSTRUMENTED_INSTRUCTION) {
set_original_opcode(lines, i, monitoring->per_instruction_opcodes[i]);
}
CHECK(original_opcode != 0);
CHECK(original_opcode == _PyOpcode_Deopt[original_opcode]);
FT_ATOMIC_STORE_UINT8(instr->op.code, original_opcode);
Expand Down Expand Up @@ -882,7 +879,15 @@ remove_line_tools(PyCodeObject * code, int offset, int tools)
should_de_instrument = ((single_tool & tools) == single_tool);
}
if (should_de_instrument) {
/* Restore all thread-local bytecodes before updating the shared

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment here is actually explaining what the line above is doing right? I think we should move this comment before the line above, or rephase it.

* original opcode. */
MODIFY_BYTECODE(code, de_instrument_line, monitoring, offset);
if (_PyCode_GetOriginalOpcode(monitoring->lines, offset) ==
INSTRUMENTED_INSTRUCTION)
{
set_original_opcode(monitoring->lines, offset,
monitoring->per_instruction_opcodes[offset]);
}
}
}

Expand Down Expand Up @@ -1424,6 +1429,11 @@ _Py_call_instrumentation_line(PyThreadState *tstate, _PyInterpreterFrame* frame,
uint8_t original_opcode;
done:
original_opcode = _PyCode_GetOriginalOpcode(line_data, i);
if (instr->op.code == INSTRUMENTED_INSTRUCTION) {
/* A LINE callback may have disabled the last LINE tool while
* leaving INSTRUCTION monitoring enabled. */
original_opcode = INSTRUMENTED_INSTRUCTION;
}
assert(original_opcode != 0);
assert(original_opcode != INSTRUMENTED_LINE);
assert(_PyOpcode_Deopt[original_opcode] == original_opcode);
Expand Down
Loading