Title: Provide a way to get/set PyInterpreterState.frame_eval without needing to access interpreter internals
Type: enhancement Stage:
Components: Interpreter Core Versions: Python 3.8
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Mark.Shannon, brett.cannon, dino.viehland, eric.snow, fabioz, vstinner
Priority: normal Keywords:

Created on 2019-10-16 16:17 by fabioz, last changed 2019-11-11 12:04 by Mark.Shannon.

Messages (16)
msg354803 - (view) Author: Fabio Zadrozny (fabioz) Date: 2019-10-16 16:17
In CPython 3.7 it was possible to do:

#include "pystate.h"
PyThreadState *ts = PyThreadState_Get();
PyInterpreterState *interp = ts->interp;
interp->eval_frame = my_frame_eval_func;

This is no longer possible because in 3.8 the PyInterpreterState is opaque, so, Py_BUILD_CORE_MODULE needs to be defined defined and "internal/pycore_pystate.h" must be included to set PyInterpreterState.eval_frame.

This works but isn't ideal -- maybe there could be a function to set PyInterpreterState.eval_frame?
msg355845 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-11-01 21:24
It depends on how you look at the degree to which you are interacting with the runtime.  This is a fairly low-level hook into the runtime.  So arguably if you are using this API then you should specify being a "core" extension.  That said, getting that clever about it is a bit too much.  The authors or PEP 523 can correct me if I'm wrong, but it seems like there isn't a good reason to restrict access.

So basically, I agree with you. :)

How about one of the following?

* _PyInterpreterState_SetEvalFrame(_PyFrameEvalFunction eval_frame)
* _PyInterpreterState_SetFrameEval(_PyFrameEvalFunction eval_frame)

The underscore basically says "don't use this unless you know what you are doing".  Or perhaps that is overkill too?  "_PyFrameEvalFunction" has an underscore, so perhaps not.

Also, it would make sense to have a matching getter.
msg355849 - (view) Author: Dino Viehland (dino.viehland) * (Python committer) Date: 2019-11-01 22:17
Adding the getter/setters seems perfectly reasonable to me, and I agree they should be underscore prefixed as well.
msg355933 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2019-11-04 10:03
I'm strongly opposed to this change.

PEP 523 does not specify what the semantics of changing the interpreter frame evaluator actually is:
Is the VM obliged to call the new interpreter?
What happens if the custom evaluator leaves the VM in a inconsistent state?
Does the VM have to roll back any speculative optimisations it has made? What happens if it the evaluator is changed multiple times by different modules?
What if the evaluator is changed when a coroutine or generator is suspended, or in another thread?
I could go on...

IMO this punches a big hole in the Python execution model, but provides no benefit.
msg355934 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2019-11-04 10:03

OOI what are you trying achieve by changing the frame evaluator?
msg355936 - (view) Author: Fabio Zadrozny (fabioz) Date: 2019-11-04 11:07
@Mark Shannon what I do is change the code object of the frame about to be evaluated to add a programmatic breakpoint, to avoid the need to have the trace function set at contexts that would need to be traced (after changing the frame.f_code it goes on to call the regular _PyEval_EvalFrameDefault), so, the user-code runs at full speed on all contexts (there's still added overhead on a function call to decide if the code object needs to be changed, but that'd happen on the regular tracing code too).

Note that this does not change the semantics of anything as it calls the regular _PyEval_EvalFrameDefault, so, the worries you're listing shouldn't be a concern in this particular use-case.

Also note that until Python 3.7 this was easy to change, and that's still possible in Python 3.8 (the only thing is that now it's less straightforward).

Note that my use is much simpler that the original intent of the frame evaluator -- my use case could be solved by having a callback to change the code object before the frame execution -- but as far as I know, right now, the way to do that is through the frame evaluation API.
msg355971 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-11-04 18:43
@Mark are you strongly opposed because we're providing an API for changing the eval function in the CPython API and you think it should be in the private API? Or you objecting to PEP 523 all-up (based on your list of objections)? Either way the PEP was accepted and implemented a while ago and so I'm not quite sure what you are expecting as an outcome short of a repeal of PEP 523 which would require a separate PEP.
msg355990 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-05 01:14
> IMO this punches a big hole in the Python execution model, but provides no benefit.

This PEP is about fixing a Python 3.8 regression. In Python 3.7, it was possible to get and set frame_eval. In Python 3.8, it's no longer possible.

One option to fix the regression would be to again expose PyInterpreterState structure... but we are trying to do the opposite: hide more structures, not expose more structures :-/

IMHO private getter and setter functions are perfectly fine. Please ensure that the setter can report an issue. We have too many setters which cannot report an error which is very painful :-(
msg356196 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2019-11-07 16:02
I don't think this is a regression.
`PyThreadState` is an internal opaque data structure, which means we are free to change it.
That the `eval_frame` is hard to access is a feature not a bug, as it discourages misuse and enables us to remove it easily, when a better approach becomes available.

PEP 523 is quite vague, but the rationale indicates that exposing `eval_frame` is for "a method-level JIT". PEP 523 did not suggest adding an API. If it had (and I had had the time) I would have opposed it more vigorously.

IMO, the correct way to change the code object is to set `function.__code__` which can be done easily from either Python or C code.

@Fabioz, is there anything preventing you from doing that?
msg356197 - (view) Author: Fabio Zadrozny (fabioz) Date: 2019-11-07 16:14
@Mark I don't want to change the original function code, I just want to change the code to be executed in the frame (i.e.: as breakpoints change things may be different).

Changing the actual function code is a no-go since changing the real function code can break valid user code.
msg356200 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-07 16:37
By the way, it's still possible to access directly ts->interp if you include "pycore_pystate.h" header, which requires to define Py_BUILD_CORE_MODULE. This header is an internal header.
msg356215 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-11-07 23:13
PEP 523 was to give user code the ability to change the eval function. While the work was motivated by our JIT work, supporting debugging was another motivating factor: There's no C API because at the time there was no need as PyInterpreterState was publicly exposed.

I don't think anyone is suggesting to add something to the stable ABI, so this comes down to whether this should be exposed as part of the CPython API or the publicly accessible internal API. Since there is no distinction of "you probably don't want to use this but we won't yank it out from underneath" you I think this belongs in the CPython API somehow. Otherwise someone should propose withdrawing PEP 523 as I think that shifts what the PEP was aiming for. You can also ask on python-dev or the steering council (which I will abstain myself from due to being a co-author of PEP 523).
msg356216 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-07 23:44
I heard that for debuggers, the PEP 523 is really useful. I recall a huge speedup thanks to this PEP in Jetbrain:
msg356255 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2019-11-08 19:10
It sounds to me like `PyInterpreterState.eval_frame` is being used to lazily modify the bytecode to support breakpoints.

I can see no reason why changing the bytecode can't be done via `function.__code__`.
Suppose the code-object with the breakpoint add is `bcode`, then to turn on the breakpoint:
original_code = f.__code__
f.__code__ = bcode

and to turn it off
f.__code__ = original_code

The JVM supports bytecode instrumentation (via class loaders). It works well, as it provides a clear way for third party tools to modify the behaviour of a particular piece of code without violating any of the invariants of the interpreter. 
We don't really advertise setting `function.__code__` as a way to add low-impact breakpoints or profiling, but it is there.

If this use case is important, and it sounds like it is, then a better option would be to offer library support for adding and removing breakpoints/instrumentation.
This would have the advantage of being composable in a way that changing `PyInterpreterState.eval_frame` is not; in other words, it would be possible for one tool to add profiling and another to add breakpoints and have both work correctly.

I can write up a PEP if necessary.
msg356256 - (view) Author: Fabio Zadrozny (fabioz) Date: 2019-11-08 19:25

I can think of many use-cases which may break if the function code is changed (users can change the code in real-use cases and when they do that they'd loose debugging).

So, as long as the function code is part of the public API of Python, the debugger can't really change it for breakpoints (which is a bit different from the frame code, which the user can't really swap and it's not so common to change).
msg356359 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2019-11-11 12:04

If the user changes the `__code__` attribute of a function then, AFAICT, your debugger does the wrong thing, but bytecode modification does the right thing.

Suppose we have two functions `spam` and `eggs`.
Set a break point in `eggs`, set `spam.__code__ = eggs.__code__`, then call `spam`.
With bytecode modification, we get the correct result. That is, execution breaks in the source code of `eggs` when `spam` is run.
I think your debugger will do the wrong thing as it will execute the original code of `spam`. Could you confirm what it does?

But that's not the main issue, IMO. The big problem is that changing out the interpreter is not composable, unlike bytecode modification.

Suppose we have MyProfiler and YourDebugger.
MyProfiler wants to record calls and YourDebugger wants to support breakpoints.

With bytecode modification, and some care, we can do both.
Swapping out the interpreter is likely to cause all sorts of errors and confusion.
Date User Action Args
2019-11-11 12:04:53Mark.Shannonsetmessages: + msg356359
2019-11-08 19:25:53fabiozsetmessages: + msg356256
2019-11-08 19:10:01Mark.Shannonsetmessages: + msg356255
2019-11-07 23:44:26vstinnersetmessages: + msg356216
2019-11-07 23:13:29brett.cannonsetmessages: + msg356215
2019-11-07 16:37:22vstinnersetmessages: + msg356200
2019-11-07 16:14:46fabiozsetmessages: + msg356197
2019-11-07 16:02:10Mark.Shannonsetmessages: + msg356196
2019-11-05 01:14:22vstinnersetmessages: + msg355990
2019-11-04 18:43:59brett.cannonsetmessages: + msg355971
2019-11-04 11:07:34fabiozsetmessages: + msg355936
2019-11-04 10:03:49Mark.Shannonsetmessages: + msg355934
2019-11-04 10:03:10Mark.Shannonsetnosy: + Mark.Shannon
messages: + msg355933
2019-11-01 22:17:57dino.viehlandsetmessages: + msg355849
2019-11-01 21:24:45eric.snowsetnosy: + brett.cannon, dino.viehland
messages: + msg355845
2019-10-21 14:04:04vstinnersetnosy: + eric.snow
2019-10-16 16:17:06fabiozcreate