GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — GH-103082: Implementation of PEP 669: Low Impact Monitoring for CPython by markshannon · Pull Request #103083 · python/cpython Conversation
|
But does it matter? There is no promise that |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
This happens basically whenever a module imports another module: >>> import ast >>> type(ast.sys) <class 'module'> >>> import ast.sys Traceback (most recent call last): File "<stdin>", line 1, in <module> ModuleNotFoundError: No module named 'ast.sys'; 'ast' is not a package 🙃 Perhaps a good mental model for |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
It is a bit bit weird that I expect to (hopefully before 3.12) use some sort of lazy loading to avoid creating the monitoring object if it isn't needed. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@fabioz Thanks for the reproducer. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
OK. I'm merging this. I think this is stable enough to merge, and we can probably get better bug reports with this merged than on a branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@markshannon from the fix you seem to have put there, it'd still fail if the user set the tracing to |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I think that is the current behavior, is it not? If you restart tracing, then you want a line event for the current line. TBH, it is all an undocumented black box, so some guess work is required. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Darn, I was just about to complete my second review. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| Having stacktop <= 0 ensures that invalid | ||
| values are not visible to the cycle GC. | ||
| We choose -1 rather than 0 to assist debugging. */ |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| Having stacktop <= 0 ensures that invalid | |
| values are not visible to the cycle GC. | |
| We choose -1 rather than 0 to assist debugging. */ | |
| Having stacktop <= 0 ensures that invalid | |
| values are not visible to the cycle GC. | |
| We choose -1 rather than 0 to assist debugging. */ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| DTRACE_FUNCTION_ENTRY(); | ||
| /* Because this avoids the RESUME, | ||
| * we need to update instrumentation */ | ||
| _Py_Instrument(frame->f_code, tstate->interp); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The return value of _Py_Instrument is ignored here. Since it might set an exception, I'd expect a goto exit_unwind on error here.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
We are already handling an exception here, so the exception isn't lost, it replaces the thrown exception.
The question is whether to raise it back to the caller, or to inject into the coroutine/generator?
If unwinding were done in a separate function from evaluation, then the thrown exception would unwind the stack, then the new exception would be raised on continued execution, which would be better.
I'm inclined to leave it for now, and let it get fixed as a side effect of separating evaluation and unwinding.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| }; | ||
|
|
||
| static inline bool | ||
| opcode_has_event(int opcode) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| opcode_has_event(int opcode) { | |
| opcode_has_event(uint8_t opcode) | |
| { |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| } | ||
|
|
||
| static inline bool | ||
| is_instrumented(int opcode) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| is_instrumented(int opcode) { | |
| is_instrumented(uint8_t opcode) | |
| { |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| assert(test); \ | ||
| } while (0) | ||
|
|
||
| bool valid_opcode(int opcode) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| bool valid_opcode(int opcode) { | |
| bool valid_opcode(int opcode) | |
| { |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| _PyInterpreterFrame *frame, _Py_CODEUNIT *instr, _Py_CODEUNIT *target | ||
| ) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| _PyInterpreterFrame *frame, _Py_CODEUNIT *instr, _Py_CODEUNIT *target | |
| ) { | |
| _PyInterpreterFrame *frame, _Py_CODEUNIT *instr, _Py_CODEUNIT *target) | |
| { |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| #define C_RETURN_EVENTS \ | ||
| ((1 << PY_MONITORING_EVENT_C_RETURN) | \ | ||
| (1 << PY_MONITORING_EVENT_C_RAISE)) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| (1 << PY_MONITORING_EVENT_C_RAISE)) | |
| (1 << PY_MONITORING_EVENT_C_RAISE)) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| interp->monitoring_tool_names[tool_id] == NULL | ||
| ) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| interp->monitoring_tool_names[tool_id] == NULL | |
| ) { | |
| interp->monitoring_tool_names[tool_id] == NULL) | |
| { |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| else { | ||
| return MOST_SIGNIFICANT_BITS[bits]; | ||
| } |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| else { | |
| return MOST_SIGNIFICANT_BITS[bits]; | |
| } | |
| return MOST_SIGNIFICANT_BITS[bits]; |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| /* Should use instruction metadata for this */ | ||
| static bool | ||
| is_super_instruction(int opcode) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| is_super_instruction(int opcode) { | |
| is_super_instruction(uint8_t opcode) | |
| { |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Ok, I'll try another round of the debugger tests to see if there's more breakage -- that previous issue didn't really let me get further, so, I'll check and report back -- with bugs in the tracker if that's the case I guess ;) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@markshannon I just checked and changing the tracing shouldn't duplicate line events in the new tracer (it should only report when the line actually changes). I created #103471 with a test case for this (which works in Python 3.11 and fails in the current master). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
… CPython (pythonGH-103083) * The majority of the monitoring code is in instrumentation.c * The new instrumentation bytecodes are in bytecodes.c * legacy_tracing.c adapts the new API to the old sys.setrace and sys.setprofile APIs
…here it was removed from the struct. See PEP-669 (https://peps.python.org/pep-0669/) and the implementation in python/cpython#103083. There is more to be done to properly support PEP-669, but this makes it compile. See #5450
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.