Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
GH-103082: Implementation of PEP 669: Low Impact Monitoring for CPython#103083
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
For the record, I had the same issue when I was using it and I was confused. |
But does it matter? There is no promise that |
brandtbucher commentedApr 11, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This happens basically whenever a module imports another module: >>>importast>>> type(ast.sys)<class'module'>>>>importast.sysTraceback (mostrecentcalllast):File"<stdin>",line1,in<module>ModuleNotFoundError:Nomodulenamed'ast.sys';'ast'isnotapackage 🙃 Perhaps a good mental model for |
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. |
@fabioz Thanks for the reproducer. |
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. |
@markshannon from the fix you seem to have put there, it'd still fail if the user set the tracing to |
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. |
Darn, I was just about to complete my second review.
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Havingstacktop <=0ensuresthatinvalid | |
valuesarenotvisibletothecycleGC. | |
Wechoose-1ratherthan0toassistdebugging.*/ | |
Havingstacktop <=0ensuresthatinvalid | |
valuesarenotvisibletothecycleGC. | |
Wechoose-1ratherthan0toassistdebugging.*/ |
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.
Choose a reason for 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 agoto exit_unwind
on error here.
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.
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.
}; | ||
static inline bool | ||
opcode_has_event(int opcode) { |
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.
opcode_has_event(intopcode) { | |
opcode_has_event(uint8_topcode) | |
{ |
} | ||
static inline bool | ||
is_instrumented(int opcode) { |
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.
is_instrumented(intopcode) { | |
is_instrumented(uint8_topcode) | |
{ |
assert(test); \ | ||
} while (0) | ||
bool valid_opcode(int opcode) { |
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.
boolvalid_opcode(intopcode) { | |
boolvalid_opcode(intopcode) | |
{ |
_PyInterpreterFrame *frame, _Py_CODEUNIT *instr, _Py_CODEUNIT *target | ||
) { |
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.
_PyInterpreterFrame*frame,_Py_CODEUNIT*instr,_Py_CODEUNIT*target | |
){ | |
_PyInterpreterFrame*frame,_Py_CODEUNIT*instr,_Py_CODEUNIT*target) | |
{ |
#define C_RETURN_EVENTS \ | ||
((1 << PY_MONITORING_EVENT_C_RETURN) | \ | ||
(1 << PY_MONITORING_EVENT_C_RAISE)) |
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.
(1 <<PY_MONITORING_EVENT_C_RAISE)) | |
(1 <<PY_MONITORING_EVENT_C_RAISE)) |
interp->monitoring_tool_names[tool_id] == NULL | ||
) { |
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.
interp->monitoring_tool_names[tool_id]==NULL | |
){ | |
interp->monitoring_tool_names[tool_id]==NULL) | |
{ |
else { | ||
return MOST_SIGNIFICANT_BITS[bits]; | ||
} |
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.
else { | |
returnMOST_SIGNIFICANT_BITS[bits]; | |
} | |
returnMOST_SIGNIFICANT_BITS[bits]; |
/* Should use instruction metadata for this */ | ||
static bool | ||
is_super_instruction(int opcode) { |
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.
is_super_instruction(intopcode) { | |
is_super_instruction(uint8_topcode) | |
{ |
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 ;) |
@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). |
bedevere-bot commentedApr 12, 2023
|
… 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 inpython/cpython#103083.There is more to be done to properly support PEP-669, but this makes it compile.See#5450
Uh oh!
There was an error while loading.Please reload this page.
This implements PEP 669.
There are a couple of things missing, but no harm in early review.