Skip to content

GH-108976. Keep monitoring data structures valid during de-optimization during callback. #109131

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
merged 2 commits into from
Sep 11, 2023
Merged

GH-108976. Keep monitoring data structures valid during de-optimization during callback. #109131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe72832
Make sure that monitoring data structures remain valid during de-opti…
markshannon Sep 6, 2023
1b73f79
Add news
markshannon Sep 6, 2023
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
8 changes: 8 additions & 0 deletions Lib/test/test_monitoring.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1719,6 +1719,14 @@ def make_foo_optimized_then_set_event():
finally:
sys.monitoring.set_events(TEST_TOOL, 0)

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 TestOptimizer(MonitoringTestBase, unittest.TestCase):

Expand Down
18 changes: 18 additions & 0 deletions Lib/test/test_pdb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2297,6 +2297,24 @@ 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'
> <doctest test.test_pdb.test_pdb_issue_gh_108976[0]>(5)test_function()
-> a = 1
(Pdb) continue
"""

def test_pdb_ambiguous_statements():
"""See GH-104301

Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2023-09-06-22-50-25.gh-issue-108976.MUKaIJ.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix crash that occurs after de-instrumenting a code object in a monitoring
callback.
106 changes: 51 additions & 55 deletions Python/instrumentation.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,45 +364,21 @@ dump_instrumentation_data_per_instruction(PyCodeObject *code, _PyCoMonitoringDat
}

static void
dump_monitors(const char *prefix, _Py_Monitors monitors, FILE*out)
dump_global_monitors(const char *prefix, _Py_GlobalMonitors monitors, FILE*out)
{
fprintf(out, "%s monitors:\n", prefix);
for (int event = 0; event < _PY_MONITORING_UNGROUPED_EVENTS; event++) {
fprintf(out, " Event %d: Tools %x\n", event, monitors.tools[event]);
}
}

/* Like _Py_GetBaseOpcode but without asserts.
* Does its best to give the right answer, but won't abort
* if something is wrong */
static int
get_base_opcode_best_attempt(PyCodeObject *code, int offset)
static void
dump_local_monitors(const char *prefix, _Py_LocalMonitors monitors, FILE*out)
{
int opcode = _Py_OPCODE(_PyCode_CODE(code)[offset]);
if (INSTRUMENTED_OPCODES[opcode] != opcode) {
/* Not instrumented */
return _PyOpcode_Deopt[opcode] == 0 ? opcode : _PyOpcode_Deopt[opcode];
}
if (opcode == INSTRUMENTED_INSTRUCTION) {
if (code->_co_monitoring->per_instruction_opcodes[offset] == 0) {
return opcode;
}
opcode = code->_co_monitoring->per_instruction_opcodes[offset];
}
if (opcode == INSTRUMENTED_LINE) {
if (code->_co_monitoring->lines[offset].original_opcode == 0) {
return opcode;
}
opcode = code->_co_monitoring->lines[offset].original_opcode;
}
int deinstrumented = DE_INSTRUMENT[opcode];
if (deinstrumented) {
return deinstrumented;
}
if (_PyOpcode_Deopt[opcode] == 0) {
return opcode;
fprintf(out, "%s monitors:\n", prefix);
for (int event = 0; event < _PY_MONITORING_LOCAL_EVENTS; event++) {
fprintf(out, " Event %d: Tools %x\n", event, monitors.tools[event]);
}
return _PyOpcode_Deopt[opcode];
}

/* No error checking -- Don't use this for anything but experimental debugging */
Expand All @@ -417,9 +393,9 @@ dump_instrumentation_data(PyCodeObject *code, int star, FILE*out)
fprintf(out, "NULL\n");
return;
}
dump_monitors("Global", _PyInterpreterState_GET()->monitors, out);
dump_monitors("Code", data->local_monitors, out);
dump_monitors("Active", data->active_monitors, out);
dump_global_monitors("Global", _PyInterpreterState_GET()->monitors, out);
dump_local_monitors("Code", data->local_monitors, out);
dump_local_monitors("Active", data->active_monitors, out);
int code_len = (int)Py_SIZE(code);
bool starred = false;
for (int i = 0; i < code_len; i += _PyInstruction_GetLength(code, i)) {
Expand Down Expand Up @@ -458,6 +434,9 @@ dump_instrumentation_data(PyCodeObject *code, int star, FILE*out)
static bool
valid_opcode(int opcode)
{
if (opcode == INSTRUMENTED_LINE) {
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to make the generator add INSTRUMENTED_LINE to _PyOpcode_opcode_metadata

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be, but this PR needs backporting to 3.12 and it's already quite large for a backport this late in the release cycle.

if (IS_VALID_OPCODE(opcode) &&
opcode != CACHE &&
opcode != RESERVED &&
Expand All @@ -475,18 +454,23 @@ sanity_check_instrumentation(PyCodeObject *code)
if (data == NULL) {
return;
}
_Py_GlobalMonitors active_monitors = _PyInterpreterState_GET()->monitors;
_Py_GlobalMonitors global_monitors = _PyInterpreterState_GET()->monitors;
_Py_LocalMonitors active_monitors;
if (code->_co_monitoring) {
_Py_Monitors local_monitors = code->_co_monitoring->local_monitors;
active_monitors = local_union(active_monitors, local_monitors);
_Py_LocalMonitors local_monitors = code->_co_monitoring->local_monitors;
active_monitors = local_union(global_monitors, local_monitors);
}
else {
_Py_LocalMonitors empty = (_Py_LocalMonitors) { 0 };
active_monitors = local_union(global_monitors, empty);
}
assert(monitors_equals(
code->_co_monitoring->active_monitors,
active_monitors)
);
active_monitors));
int code_len = (int)Py_SIZE(code);
for (int i = 0; i < code_len;) {
int opcode = _PyCode_CODE(code)[i].op.code;
_Py_CODEUNIT *instr = &_PyCode_CODE(code)[i];
int opcode = instr->op.code;
int base_opcode = _Py_GetBaseOpcode(code, i);
CHECK(valid_opcode(opcode));
CHECK(valid_opcode(base_opcode));
Expand All @@ -506,23 +490,30 @@ sanity_check_instrumentation(PyCodeObject *code)
opcode = data->lines[i].original_opcode;
CHECK(opcode != END_FOR);
CHECK(opcode != RESUME);
CHECK(opcode != RESUME_CHECK);
CHECK(opcode != INSTRUMENTED_RESUME);
if (!is_instrumented(opcode)) {
CHECK(_PyOpcode_Deopt[opcode] == opcode);
}
CHECK(opcode != INSTRUMENTED_LINE);
}
else if (data->lines && !is_instrumented(opcode)) {
CHECK(data->lines[i].original_opcode == 0 ||
data->lines[i].original_opcode == base_opcode ||
DE_INSTRUMENT[data->lines[i].original_opcode] == base_opcode);
else if (data->lines) {
/* If original_opcode is INSTRUMENTED_INSTRUCTION
* *and* we are executing a INSTRUMENTED_LINE instruction
* that has de-instrumented itself, then we will execute
* an invalid INSTRUMENTED_INSTRUCTION */
CHECK(data->lines[i].original_opcode != INSTRUMENTED_INSTRUCTION);
}
if (opcode == INSTRUMENTED_INSTRUCTION) {
CHECK(data->per_instruction_opcodes[i] != 0);
opcode = data->per_instruction_opcodes[i];
}
if (is_instrumented(opcode)) {
CHECK(DE_INSTRUMENT[opcode] == base_opcode);
int event = EVENT_FOR_OPCODE[DE_INSTRUMENT[opcode]];
if (event < 0) {
/* RESUME fixup */
event = _PyCode_CODE(code)[i].op.arg;
event = instr->op.arg ? 1: 0;
}
CHECK(active_monitors.tools[event] != 0);
}
Expand Down Expand Up @@ -608,30 +599,30 @@ static void
de_instrument_line(PyCodeObject *code, int i)
{
_Py_CODEUNIT *instr = &_PyCode_CODE(code)[i];
uint8_t *opcode_ptr = &instr->op.code;
int opcode =*opcode_ptr;
int opcode = instr->op.code;
if (opcode != INSTRUMENTED_LINE) {
return;
}
_PyCoLineInstrumentationData *lines = &code->_co_monitoring->lines[i];
int original_opcode = lines->original_opcode;
if (original_opcode == INSTRUMENTED_INSTRUCTION) {
lines->original_opcode = code->_co_monitoring->per_instruction_opcodes[i];
}
CHECK(original_opcode != 0);
CHECK(original_opcode == _PyOpcode_Deopt[original_opcode]);
*opcode_ptr = instr->op.code = original_opcode;
instr->op.code = original_opcode;
if (_PyOpcode_Caches[original_opcode]) {
instr[1].cache = adaptive_counter_warmup();
}
assert(*opcode_ptr != INSTRUMENTED_LINE);
assert(instr->op.code != INSTRUMENTED_LINE);
}


static void
de_instrument_per_instruction(PyCodeObject *code, int i)
{
_Py_CODEUNIT *instr = &_PyCode_CODE(code)[i];
uint8_t *opcode_ptr = &instr->op.code;
int opcode =*opcode_ptr;
int opcode = *opcode_ptr;
if (opcode == INSTRUMENTED_LINE) {
opcode_ptr = &code->_co_monitoring->lines[i].original_opcode;
opcode = *opcode_ptr;
Expand All @@ -642,10 +633,11 @@ de_instrument_per_instruction(PyCodeObject *code, int i)
int original_opcode = code->_co_monitoring->per_instruction_opcodes[i];
CHECK(original_opcode != 0);
CHECK(original_opcode == _PyOpcode_Deopt[original_opcode]);
instr->op.code = original_opcode;
*opcode_ptr = original_opcode;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to do this through a pointer?

Copy link
Member Author

@markshannon markshannon Sep 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If we have INSTRUMENTED_LINE -> INSTRUMENTED_INSTRUCTION -> NORMAL_OPCODE,
then we want to change the original_opcode for INSTRUMENTED_LINE, not the original instruction.
That way we end up with INSTRUMENTED_LINE -> NORMAL_OPCODE, not just NORMAL_OPCODE.

This fixes #109156 as well, I think.

if (_PyOpcode_Caches[original_opcode]) {
instr[1].cache = adaptive_counter_warmup();
Copy link
Member

Choose a reason for hiding this comment

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

The cache would be initialized here even if the instr is INSTRUMENTED_LINE (but the original_opcode is an instruction with cache). I guess that's no harm?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is indeed harmless.

}
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 @@ -685,7 +677,7 @@ static void
instrument_line(PyCodeObject *code, int i)
{
uint8_t *opcode_ptr = &_PyCode_CODE(code)[i].op.code;
int opcode =*opcode_ptr;
int opcode = *opcode_ptr;
if (opcode == INSTRUMENTED_LINE) {
return;
}
Expand All @@ -700,13 +692,14 @@ instrument_per_instruction(PyCodeObject *code, int i)
{
_Py_CODEUNIT *instr = &_PyCode_CODE(code)[i];
uint8_t *opcode_ptr = &instr->op.code;
int opcode =*opcode_ptr;
int opcode = *opcode_ptr;
if (opcode == INSTRUMENTED_LINE) {
_PyCoLineInstrumentationData *lines = &code->_co_monitoring->lines[i];
opcode_ptr = &lines->original_opcode;
opcode = *opcode_ptr;
}
if (opcode == INSTRUMENTED_INSTRUCTION) {
assert(code->_co_monitoring->per_instruction_opcodes[i] > 0);
return;
}
CHECK(opcode != 0);
Expand Down Expand Up @@ -1133,7 +1126,6 @@ _Py_call_instrumentation_line(PyThreadState *tstate, _PyInterpreterFrame* frame,

_PyCoMonitoringData *monitoring = code->_co_monitoring;
_PyCoLineInstrumentationData *line_data = &monitoring->lines[i];
uint8_t original_opcode = line_data->original_opcode;
if (tstate->tracing) {
goto done;
}
Expand Down Expand Up @@ -1216,7 +1208,9 @@ _Py_call_instrumentation_line(PyThreadState *tstate, _PyInterpreterFrame* frame,
}
} while (tools);
Py_DECREF(line_obj);
uint8_t original_opcode;
Copy link
Member

Choose a reason for hiding this comment

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

why not inline the declaration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because C doesn't allow a declaration immediately following a label.
For arcane historical reasons, probably.

done:
original_opcode = line_data->original_opcode;
assert(original_opcode != 0);
assert(original_opcode != INSTRUMENTED_LINE);
assert(_PyOpcode_Deopt[original_opcode] == original_opcode);
Expand Down Expand Up @@ -1659,7 +1653,9 @@ _Py_Instrument(PyCodeObject *code, PyInterpreterState *interp)
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];

Expand Down