Skip to content

gh-118415: Fix issues with local tracing being enabled/disabled on a function #118496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DinoV merged 4 commits into from
May 6, 2024
Merged

gh-118415: Fix issues with local tracing being enabled/disabled on a function #118496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3634b86
Lock around sanity assertions and always maintain original opcodes so…
DinoV May 1, 2024
da2270b
Be explicit and use _PyRLock instead of _RLock as we want the Python …
DinoV May 2, 2024
8d35484
Fix test and instrumentation_tools
DinoV May 3, 2024
c19b622
Fix formatting
DinoV May 6, 2024
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
39 changes: 26 additions & 13 deletions Lib/test/test_free_threading/test_monitoring.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,14 @@

from sys import monitoring
from test.support import is_wasi
from threading import Thread
from threading import Thread, _PyRLock
from unittest import TestCase


class InstrumentationMultiThreadedMixin:
if not hasattr(sys, "gettotalrefcount"):
thread_count = 50
func_count = 1000
fib = 15
else:
# Run a little faster in debug builds...
thread_count = 25
func_count = 500
fib = 15
thread_count = 10
func_count = 200
fib = 12

def after_threads(self):
"""Runs once after all the threads have started"""
Expand Down Expand Up @@ -175,9 +169,6 @@ def during_threads(self):
@unittest.skipIf(is_wasi, "WASI has no threads.")
class SetProfileMultiThreaded(InstrumentationMultiThreadedMixin, TestCase):
"""Uses sys.setprofile and repeatedly toggles instrumentation on and off"""
thread_count = 25
func_count = 200
fib = 15

def setUp(self):
self.set = False
Expand Down Expand Up @@ -227,6 +218,28 @@ def test_register_callback(self):
for ref in self.refs:
self.assertEqual(ref(), None)

def test_set_local_trace_opcodes(self):
def trace(frame, event, arg):
frame.f_trace_opcodes = True
return trace

sys.settrace(trace)
try:
l = _PyRLock()

def f():
for i in range(3000):
with l:
pass

t = Thread(target=f)
t.start()
for i in range(3000):
with l:
pass
t.join()
finally:
sys.settrace(None)

if __name__ == "__main__":
unittest.main()
91 changes: 45 additions & 46 deletions Python/instrumentation.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,6 @@ dump_instrumentation_data(PyCodeObject *code, int star, FILE*out)
}

#define CHECK(test) do { \
ASSERT_WORLD_STOPPED_OR_LOCKED(code); \
if (!(test)) { \
dump_instrumentation_data(code, i, stderr); \
} \
Expand Down Expand Up @@ -516,10 +515,6 @@ sanity_check_instrumentation(PyCodeObject *code)
if (!is_instrumented(opcode)) {
CHECK(_PyOpcode_Deopt[opcode] == opcode);
}
if (data->per_instruction_tools) {
uint8_t tools = active_monitors.tools[PY_MONITORING_EVENT_INSTRUCTION];
CHECK((tools & data->per_instruction_tools[i]) == data->per_instruction_tools[i]);
}
}
if (opcode == INSTRUMENTED_LINE) {
CHECK(data->lines);
Expand Down Expand Up @@ -677,8 +672,6 @@ de_instrument_per_instruction(PyCodeObject *code, int i)
}
assert(*opcode_ptr != INSTRUMENTED_INSTRUCTION);
assert(instr->op.code != INSTRUMENTED_INSTRUCTION);
/* Keep things clean for sanity check */
code->_co_monitoring->per_instruction_opcodes[i] = 0;
}


Expand Down Expand Up @@ -992,18 +985,32 @@ set_global_version(PyThreadState *tstate, uint32_t version)
static bool
is_version_up_to_date(PyCodeObject *code, PyInterpreterState *interp)
{
ASSERT_WORLD_STOPPED_OR_LOCKED(code);
return global_version(interp) == code->_co_instrumentation_version;
}

#ifndef NDEBUG
static bool
instrumentation_cross_checks(PyInterpreterState *interp, PyCodeObject *code)
{
ASSERT_WORLD_STOPPED_OR_LOCKED(code);
_Py_LocalMonitors expected = local_union(
interp->monitors,
code->_co_monitoring->local_monitors);
return monitors_equals(code->_co_monitoring->active_monitors, expected);
}

static int
debug_check_sanity(PyInterpreterState *interp, PyCodeObject *code)
{
int res;
LOCK_CODE(code);
res = is_version_up_to_date(code, interp) &&
instrumentation_cross_checks(interp, code);
UNLOCK_CODE();
return res;
}

#endif

static inline uint8_t
Expand All @@ -1018,8 +1025,7 @@ get_tools_for_instruction(PyCodeObject *code, PyInterpreterState *interp, int i,
event = PY_MONITORING_EVENT_CALL;
}
if (PY_MONITORING_IS_INSTRUMENTED_EVENT(event)) {
CHECK(is_version_up_to_date(code, interp));
CHECK(instrumentation_cross_checks(interp, code));
CHECK(debug_check_sanity(interp, code));
if (code->_co_monitoring->tools) {
tools = code->_co_monitoring->tools[i];
}
Expand Down Expand Up @@ -1218,8 +1224,7 @@ _Py_call_instrumentation_line(PyThreadState *tstate, _PyInterpreterFrame* frame,
{
PyCodeObject *code = _PyFrame_GetCode(frame);
assert(tstate->tracing == 0);
assert(is_version_up_to_date(code, tstate->interp));
assert(instrumentation_cross_checks(tstate->interp, code));
assert(debug_check_sanity(tstate->interp, code));
int i = (int)(instr - _PyCode_CODE(code));

_PyCoMonitoringData *monitoring = code->_co_monitoring;
Expand Down Expand Up @@ -1339,8 +1344,7 @@ int
_Py_call_instrumentation_instruction(PyThreadState *tstate, _PyInterpreterFrame* frame, _Py_CODEUNIT *instr)
{
PyCodeObject *code = _PyFrame_GetCode(frame);
assert(is_version_up_to_date(code, tstate->interp));
assert(instrumentation_cross_checks(tstate->interp, code));
assert(debug_check_sanity(tstate->interp, code));
int offset = (int)(instr - _PyCode_CODE(code));
_PyCoMonitoringData *instrumentation_data = code->_co_monitoring;
assert(instrumentation_data->per_instruction_opcodes);
Expand Down Expand Up @@ -1671,9 +1675,11 @@ update_instrumentation_data(PyCodeObject *code, PyInterpreterState *interp)
PyErr_NoMemory();
return -1;
}
/* This may not be necessary, as we can initialize this memory lazily, but it helps catch errors. */
// Initialize all of the instructions so if local events change while another thread is executing
// we know what the original opcode was.
for (int i = 0; i < code_len; i++) {
code->_co_monitoring->per_instruction_opcodes[i] = 0;
int opcode = _PyCode_CODE(code)[i].op.code;
code->_co_monitoring->per_instruction_opcodes[i] = _PyOpcode_Deopt[opcode];
}
}
if (multitools && code->_co_monitoring->per_instruction_tools == NULL) {
Expand All @@ -1682,7 +1688,6 @@ update_instrumentation_data(PyCodeObject *code, PyInterpreterState *interp)
PyErr_NoMemory();
return -1;
}
/* This may not be necessary, as we can initialize this memory lazily, but it helps catch errors. */
for (int i = 0; i < code_len; i++) {
code->_co_monitoring->per_instruction_tools[i] = 0;
}
Expand All @@ -1692,17 +1697,10 @@ update_instrumentation_data(PyCodeObject *code, PyInterpreterState *interp)
}

static int
instrument_lock_held(PyCodeObject *code, PyInterpreterState *interp)
force_instrument_lock_held(PyCodeObject *code, PyInterpreterState *interp)
{
ASSERT_WORLD_STOPPED_OR_LOCKED(code);

if (is_version_up_to_date(code, interp)) {
assert(
interp->ceval.instrumentation_version == 0 ||
instrumentation_cross_checks(interp, code)
);
return 0;
}
#ifdef _Py_TIER2
if (code->co_executors != NULL) {
_PyCode_Clear_Executors(code);
Expand Down Expand Up @@ -1769,17 +1767,14 @@ instrument_lock_held(PyCodeObject *code, PyInterpreterState *interp)

// GH-103845: We need to remove both the line and instruction instrumentation before
// adding new ones, otherwise we may remove the newly added instrumentation.

uint8_t removed_line_tools = removed_events.tools[PY_MONITORING_EVENT_LINE];
uint8_t removed_per_instruction_tools = removed_events.tools[PY_MONITORING_EVENT_INSTRUCTION];

if (removed_line_tools) {
_PyCoLineInstrumentationData *line_data = code->_co_monitoring->lines;
for (int i = code->_co_firsttraceable; i < code_len;) {
if (line_data[i].original_opcode) {
if (removed_line_tools) {
remove_line_tools(code, i, removed_line_tools);
}
remove_line_tools(code, i, removed_line_tools);
}
i += _PyInstruction_GetLength(code, i);
}
Expand All @@ -1791,25 +1786,22 @@ instrument_lock_held(PyCodeObject *code, PyInterpreterState *interp)
i += _PyInstruction_GetLength(code, i);
continue;
}
if (removed_per_instruction_tools) {
remove_per_instruction_tools(code, i, removed_per_instruction_tools);
}
remove_per_instruction_tools(code, i, removed_per_instruction_tools);
i += _PyInstruction_GetLength(code, i);
}
}
#ifdef INSTRUMENT_DEBUG
sanity_check_instrumentation(code);
#endif

uint8_t new_line_tools = new_events.tools[PY_MONITORING_EVENT_LINE];
uint8_t new_per_instruction_tools = new_events.tools[PY_MONITORING_EVENT_INSTRUCTION];

if (new_line_tools) {
_PyCoLineInstrumentationData *line_data = code->_co_monitoring->lines;
for (int i = code->_co_firsttraceable; i < code_len;) {
if (line_data[i].original_opcode) {
if (new_line_tools) {
add_line_tools(code, i, new_line_tools);
}
add_line_tools(code, i, new_line_tools);
}
i += _PyInstruction_GetLength(code, i);
}
Expand All @@ -1821,12 +1813,11 @@ instrument_lock_held(PyCodeObject *code, PyInterpreterState *interp)
i += _PyInstruction_GetLength(code, i);
continue;
}
if (new_per_instruction_tools) {
add_per_instruction_tools(code, i, new_per_instruction_tools);
}
add_per_instruction_tools(code, i, new_per_instruction_tools);
i += _PyInstruction_GetLength(code, i);
}
}

done:
FT_ATOMIC_STORE_UINTPTR_RELEASE(code->_co_instrumentation_version,
global_version(interp));
Expand All @@ -1837,6 +1828,22 @@ instrument_lock_held(PyCodeObject *code, PyInterpreterState *interp)
return 0;
}

static int
instrument_lock_held(PyCodeObject *code, PyInterpreterState *interp)
{
ASSERT_WORLD_STOPPED_OR_LOCKED(code);

if (is_version_up_to_date(code, interp)) {
assert(
interp->ceval.instrumentation_version == 0 ||
instrumentation_cross_checks(interp, code)
);
return 0;
}

return force_instrument_lock_held(code, interp);
}

int
_Py_Instrument(PyCodeObject *code, PyInterpreterState *interp)
{
Expand Down Expand Up @@ -1983,16 +1990,8 @@ _PyMonitoring_SetLocalEvents(PyCodeObject *code, int tool_id, _PyMonitoringEvent
goto done;
}
set_local_events(local, tool_id, events);
if (is_version_up_to_date(code, interp)) {
/* Force instrumentation update */
code->_co_instrumentation_version -= MONITORING_VERSION_INCREMENT;
}

#ifdef _Py_TIER2
_Py_Executors_InvalidateDependency(interp, code, 1);
#endif

res = instrument_lock_held(code, interp);
res = force_instrument_lock_held(code, interp);

done:
UNLOCK_CODE();
Expand Down