bpo-31344: Per-frame control of trace events#3417
Conversation
|
Ping @int19h - any thoughts? :) |
zooba
left a comment
zooba
left a comment
There was a problem hiding this comment.
Looks good to me (with a post-8pm caveat)
|
This would potentially come in handy for debuggers; e.g. we could implement per-statement (rather than per-line) stepping with this. |
f_trace_lines: enable/disable line trace events f_trace_opcodes: enable/disable opcode trace events These are intended primarily for testing of the interpreter itself, as they make it much easier to emulate signals arriving at unfortunate times.
ncoghlan
force-pushed
the
bpo-31344-per-opcode-trace-events
branch
from
494f6e7 to
8c9fcba
Compare
gpshead
left a comment
gpshead
left a comment
There was a problem hiding this comment.
overall good, some comments to address.
Doc/reference/datamodel.rst
Outdated
|
|
||
| Implementations *may* allow per-opcode events to be requested by setting | ||
| :attr:`f_trace_opcodes` to :const:`True`. This is intended specifically | ||
| for interpreter signal safety testing, and may have surprising results |
There was a problem hiding this comment.
perhaps change the wording to:
and may result in surprising undefined behavior if an exception escapes the trace function.
i'm trying to make it more explicit that we do not guarantee any behavior when exceptions are allowed to escape during opcode tracing. changing "are raised by" to something involving "escape" makes it clear that exceptions can be raised and handled. the important thing is that they are resolved before the function exits.
| /* The following values are used for 'what' for tracefunc functions: */ | ||
| /* The following values are used for 'what' for tracefunc functions | ||
| * | ||
| * To add a new kind of trace event, also update "trace_init" in |
There was a problem hiding this comment.
I'm gonna guess you learned this the hard way. thanks for the comment! :)
| self.trace_line_events = trace_line_events | ||
| self.trace_opcode_events = trace_opcode_events | ||
| self.events = [] | ||
| def _reconfigure_frame(self, frame): |
There was a problem hiding this comment.
style nit, leave a blank line between methods
| gc.enable() | ||
|
|
||
| @staticmethod | ||
| def make_tracer(): |
There was a problem hiding this comment.
add a comment mentioning why is this needed at all. (letting subclasses such as XYZ override it)
Lib/test/test_sys_settrace.py
Outdated
| def compare_events(self, line_offset, events, expected_events): | ||
| skip_opcode_events = [e for e in events if e[1] != 'opcode'] | ||
| if len(events) > 1: | ||
| self.assertLess(len(skip_opcode_events), len(events)) |
There was a problem hiding this comment.
add a msg= parameter describing why this is a problem if this were to fail.
Doc/reference/datamodel.rst
Outdated
| disabled by setting :attr:`f_trace_lines` to :const:`False`. | ||
|
|
||
| Implementations *may* allow per-opcode events to be requested by setting | ||
| :attr:`f_trace_opcodes` to :const:`True`. This is intended specifically |
There was a problem hiding this comment.
Lets not describe what it is intended for here. that we happen to use it for interpreter signal exception safety testing isn't relevant to someone reading about the API. It'll gain other uses.
|
Merging as the previous Appveyor run already passed: https://ci.appveyor.com/project/python/cpython/build/3.7.0a0.6126 |
7 participants
f_trace_lines: enable/disable line trace events
f_trace_opcodes: enable/disable opcode trace events
These are intended primarily for testing of the interpreter
itself, as they make it much easier to emulate signals
arriving at unfortunate times.
https://bugs.python.org/issue31344