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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add instruction level tracing via sys.settrace #73586
Comments
I have recently put some effort into developing a code coverage tool that shows correct results for intraline branches. In order to get intraline trace data, I patched CPython, adding an optional "trace instructions" flag to sys.settrace. The patch is fairly simple. http://github.com/gwk/cpython/tree/inst-tracing The crucial detail in ceval is here: @@ -4521,6 +4523,9 @@ maybe_call_line_trace(Py_tracefunc func, PyObject *obj,
frame->f_lineno = line;
result = call_trace(func, obj, tstate, frame, PyTrace_LINE, Py_None);
}
+ else if (tstate->inst_tracing) {
+ result = call_trace(func, obj, tstate, frame, PyTrace_INSTRUCTION, Py_None);
+ } The call to maybe_call_line_trace is guarded by several checks, so I do not see how this would significantly impact performance, except for when tracing is enabled. I built python3.6 using clang -O3 -flto (I saw lots of test failures during the --with-optimizations PGO tests so I opted for a simpler build), with and without my patch. I then ran I would appreciate feedback before going any further with this. I looked at haypo's perf project but was unsure how to use it to run a more detailed comparison.
I am also curious about backwards compatibility. This adds an optional argument to settrace (haypo's recommendation) and some other things that appear to be under Py_LIMITED_API or internal (but I am not sure).
Lastly, I have a few ideas for enhancing this patch that will require some more work on my end to verify their utility.
|
Can you please attach your patch? |
A git style patch can be found at https://github.com/gwk/cpython/commit/071d17cedfdf2db8b405aab5adabebe2ac5ef67b.patch |
Here is the patch from git; if you need a patch for hg I can work on that tomorrow! |
inst-tracing-2.diff:
About the API, I see an issue: the "old=sys.gettrace(); sys.settrace(old)" pattern doesn't work anymore, since old doesn't contain the trace_instructions flag :-/ So I suggest to add a new method and make it exclusive with sys.settrace(), sys.settraceinstr() / sys.gettraceinstr():
The sys module is a critical module, so the API should probably be discussed on python-dev (later?). About performances: it's fine to add anything to maybe_call_line_trace(), it cannot impact performances when tracing is not used. Can you please try to reimplement something like __lltrace__ on top of sys.settraceinstr()? It would be a nice example of the API, but also a good reason to add sys.settraceinstr(). See the issue bpo-25571 "Improve the lltrace feature with the Py_Debug mode". Currently, the __lltrace__ feature requires a Python compiled in debug mode, which is not convenient. Moreover, it requires to put a __ltrace__ variable in global variables. The best would be able to add an instruction level debugger inside pdb! It would be a cool tool to learn Python and to play with the bytecode! |
I do not agree, Python debuggers are already really really slow. They should not have to process 'instruction' trace events as this would happen if George does "Remove the |
Hum, there are two things:
|
Xavier, this is a misunderstanding; sorry for not being more clear. When I said "remove the + if (tstate->inst_tracing) { Line-oriented tracers/debuggers would not have set trace_instructions, so they would not receive any 'instruction' events, and should not suffer. The motivation for this suggestion was that for tracers that *are* interested in instructions, it *might* be simpler to get two calls, one of 'line', and one of 'instruction'. Maybe it's a bad idea though. |
Attached is a demo of using the feature as a fancy replacement for __ltrace__. It demonstrates using a closure for the local trace function to track the previous offset, and prints the offset transitions as src -> dst pairs. This helped me learn a lot about how exception handling works at the bytecode level. |
Thanks to both of you for your feedback. I will take a stab at updating the patch with Victor's suggestions as soon as I can. |
Hum, I don't know well how Python tracing works, but if we had a new |
Thanks for the clarification. It's ok if instruction tracing remains an opt-in feature. As for this suggestion, I am not sure it is useful for debuggers based on the bdb module as trace_dispatch() (the bdb trace function) receives notification of the trace event type as the 'event' argument. |
Attached is a new patch, which does not settrace/gettrace and instead offers new settraceinst/gettraceinst per Victor's recommendation. I did not implement the proposed behavior of raising an exception if the old APIs are used when the inst_tracing flag is set. However I do think it makes sense to do so. The new sys.settraceinst function takes two arguments: a function and an integer flag that is currently interpreted as a boolean (whether or not to enable instruction level tracing). I did it this way because I would like to consider adding a third mode, which would only trigger tracing for interesting control-flow events, namely steps for which the previous instruction was a branch. The motivation is that I expect per-instruction tracing to be very slow, and for the code coverage use case at least, these are the interesting events. For this to be useful, a (prev_offset, current_offset) pair would need to be passed as to the trace callback. I am not sure if this would be useful to other applications, e.g. pdb. |
Hum, let's use https://en.wikipedia.org/wiki/Basic_block terminology. The callback would be called when you enter and exit a basic block? Or only on exit? Only on conditional branches? Or also on unconditional branches? In term of instructions, can give examples of instructions which would be traced or not? IMHO the two instruction level modes would be useful. |
I'm not sure exactly, but the way I see it (for code coverage), we want to trace transitions between basic blocks. So I would define it as: each entry into a BB is traced, with a tuple of (previous_offset, current_offset). This way when a function call starts, we callback with (-1, 0), and then get a callback for every transition between BBs. The code is well covered if we observe every possible transition. I am not clear on the precise details, e.g. regarding unconditional branches. Since for code coverage this is essentially on optimization over per-instruction mode, I intend to first work out correctness details and test cases for the coverage tool, and then once the tests solid add the optimization. I'm happy to discuss more, but this aspect is a lower priority for me (I still have to work out remaining correctness issues with my opcode analysis). Would it make sense to split it out into a second issue? |
See aslo issue bpo-29502: "Should PyObject_Call() call the profiler on C functions, use C_TRACE() macro?" |
After reviewing the thread, I'm reminded that the main design problem concerns preserving behavior of this idiom: If we add more state, i.e. the Here are the options that I see:
As I see it:
Examples of possible future features via keywords:
I intend to prototype one or both of these, but I'm also feeling a bit of time pressure to get the basic feature on track for 3.7. |
I've updated the patch and I think it's ready for a more serious review. A few notes:
The patch can be viewed here: master...gwk:trace-inst Let me know if I should open up an official PR. Also I just filled out the CLA. |
(Also I did prototype instruction filtering but it had mild performance implications when tracing so I have shelved it for the moment.) |
Attached updated demo script. |
The main blocker question is the API change. I started a thread on python-dev to discuss it: |
Nick's implementation of f_trace_lines/f_trace_opcodes serves the same purpose as my proposal, and is a simpler solution so I'm abandoning this patch and working with that feature instead. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: