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

Expose _PyInterpreterFrame_GetLine in the private API #96803

Open
pablogsal opened this issue Sep 13, 2022 · 13 comments
Open

Expose _PyInterpreterFrame_GetLine in the private API #96803

pablogsal opened this issue Sep 13, 2022 · 13 comments
Labels
3.11 only security fixes 3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@pablogsal
Copy link
Member

pablogsal commented Sep 13, 2022

In 3.11 we are using now an opaque pointer to _PyInterpreterFrame in the eval function. The problem is that for PEP 523's APIs, we changed the type of _PyFrameEvalFunction to be PyObject *(*_PyFrameEvalFunction)(PyThreadState *tstate, _PyInterpreterFrame *frame, int throwflag) but there is no way to do anything with _PyInterpreterFrame because is opaque. Even if the extension defines PY_BUILD_CORE the symbols are not exposed in the executable/libpython, which means that including the headers is insufficient.

This is a problem because it really limits what users of PEP 523 can do with the frame object that they receive, even if is opaque. Most profilers using PEP 523 API do it for speed reasons and the only thing they need is to get the line number, but that is now impossible because the pointer is opaque and we don't offer any exposed APIs.

Linked PRs

@pablogsal pablogsal added type-bug An unexpected behavior, bug, or error 3.11 only security fixes 3.12 bugs and security fixes labels Sep 13, 2022
@pablogsal
Copy link
Member Author

@ericsnowcurrently
Copy link
Member

CC @mdboom

@markshannon
Copy link
Member

markshannon commented Sep 14, 2022

I see your point, but I don't think that _PyInterpreterFrame_GetLine() is the right API.
I think the pair of _PyInterpreterFrame_GetCode() and _PyInterpreterFrame_GetLasti() would be better.
Getting the offset is much cheaper than computing the line number.
The line number can be computed, if needed.
Other information to be extracted from the code object, such as the filename and function name.

@pablogsal
Copy link
Member Author

I started exposing _PyInterpreterFrame_GetCode and _PyInterpreterFrame_GetLasti but then I realized that neither _PyInterpreterFrame_GetCode and _PyInterpreterFrame_GetLasti are in 3.12 so exposing those is going to be more chaotic than just exposing _PyInterpreterFrame_GetLine. Notice that if the application defines PY_BUILD_CORE it can already access f_code and f_lasti manually from the struct.

As there can be arbitrary logic in _PyInterpreterFrame_GetLine I still think is a better idea to export _PyInterpreterFrame_GetLine.

@markshannon
Copy link
Member

It still think that the pair of _PyInterpreterFrame_GetCode and _PyInterpreterFrame_GetLasti is a better interface.

If you think that doing that would be too disruptive for 3.11, we can add them for 3.12, and just expose _PyInterpreterFrame_GetLine for 3.11.

@pablogsal
Copy link
Member Author

pablogsal commented Sep 15, 2022

If you think that doing that would be too disruptive for 3.11, we can add them for 3.12, and just expose

In 3.12 they are already exposed if the user calls PY_BUILD_CORE because one is a macro and the other can be basically substituted by frame->f_code because the struct definition is available if PY_BUILD_CORE is defined. The problem here is having to re-implement _PyInterpreterFrame_GetLine.

Exposing _PyInterpreterFrame_GetCode and _PyInterpreterFrame_GetLasti doesn't give the user anything that cannot be achieved by just calling frame->f_code and frame->f_lasti after defining PY_BUILD_CORE

@markshannon
Copy link
Member

_PyInterpreterFrame_GetLine is quite simple once we have the other two functions. There is no harm in exposing it as well.

the struct definition is available if PY_BUILD_CORE is defined.

All that does is expose the existence of a field call f_code. It doesn't say anything about its meaning or when it is valid.

In general, frame->f_code cannot be substituted for _PyInterpreterFrame_GetCode() as the meaning of frame->f_code is not defined. E.g. we might make it such that f_code is only valid for some frames faster-cpython/ideas#111

@pablogsal
Copy link
Member Author

pablogsal commented Sep 15, 2022

Ok, I am convinced. I will expose _PyInterpreterFrame_GetCode and _PyInterpreterFrame_GetLasti

pablogsal added a commit to pablogsal/cpython that referenced this issue Sep 15, 2022
@pablogsal
Copy link
Member Author

Ah, turns out this has changed in ef6a482 and now is a macro on 3.11 as well. This means we can backport the 3.12 change

pablogsal added a commit to pablogsal/cpython that referenced this issue Sep 15, 2022
@pablogsal
Copy link
Member Author

@markshannon I had another idea. Maybe we should expose the API to transform the interpreter frame into a python object. At that point you can do whatever you want with it. What do you think?

@markshannon
Copy link
Member

Doesn't that undermine the whole point as using the _PyInterpreterFrame in the first place, i.e. efficiency?
Also, incomplete _PyInterpreterFrames have a code object and lasti, but we cannot safely produce a frame object for them

@erlend-aasland
Copy link
Contributor

gh-96849 added new APIs, but no documentation was added. Mentioning it here, so we don't forget before we close this issue.

carljm added a commit to carljm/cpython that referenced this issue May 5, 2023
* main:
  pythongh-99113: Add PyInterpreterConfig.own_gil (pythongh-104204)
  pythongh-104146: Remove unused var 'parser_body_declarations' from clinic.py (python#104214)
  pythongh-99113: Add Py_MOD_PER_INTERPRETER_GIL_SUPPORTED (pythongh-104205)
  pythongh-104108: Add the Py_mod_multiple_interpreters Module Def Slot (pythongh-104148)
  pythongh-99113: Share the GIL via PyInterpreterState.ceval.gil (pythongh-104203)
  pythonGH-100479: Add `pathlib.PurePath.with_segments()` (pythonGH-103975)
  pythongh-69152: Add _proxy_response_headers attribute to HTTPConnection (python#26152)
  pythongh-103533: Use PEP 669 APIs for cprofile (pythonGH-103534)
  pythonGH-96803: Add three C-API functions to make _PyInterpreterFrame less opaque for users of PEP 523. (pythonGH-96849)
jbower-fb pushed a commit to jbower-fb/cpython-jbowerfb that referenced this issue May 8, 2023
@encukou
Copy link
Member

encukou commented May 9, 2023

Note that even unstable API needs tests as well as documentation.

markshannon added a commit that referenced this issue May 18, 2023
…GH-104211)

Weaken contract of PyUnstable_InterpreterFrame_GetCode to return PyObject*.
carljm added a commit to carljm/cpython that referenced this issue May 18, 2023
* main:
  pythongh-74690: Don't set special protocol attributes on non-protocol subclasses of protocols (python#104622)
  pythongh-104623: Update Windows installer to use SQLite 3.42.0 (python#104625)
  pythongh-104050: Add more type annotations to Argument Clinic (python#104628)
  pythongh-104629: Don't skip test_clinic if _testclinic is missing (python#104630)
  pythongh-104549: Set __module__ on TypeAliasType (python#104550)
  pythongh-104050: Improve some typing around `default`s and sentinel values (python#104626)
  pythongh-104146: Remove unused vars from Argument Clinic (python#104627)
  pythongh-104615: don't make unsafe swaps in apply_static_swaps (python#104620)
  pythonGH-104484: Add case_sensitive argument to `pathlib.PurePath.match()` (pythonGH-104565)
  pythonGH-96803: Document and test new unstable internal frame API functions (pythonGH-104211)
  pythonGH-104580: Don't cache eval breaker in interpreter (pythonGH-104581)
vstinner added a commit to vstinner/cpython that referenced this issue Jul 24, 2023
Declare the following 3 PyUnstable functions in
Include/cpython/pyframe.h rather than Include/cpython/frameobject.h,
so they are now provided by the standard "#include <Python.h>".
vstinner added a commit to vstinner/cpython that referenced this issue Jul 24, 2023
Declare the following 3 PyUnstable functions in
Include/cpython/pyframe.h rather than Include/cpython/frameobject.h,
so they are now provided by the standard "#include <Python.h>".
vstinner added a commit that referenced this issue Jul 24, 2023
…7188)

Declare the following 3 PyUnstable functions in
Include/cpython/pyframe.h rather than Include/cpython/frameobject.h,
so they are now provided by the standard "#include <Python.h>".
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 24, 2023
pythonGH-107188)

Declare the following 3 PyUnstable functions in
Include/cpython/pyframe.h rather than Include/cpython/frameobject.h,
so they are now provided by the standard "GH-include <Python.h>".
(cherry picked from commit 837fa5c)

Co-authored-by: Victor Stinner <[email protected]>
vstinner added a commit that referenced this issue Jul 24, 2023
….h (GH-107188) (#107195)

GH-96803: Move PyUnstable_InterpreterFrame_GetCode() to Python.h (GH-107188)

Declare the following 3 PyUnstable functions in
Include/cpython/pyframe.h rather than Include/cpython/frameobject.h,
so they are now provided by the standard "GH-include <Python.h>".
(cherry picked from commit 837fa5c)

Co-authored-by: Victor Stinner <[email protected]>
jtcave pushed a commit to jtcave/cpython that referenced this issue Jul 27, 2023
python#107188)

Declare the following 3 PyUnstable functions in
Include/cpython/pyframe.h rather than Include/cpython/frameobject.h,
so they are now provided by the standard "#include <Python.h>".
@iritkatriel iritkatriel added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Nov 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes 3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

6 participants