gh-107265: Remove all ENTER_EXECUTOR when execute _Py_Instrument#108539
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1479,6 +1479,23 @@ clear_executors(PyCodeObject *co) | ||
| co->co_executors = NULL; | ||
| } | ||
| void | ||
| _PyCode_Clear_Executors(PyCodeObject *code){ | ||
| int code_len = (int)Py_SIZE(code); | ||
| for (int i = 0; i < code_len; i += _PyInstruction_GetLength(code, i)){ | ||
| _Py_CODEUNIT *instr = &_PyCode_CODE(code)[i]; | ||
| uint8_t opcode = instr->op.code; | ||
| uint8_t oparg = instr->op.arg; | ||
| if (opcode == ENTER_EXECUTOR){ | ||
| _PyExecutorObject *exec = code->co_executors->executors[oparg]; | ||
| assert(exec->vm_data.opcode != ENTER_EXECUTOR); | ||
| instr->op.code = exec->vm_data.opcode; | ||
| instr->op.arg = exec->vm_data.oparg; | ||
| } | ||
| } | ||
| clear_executors(code); | ||
gvanrossum marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| static void | ||
| deopt_code(PyCodeObject *code, _Py_CODEUNIT *instructions) | ||
| { | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -3,6 +3,7 @@ | ||
| #include "opcode_ids.h" | ||
| #include "pycore_call.h" | ||
| #include "pycore_code.h" // _PyCode_Clear_Executors() | ||
| #include "pycore_frame.h" | ||
| #include "pycore_interp.h" | ||
| #include "pycore_long.h" | ||
| @@ -583,13 +584,7 @@ de_instrument(PyCodeObject *code, int i, int event) | ||
| _Py_CODEUNIT *instr = &_PyCode_CODE(code)[i]; | ||
| uint8_t *opcode_ptr = &instr->op.code; | ||
| int opcode = *opcode_ptr; | ||
| assert(opcode != ENTER_EXECUTOR); | ||
| if (opcode == INSTRUMENTED_LINE){ | ||
| opcode_ptr = &code->_co_monitoring->lines[i].original_opcode; | ||
| opcode = *opcode_ptr; | ||
| @@ -734,22 +729,7 @@ remove_tools(PyCodeObject * code, int offset, int event, int tools) | ||
| assert(event != PY_MONITORING_EVENT_LINE); | ||
| assert(event != PY_MONITORING_EVENT_INSTRUCTION); | ||
| assert(PY_MONITORING_IS_INSTRUMENTED_EVENT(event)); | ||
| assert(opcode_has_event(_Py_GetBaseOpcode(code, offset))); | ||
| _PyCoMonitoringData *monitoring = code->_co_monitoring; | ||
| if (monitoring && monitoring->tools){ | ||
| monitoring->tools[offset] &= ~tools; | ||
| @@ -1315,16 +1295,10 @@ initialize_tools(PyCodeObject *code) | ||
| for (int i = 0; i < code_len; i++){ | ||
| _Py_CODEUNIT *instr = &_PyCode_CODE(code)[i]; | ||
| int opcode = instr->op.code; | ||
| assert(opcode != ENTER_EXECUTOR); | ||
| if (opcode == INSTRUMENTED_LINE){ | ||
| opcode = code->_co_monitoring->lines[i].original_opcode; | ||
| } | ||
| bool instrumented = is_instrumented(opcode); | ||
| if (instrumented){ | ||
| opcode = DE_INSTRUMENT[opcode]; | ||
| @@ -1335,7 +1309,7 @@ initialize_tools(PyCodeObject *code) | ||
| if (instrumented){ | ||
| int8_t event; | ||
| if (opcode == RESUME){ | ||
| event = instr->op.arg != 0; | ||
| } | ||
| else{ | ||
| event = EVENT_FOR_OPCODE[opcode]; | ||
| @@ -1588,6 +1562,9 @@ _Py_Instrument(PyCodeObject *code, PyInterpreterState *interp) | ||
| ); | ||
| return 0; | ||
| } | ||
| if (code->co_executors != NULL){ | ||
| _PyCode_Clear_Executors(code); | ||
gvanrossum marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| int code_len = (int)Py_SIZE(code); | ||
| /* code->_co_firsttraceable >= code_len indicates | ||
| * that no instrumentation can be inserted. | ||
| @@ -1629,7 +1606,9 @@ _Py_Instrument(PyCodeObject *code, PyInterpreterState *interp) | ||
| for (int i = code->_co_firsttraceable; i < code_len; i+= _PyInstruction_GetLength(code, i)){ | ||
| _Py_CODEUNIT *instr = &_PyCode_CODE(code)[i]; | ||
| CHECK(instr->op.code != 0); | ||
| assert(instr->op.code != ENTER_EXECUTOR); | ||
| int base_opcode = _Py_GetBaseOpcode(code, i); | ||
| assert(base_opcode != ENTER_EXECUTOR); | ||
| if (opcode_has_event(base_opcode)){ | ||
| int8_t event; | ||
| if (base_opcode == RESUME){ | ||
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markshannon cc @gvanrossum
I added a test, but not sure what you wanted :)
Without this PR, Python/instrumentation.c#L1589 will not be able to pass the assert through this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this test supposed to do? When I copy this test into the main branch, it passes (in debug mode, so failing asserts would cause crashes), so I don't think this proves your fix works.
By adding some
dis.dis(test_func, adaptive=True)calls to the test I think I see the difference -- in main, an ENTER_EXECUTOR opcode remains in the code object, whereas with this PR, the ENTER_EXECUTOR opcode appears after the function is called, but disappears again after the second set_local_events call.You could check for that ENTER_EXECUTOR (there are examples in test_capi/test_misc.py) but it would be nice if you had example code that actually crashed or triggered an assert (in debug mode) in main without your fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm,, interesting, It looks like I watched the Hallucination when I wrote the test in the first place :(
I will soon update the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: #108539 (comment)