Skip to content

Behavior change for opcode trace after PEP 669 #103615

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

Closed
gaogaotiantian opened this issue Apr 18, 2023 · 6 comments
Closed

Behavior change for opcode trace after PEP 669 #103615

gaogaotiantian opened this issue Apr 18, 2023 · 6 comments
Assignees
Labels
3.12 only security fixes 3.13 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) triaged The issue has been accepted as valid by a triager. type-bug An unexpected behavior, bug, or error

Comments

@gaogaotiantian
Copy link
Member

gaogaotiantian commented Apr 18, 2023

Before PEP 669, the opcode trace is togglable after trace function being set. So something like

import sys

def opcode_trace_func(frame, event, arg):
    print(frame, event)
    return opcode_trace_func

sys.settrace(opcode_trace_func)
sys._getframe().f_trace = opcode_trace_func
sys._getframe().f_trace_opcodes = True

def f():
    a = [1, 2, 3]
    return a[0]

f()

would work.

After PEP 669, the opcode event is set on sys.settrace() so it's impossible to enable it after enabling the trace.

sys._getframe().f_trace_opcodes = True
sys.settrace(opcode_trace_func)
sys._getframe().f_trace = opcode_trace_func

kind of works but it's super intuitive.

For the current implementation, if we enabled opcode before we do sys.settrace(), it would be "enabled" forever. Meaning the event will be always on, the callback will be always triggered, but frame.f_trace_opcode will be checked in the callback function. Can we always have opcode callback on? It would be a performance hit(extra call for each opcode), but that happens if we want any opcode event at all, and I think that's basically how the old tracing system does it.

Another option would be making f_trace_opcode a descriptor and toggle(actually, probable just enable) opcode events there.

If we don't want to do that at all and just want to break the backward-compatibility because no one is doing insturction trace anyway (I only realized this when syncing #103050 to the lastest change), we should probably document it - make sure the current frame's f_trace_opcode is set before calling sys.settrace() if you want opcode events.

BTW we would've caught this if we already had the instruction level debugging :)

Linked PRs

@gaogaotiantian gaogaotiantian added the type-bug An unexpected behavior, bug, or error label Apr 18, 2023
@mdboom
Copy link
Contributor

mdboom commented Apr 20, 2023

Cc: @markshannon

@gaogaotiantian
Copy link
Member Author

Hi @markshannon , any idea on this issue?

@brandtbucher
Copy link
Member

I think that's basically how the old tracing system does it.

Yeah, but this is globally enabling a more expensive mechanism, forever. No thanks. ;)

Another option would be making f_trace_opcode a descriptor and toggle(actually, probable just enable) opcode events there.

This (enabling opcode tracing for a single code object via a descriptor on the frame) definitely seems like the right approach. It's a shame that we probably can't disable it locally too when the descriptor is set to false (or the frame dies) without a counter somewhere (maybe we can aggressively disable events for each instruction somehow?), but I think that this approach seems much better than enabling super-expensive opcode tracing globally forever just because one function somewhere got clever. :)

@brandtbucher brandtbucher added interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.12 only security fixes triaged The issue has been accepted as valid by a triager. 3.13 bugs and security fixes labels Jun 15, 2023
@gaogaotiantian
Copy link
Member Author

Can we check whether tracing is enabled in sys_trace_instruction_func(the INSTRUCTION callback)? It already checks for frame->f_trace_opcodes anyway. If we can do that (tstate->interp->sys_tracing_threads > 0?), we can get rid of the global instruction events(INSTRUCTION level tracing is toggled per frame anyway). If the user turned off the tracing, it can return DISABLE so it won't be triggered anymore.

@brandtbucher
Copy link
Member

Feel free to try it, but my understanding is that enabling these events globally is incredibly expensive (in terms of the work that needs to be done/undone for every code object that is currently executing or will be executed, as well as wiping out all specializations, allocating arrays for instruction-level metadata, etc.).

Toggling it per-code object and then checking if the frame has opcode tracing enabled in the callback seems a lot slicker to me, but you also seem to have a better idea of how all of the different pieces fit together. I could definitely be missing something. :)

@markshannon
Copy link
Member

FTR, f_trace_opcode was added in #75525 specifically to add a test for #74174. In fact, f_trace_opcode was not used in the tests for the fix, #18334.

I consider adding f_trace_opcode a bit of a mistake, but it is there, so we have to support it.
It will go away when sys.settrace is removed. Hopefully in the not too distant future.

markshannon pushed a commit that referenced this issue Nov 3, 2023
* Use local monitoring for opcode trace

* Remove f_opcode_trace_set

* Add test for setting f_trace_opcodes after settrace
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
* Use local monitoring for opcode trace

* Remove f_opcode_trace_set

* Add test for setting f_trace_opcodes after settrace
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
* Use local monitoring for opcode trace

* Remove f_opcode_trace_set

* Add test for setting f_trace_opcodes after settrace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 only security fixes 3.13 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) triaged The issue has been accepted as valid by a triager. type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants