Skip to content
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

PEP 523: Add private _PyInterpreterState_SetEvalFrameFunc() function to the C API (Python 3.8 regression) #82681

Closed
fabioz mannequin opened this issue Oct 16, 2019 · 46 comments
Labels
3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@fabioz
Copy link
Mannequin

fabioz mannequin commented Oct 16, 2019

BPO 38500
Nosy @brettcannon, @gpshead, @vstinner, @fabioz, @phsilva, @DinoV, @ambv, @markshannon, @ericsnowcurrently
PRs
  • bpo-38818: PyInterpreterState.eval_frame now pass tstate #17187
  • bpo-38500: Add _PyInterpreterState_SetEvalFrameFunc() #17340
  • bpo-38500: Add PyInterpreterState_SetEvalFrameFunc() #17352
  • 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:

    assignee = None
    closed_at = <Date 2020-03-19.03:26:02.766>
    created_at = <Date 2019-10-16.16:17:06.643>
    labels = ['interpreter-core', 'type-feature', '3.9']
    title = 'PEP 523: Add private _PyInterpreterState_SetEvalFrameFunc() function to the C API (Python 3.8 regression)'
    updated_at = <Date 2020-03-19.03:26:02.765>
    user = 'https://github.com/fabioz'

    bugs.python.org fields:

    activity = <Date 2020-03-19.03:26:02.765>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-03-19.03:26:02.766>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2019-10-16.16:17:06.643>
    creator = 'fabioz'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 38500
    keywords = ['patch', '3.8regression']
    message_count = 46.0
    messages = ['354803', '355845', '355849', '355933', '355934', '355936', '355971', '355990', '356196', '356197', '356200', '356215', '356216', '356255', '356256', '356359', '356483', '356963', '356981', '356983', '357085', '357173', '357175', '357183', '357208', '357212', '357245', '357277', '357281', '357308', '357310', '357461', '363833', '363834', '363835', '363837', '363838', '364009', '364017', '364030', '364032', '364035', '364036', '364046', '364047', '364055']
    nosy_count = 9.0
    nosy_names = ['brett.cannon', 'gregory.p.smith', 'vstinner', 'fabioz', 'phsilva', 'dino.viehland', 'lukasz.langa', 'Mark.Shannon', 'eric.snow']
    pr_nums = ['17187', '17340', '17352']
    priority = 'critical'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue38500'
    versions = ['Python 3.9']

    @fabioz
    Copy link
    Mannequin Author

    fabioz mannequin commented Oct 16, 2019

    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?

    @fabioz fabioz mannequin added 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels Oct 16, 2019
    @ericsnowcurrently
    Copy link
    Member

    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.

    @DinoV
    Copy link
    Contributor

    DinoV commented Nov 1, 2019

    Adding the getter/setters seems perfectly reasonable to me, and I agree they should be underscore prefixed as well.

    @markshannon
    Copy link
    Member

    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.

    @markshannon
    Copy link
    Member

    Fabio,

    OOI what are you trying achieve by changing the frame evaluator?

    @fabioz
    Copy link
    Mannequin Author

    fabioz mannequin commented Nov 4, 2019

    @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.

    @brettcannon
    Copy link
    Member

    @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.

    @vstinner
    Copy link
    Member

    vstinner commented Nov 5, 2019

    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 :-(

    @markshannon
    Copy link
    Member

    Victor,
    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?

    @fabioz
    Copy link
    Mannequin Author

    fabioz mannequin commented Nov 7, 2019

    @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.

    @vstinner
    Copy link
    Member

    vstinner commented Nov 7, 2019

    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.

    @brettcannon
    Copy link
    Member

    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: https://www.python.org/dev/peps/pep-0523/#debugging. 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).

    @vstinner
    Copy link
    Member

    vstinner commented Nov 7, 2019

    I heard that for debuggers, the PEP-523 is really useful. I recall a huge speedup thanks to this PEP in Jetbrain:
    https://blog.jetbrains.com/pycharm/2017/03/inside-the-debugger-interview-with-elizaveta-shashkova/

    @markshannon
    Copy link
    Member

    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.

    @fabioz
    Copy link
    Mannequin Author

    fabioz mannequin commented Nov 8, 2019

    @mark

    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).

    @markshannon
    Copy link
    Member

    Fabio,

    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.

    @brettcannon
    Copy link
    Member

    I would like to bring this issue back on topic as this about how to expose PEP-523 support in Python 3.8, not whether Fabio should be using a different approach when he has been something sanctioned in an accepted PEP and was previously doable that was (accidentally) taken away from him.

    If people want to do a separate PEP to roll back PEP-523 that's fine, but until that occurs I think we should move forward with the fact that PEP-523 is active.

    @fabioz
    Copy link
    Mannequin Author

    fabioz mannequin commented Nov 19, 2019

    @mark @brett

    Well, PEP-523 still works (it's just more inconvenient to use now).

    Still, if PEP-523 will still be supported I think that having the setter/getter makes sense.

    If it is to be deprecated as @mark is suggesting it doesn't really make sense to add it (but then, it should really be deprecated and ideally there'd be some replacement for the current debugger use... not sure about other use cases such as a jit, which was the initial target of PEP-523 -- @mark, do you want to go that route/create a PEP to deprecate it so that this discussion takes place in a proper place?).

    p.s.: as a note, bytecode modification on the actual object is not a usable approach for the debugger because users could break that in real-world use cases (i.e.: what happens if the user creates a **new** code and sets it to the code which had the breakpoint? What about breakpoint changes? Right now the debugger evaluates all assumptions just before the frame is executed, so, it's easier to get things right -- the case you posted currently does what's expected on pydevd). Still, a callback before the execution so that it could change the frame code before it's executed without the remainder of PEP-523 would be enough (and maybe it could be adopted in other Python implementations too) -- actually, for the debugger it'd be awesome if the frame code could be changed from inside a trace call and then that stack would restart execution (close to what happens with setting the frame line to be executed), but I guess this would be a completely different request ;)

    p.s.: please don't reply to my previous p.s. here (let's move the discussion to another place -- either by @mark creating a PEP for discussion or acknowledging the issue is ok given the current status quo).

    @brettcannon
    Copy link
    Member

    I think the real question is whether this is part of the CPython public API or the CPython internal API.

    @fabio how burdensome would it be if we placed this in the internal API that you can get access to but we don't make backwards-compatibility guarantees about? For instance, Victor wants to start passing in thread state which will change the API for 3.9, but we wouldn't expect to change it in a bugfix release.

    @fabioz
    Copy link
    Mannequin Author

    fabioz mannequin commented Nov 19, 2019

    @bret

    I don't really see a problem in breaking the API in major releases (so, having access for it in the internal API but without a backwards-compatibility guarantee seems like a good fit for me).

    @brettcannon
    Copy link
    Member

    OK, then my vote is to provide getter and setter methods that are underscore-prefixed to mark them as "internal" with clear comments specifying that they are part of PEP-523 and thus should not be needlessly broken in a bugfix release.

    @markshannon
    Copy link
    Member

    Brett,
    PEP-523 makes no mention of adding a getter or setter.
    Adding them is a big change to Python semantics and shouldn't, IMO, be done without a PEP that explicit states they are going to be added.

    @markshannon
    Copy link
    Member

    Fabio,

    Can you give me a specific example where changing the bytecode via the __code__ attribute of a function does not work as expected?

    I am assuming that adding a breakpoint on a line is equivalent to adding breakpoint(); at the beginning of that line. If the bytecode of a function is modified dynamically, say by a decorator, then it is unclear what should be done, and it is hard to claim that any particular approach is more correct.
    However, assuming that the change is well defined and keeps the mapping to the original code, then adding a breakpoint to the modified code should work just as well before or after modification.

    As an example, consider http://code.activestate.com/recipes/277940-decorator-for-bindingconstants-at-compile-time/ which provides a means of converting global variable reads to constants. In that case the order in which the decorator and breakpoint insertion are applied shouldn't matter.

    I propose a new method on code objects withCallAtLine(callable: Callable[], line:int) which returns a new code object with calls to the given callable at the given line.
    A breakpoint can then be inserted at line L in function f with f.__code__ = f.__code__.withCallAtLine(sys.breakpoint, L).

    @fabioz
    Copy link
    Mannequin Author

    fabioz mannequin commented Nov 21, 2019

    @mark

    First you have to explain to me how you envision changing the method code reliably in the debugger...

    Import hooks don't work (they'd break with something as simple as the code below)

    def method():
       a = 10
    
    mod = reload(old_mod)
    old_mod.method.__code__ = mod.method.__code__

    using the tracing also doesn't work (it's too late to change the code)

    Note: consider the reload just an example, not the only use case (say, the user could pickle code objects to remote execution and the debugger should still work).

    Also, consider you have to change the bytecode of methods which are only internal to a function (and thus can't be accessed from the outside).

    Then, if you have a reliable way to do it, how do you keep track of those code objects to reapply the patching when breakpoints change? What if it adds a breakpoint to a new method, how do you locate it? Creating strong references to methods isn't an option because it would prevent things from being garbage collected (and you'd have to track all objects containing code objects for it to be reliable).

    As a note, pydevd does have some support for hot-reloading, but there are too many corner-cases and it doesn't work 100% for live coding (it's an unsolved problem is Python) -- and I can't really envision it working for regular breakpoints as there are too many corner cases to handle.

    @brettcannon
    Copy link
    Member

    @mark

    PEP-523 makes no mention of adding a getter or setter.
    Adding them is a big change to Python semantics and shouldn't, IMO, be done without a PEP that explicit states they are going to be added.

    Adding getters or setters for something that was previously doable that we accidentally took away from users is not an expansion of semantics; it's fixing a backwards-compatibility break in a way that lets us keep the goal of making PyInterpreterState opaque.

    I'm considering this issue at a stalemate and so I'm going to loop in python-dev to help settle this. If that still locks up then we can bring this to the steering council.

    @brettcannon
    Copy link
    Member

    @gpshead
    Copy link
    Member

    gpshead commented Nov 22, 2019

    +1 to re-exposing a way to do PEP-523.

    PEP-523 added a public API, we unintentionally hid it behind the mask of Py_BUILD_CORE_MODULE in 3.8. We shouldn't remove PEP-523's abilities without a deprecation cycle. But given the use cases tend to be "deep" knowledge places that can adapt to API change and version checks, I think re-exposing it without the need for Py_BUILD_CORE_MODULE is reasonable.

    Are you proposing this for 3.8.1?

    Or is 3.8 burned for this 3.6 & 3.7 feature :sadface: (but we know how to work around it - so if our RM says it is, we'll deal) and re-exposing in some manner it only happens in 3.9?

    @gpshead gpshead added the 3.9 only security fixes label Nov 22, 2019
    @vstinner
    Copy link
    Member

    I wrote PR 17340 to add Add PyInterpreterState_GetEvalFrameFunc() and PyInterpreterState_SetEvalFrameFunc() functions.

    Since the PEP-523 has been approved, for me, these functions must be public, not private. My implementation adds these functions to Include/cpython/pystate.h: "non-portable" API specific to CPython (exclude from the limited C API).

    PyInterpreterState_GetEvalFrameFunc() is needed if you want to inject a "hook" and not really replaced the whole function. For example, execute code before or after the call. I guess that PyCharm debugger works like that.

    @vstinner
    Copy link
    Member

    New question: if we add a public API to Python 3.8.1, would it make sense to directly change eval_frame type to add a new "tstate" parameter? See bpo-38818 for the rationale.

    I already proposed a change for the master branch: #17187

    Maybe I should combine PR 17340 (add PyInterpreterState_SetEvalFrameFunc()) with PR 17187 (add tstate parameter).

    @brettcannon
    Copy link
    Member

    @greg I hope this goes into 3.8.1 since it was a breaking change.

    @victor if we are going to ask folks to start using a setter and getter I say we might as well get it right the first time and start taking the tstate now.

    @vstinner
    Copy link
    Member

    @victor if we are going to ask folks to start using a setter and getter I say we might as well get it right the first time and start taking the tstate now.

    Done: New PR 17352 combines my two other PRs.

    @vstinner
    Copy link
    Member

    Fabio: "I don't really see a problem in breaking the API in major releases (so, having access for it in the internal API but without a backwards-compatibility guarantee seems like a good fit for me)."

    In Python, the internal C API now means that you have to define Py_BUILD_CORE_MODULE macro and include a special "internal/pycore_pystate.h" header file.

    If you are fine with doing that in PyCharm, what is the purpose of this issue?

    Do you mean in *internal* API or a *private* API? Private API is basically a public API with weaker backward compatibility warranties and with a "_" prefix (_Py, _PY prefix).

    @vstinner
    Copy link
    Member

    I marked bpo-38818 "Modify PyInterpreterState.eval_frame to pass tstate (PyThreadState)" as a duplicate of this issue.

    It has been said earlier that if we choose to add a public C API to set the frame evaluation function, it's better to design it properly from the start: so include tstate.

    See https://vstinner.github.io/cpython-pass-tstate.html for the rationale behind passing tstate everywhere. See also multiple discussions on python-dev about passing explicitly tstate.

    @vstinner
    Copy link
    Member

    I plan to merge my PR 17340 at the end of week to not miss Python 3.9 feature freeze deadline, unless someone speaks up and find a good reason to not merge this PR. The PR adds a public C API PyInterpreterState_SetEvalFrameFunc() and now pass tstate to the frame evaluation function.

    --

    Mark Shannon is against the idea of providing a way to set the frame evaluation function (PEP-523), but Dino Viehland, Eric Snow, Brett Cannon and me want to provide a C API for that.

    --

    I propose to properly fix this issue in Python 3.9:

    • Add a public C API to get and set the frame evaluation function
    • Pass tstate to this frame evaluation function

    Pass tstate is a backward incompatible change. But it's unclear to me if its API was part of the "public" C API in Python 3.7. In Python 3.8, PyInterpreterState structure moved to the internal C API which is clearly excluded from backward compatibility warranties of the public C API.

    Anyway, I expect that they are less than 10 projects in the world which use the frame evaluation function, which it should be doable to update all of them to support the C API for Python 3.9 that I'm proposing.

    --

    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.

    The status quo is that Python 3.8.0, 3.8.1 and 3.8.2 have been released with that. If someone wants to support "Python 3.8", using Py_BUILD_CORE_MODULE to access the internal C API is the way to go. If we wanted to add a public C API in Python 3.8, IMHO we had to do it *before Python 3.8.0 release. Now it's way too late.

    @vstinner
    Copy link
    Member

    Posted https://mail.python.org/archives/list/python-dev@python.org/thread/4UZJYAZL3NHRAGN5WAMJC4IHAHEXF3QF/ to see if anyone else wants to weigh in.

    Mark Shannon listed flaws in the PEP-532 and suggest to withdraw this PEP. Honestly, I'm open to any solution. But this issue is a concrete regression of Python 3.8 with a concrete use case, whereas Mark only lists theoretical enhancements.

    I would like to fix the regression first. There are users of the PEP-532, we cannot simply ignore them, withdraw the PEP and remove the feature immediately without warning users.

    If someone wants to withdraw the PEP, it has to go through the regular deprecation process with a slow transition. Last time we pushed too many incompatible changes (Python 2 to Python 3 transition), it was a mess. Moreover, I would prefer to have at least one Python release (if not two or more) which provides two options, deprecated way and a new better way, to have a smooth transition.

    @vstinner vstinner removed the 3.8 only security fixes label Mar 10, 2020
    @vstinner vstinner changed the title Provide a way to get/set PyInterpreterState.frame_eval without needing to access interpreter internals PEP 523: Add PyInterpreterState_SetEvalFrameFunc() to the public C API (Python 3.8 regression) Mar 10, 2020
    @brettcannon
    Copy link
    Member

    Mark Shannon listed flaws in the PEP-532 and suggest to withdraw this PEP.

    I think you mean PEP-352? And a more formal proposal to withdraw would need to be made and that has not happened, so assume the PEP is still accepted and final.

    @vstinner
    Copy link
    Member

    I think you mean PEP-352? And a more formal proposal to withdraw would need to be made and that has not happened, so assume the PEP is still accepted and final.

    I mean PEP-523 "Adding a frame evaluation API to CPython" as I wrote in the title, sorry for the typo :-)

    @markshannon
    Copy link
    Member

    I'm not suggesting immediate removal of PEP-532.

    I am requesting that you don't add a new function to the C API that will prevent implementation of many meaningful optimizations to CPython.
    PEP-532 does not add any API functions.

    I understand that you want to fix the regression.
    Why not do so by reverting the change that caused it?

    Adding a new API function is not fixing a regression.

    @vstinner
    Copy link
    Member

    Why not do so by reverting the change that caused it?

    Making PyInterpreterState structure private was motivated by the subinterpreters work but also by the work on cleaning the C API.

    Over time, PyInterpreterState became more and more complex because many other structures have been moved there. For example, the GIL, GC state, warnings state, parser state, etc.

    I would really avoid exposing the GIL state in the C API since it uses the "pycore_atomic.h" header which caused a *lot* of compiler errors in the past. Most errors were on including the header file, even if the C extension didn't use any atomic variable.

    I'm really happy that we managed to move atomic variables into the internal C API: we got less error coming from that. I'm strongly opposed to move PyInterpreterState structure back into the Include/cpython/ C API. That would be a big mistake for various reasons.

    Even in CPython, pycore_pystate.h is causing a lot of troubles since PyInterpreterState became very complex. Example in posixmodule.c:
    ---

    #include "Python.h"
    #ifdef MS_WINDOWS
       /* include <windows.h> early to avoid conflict with pycore_condvar.h:
    
            #define WIN32_LEAN_AND_MEAN
            #include <windows.h>
    
      FSCTL_GET_REPARSE_POINT is not exported with WIN32_LEAN_AND_MEAN. \*/
    

    # include <windows.h>
    #endif

    #include "pycore_ceval.h"     /* _PyEval_ReInitThreads() */
    #include "pycore_import.h"    /* _PyImport_ReInitLock() */
    #include "pycore_pystate.h"   /* _PyRuntime */

    pycore_condvar.h is used by the GIL state.

    @vstinner
    Copy link
    Member

    For me, the *short term* goal is to find a way to limit the number of broken C extension module while we modify the C API to make it more opaque.

    We cannot reach all goals at once (opaque C API, subinterpreter, more optimizations, etc). We have to move step by step.

    For me it's ok to deprecate or even remove PyInterpreterState_SetEvalFrameFunc() later, once we will have a good reason for that.

    I'm also ok with having an alternative Python implementation which doesn't support PyInterpreterState_SetEvalFrameFunc(). Users would be able to make a choice: faster Python without PyInterpreterState_SetEvalFrameFunc(), or regular "slow" Python with PyInterpreterState_SetEvalFrameFunc(). That's part of my larger https://pythoncapi.readthedocs.io/ goal: the ability to use different Python "runtimes".

    From what I understood, to be able to provide multiple Python runtimes, we have first to "fix" the C API. The HPy project is another approach to this problem. Making the C API opaque makes CPython closer to this goal.

    @vstinner
    Copy link
    Member

    Somehow related, I just created bpo-39946: "Is it time to remove _PyThreadState_GetFrame() hook?".

    @vstinner
    Copy link
    Member

    Mark Shannon: PyThreadState is an internal opaque data structure, which means we are free to change it.

    I guess that you mean PyInterpreterState which was moved to the internal C API in Python 3.8. It was part of public C API in Python 3.7. See commit be3b295 of bpo-35886.

    By the way, I just created bpo-39947: "Make the PyThreadState structure opaque (move it to the internal C API)".

    @vstinner
    Copy link
    Member

    Mark:

    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.

    I disagree, the PEP is quite explicit: "Third-party code may then set their own frame evaluation function instead to control the execution of Python code." That's the whole point of the PEP: let third-party code set eval_frame to use the feature.

    The PEP was written in 2016, when the PyInterpreterState structure was part of the public C API. But PyInterpreterState was moved to the internal C API, after the PEP was approved.

    @vstinner vstinner changed the title PEP 523: Add PyInterpreterState_SetEvalFrameFunc() to the public C API (Python 3.8 regression) PEP 523: Add private _PyInterpreterState_SetEvalFrameFunc() function to the C API (Python 3.8 regression) Mar 12, 2020
    @vstinner
    Copy link
    Member

    I took Mark's concerns in account and I updated my PR to only add *private* functions to the C API, not *public* functions.

    @vstinner
    Copy link
    Member

    Mark: Would you mind to open a separated issue for your following idea?

    I propose a new method on code objects withCallAtLine(callable: Callable[], line:int) which returns a new code object with calls to the given callable at the given line.
    A breakpoint can then be inserted at line L in function f with f.__code__ = f.__code__.withCallAtLine(sys.breakpoint, L).

    I prefer to restrict this issue to the PEP-523.

    @vstinner
    Copy link
    Member

    New changeset 0b72b23 by Victor Stinner in branch 'master':
    bpo-38500: Add _PyInterpreterState_SetEvalFrameFunc() (GH-17340)
    0b72b23

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants