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

frame->f_lasti will be removed in Python 3.11 #1241

Closed
encukou opened this issue Oct 13, 2021 · 13 comments
Closed

frame->f_lasti will be removed in Python 3.11 #1241

encukou opened this issue Oct 13, 2021 · 13 comments
Labels
bug Something isn't working fixed

Comments

@encukou
Copy link

encukou commented Oct 13, 2021

Describe the bug
In Python 3.11, the f_lasti member of PyFrameObject will be removed.
AFAIK, the quickened interpreter uses a different strategy to track what it's executing; a byte offset into original bytecode would need to be computed from other data (and probably wouldn't make sense in some cases).

This is an early heads-up; you might want to be part of the conversation in CPython so a better API can be designed for coverage's use case.
AFAICS, coverage uses f_lasti for generators, but I'm not really sure what the code is actually doing. Should CPython add API for telling just-started generator apart from a resuming generator, for example?

To Reproduce

  1. What version of Python are you using? CPython 3.11.0a1+, main branch (380c440875)
  2. What version of coverage.py are you using? master branch (0eaeb99f2d)
  3. What versions of what packages do you have installed? Nothing extra; pip freeze output is empty
  4. What commands did you run? python3 setup.py build
...
coverage/ctracer/tracer.c: In function ‘CTracer_handle_call’:
coverage/ctracer/tracer.c:552:14: error: ‘PyFrameObject’ {aka ‘struct _frame’} has no member named ‘f_lasti’
  552 |     if (frame->f_lasti < 0) {
      |              ^~
In file included from coverage/ctracer/tracer.c:6:
coverage/ctracer/tracer.c: In function ‘CTracer_handle_return’:
coverage/ctracer/util.h:18:31: error: ‘PyFrameObject’ {aka ‘struct _frame’} has no member named ‘f_lasti’
   18 | #define MyFrame_lasti(f)    (f->f_lasti * 2)
      |                               ^~
coverage/ctracer/tracer.c:719:25: note: in expansion of macro ‘MyFrame_lasti’
  719 |             int lasti = MyFrame_lasti(frame);
      |                         ^~~~~~~~~~~~~
**
** Couldn't install with extension module, trying without it...
** BuildFailed: command '/usr/lib64/ccache/gcc' failed with exit code 1
**

Expected behavior
The extension compiles :)

@encukou encukou added the bug Something isn't working label Oct 13, 2021
@vstinner
Copy link

Discussion on Python upstream about adding abstractions to PyFrameObject for coverage use cases: https://bugs.python.org/issue40421#msg403814

@vstinner
Copy link

Adapting MyFrame_lasti() to Python 3.11 requires to dig into deep CPython internals, use the internal C API. Something like:

#if PY_VERSION_HEX >= 0x030B0000
#  include "internal/pycore_frame.h" /* Python 3.11 InterpreterFrame */
#  define MyFrame_lasti(f)    (f->f_frame->f_lasti * 2)

I would prefer to write an abstraction in the public C API for that, or maybe add a private function just for coverage.

If possible, I would prefer to avoid accessing CPython internals in coverage.

@vstinner
Copy link

For the PyFrame_GetLineNumber() code path, maybe there is a simple fix. PyFrame_GetLineNumber() now handles negative f_lasti, and so ctracer doesn't have to handle it manually. I propose a fix like that:

diff --git a/coverage/ctracer/tracer.c b/coverage/ctracer/tracer.c
index 00d9f106..9738e383 100644
--- a/coverage/ctracer/tracer.c
+++ b/coverage/ctracer/tracer.c
@@ -545,14 +545,19 @@ CTracer_handle_call(CTracer *self, PyFrameObject *frame)
     Py_INCREF(self);
     Py_XSETREF(frame->f_trace, (PyObject*)self);
 
+    // Before Python 3.10.0a3, PyFrame_GetLineNumber() didn't support
+    // negative f_lasti (generator which didn't start yet)
+#if PY_VERSION_HEX < 0x030A00A3
     /* A call event is really a "start frame" event, and can happen for
      * re-entering a generator also.  f_lasti is -1 for a true call, and a
      * real byte offset for a generator re-entry.
      */
-    if (frame->f_lasti < 0) {
+    if (MyFrame_lasti(frame) < 0) {
         self->pcur_entry->last_line = -MyFrame_GetCode(frame)->co_firstlineno;
     }
-    else {
+    else
+#endif
+    {
         self->pcur_entry->last_line = PyFrame_GetLineNumber(frame);
     }
 

@nedbat
Copy link
Owner

nedbat commented Oct 13, 2021

Thanks for starting this: it's been on my list to ask about what to do about f_lasti disappearing.

@nedbat
Copy link
Owner

nedbat commented Oct 13, 2021

@markshannon Can you help here?

@encukou
Copy link
Author

encukou commented Oct 14, 2021

@nedbat, that depends on what you use f_lasti for.

I can see it in tracer.c:

/* A call event is really a "start frame" event, and can happen for
* re-entering a generator also. f_lasti is -1 for a true call, and a
* real byte offset for a generator re-entry.
*/
if (frame->f_lasti < 0) {
self->pcur_entry->last_line = -MyFrame_GetCode(frame)->co_firstlineno;
}
else {
self->pcur_entry->last_line = PyFrame_GetLineNumber(frame);
}
which is working around a PyFrame_GetLineNumber deficiency; as Victor said this shouldn't be necessary in 3.10+.

And the other use is MyFrame_lasti, which is then used in one place in tracer.c:

/* Need to distinguish between RETURN_VALUE and YIELD_VALUE. Read
* the current bytecode to see what it is. In unusual circumstances
* (Cython code), co_code can be the empty string, so range-check
* f_lasti before reading the byte.
*/
int bytecode = RETURN_VALUE;
PyObject * pCode = MyFrame_GetCode(frame)->co_code;
int lasti = MyFrame_lasti(frame);
if (lasti < PyBytes_GET_SIZE(pCode)) {
bytecode = PyBytes_AS_STRING(pCode)[lasti];
}
if (bytecode != YIELD_VALUE) {
int first = MyFrame_GetCode(frame)->co_firstlineno;
if (CTracer_record_pair(self, self->pcur_entry->last_line, -first) < 0) {
goto error;
}
}
– to get the current bytecode instruction.
This assumes that the "current instruction" is part of the original bytecode, which is, AFAIK, not always the case any more.

I'm not exactly sure what this code actually needs to do, but it doesn't look like you need f_lasti. What would be a good API that CPython could give you for this use case? For example, opting in for the sys.settrace function to get "yield"/"resume" rather than "return"/"call"?

@nedbat
Copy link
Owner

nedbat commented Oct 14, 2021

I'm not exactly sure what this code actually needs to do, but it doesn't look like you need f_lasti. What would be a good API that CPython could give you for this use case? For example, opting in for the sys.settrace function to get "yield"/"resume" rather than "return"/"call"?

Yes, I'm using lasti to distinguish between entering a frame for the first time, and re-entering the frame. Typing that out makes me wonder if I can do my own bookkeeping to distinguish between them... (never mind, I don't think there's enough information for me to track it on my own.)

As an API, the unused third arg could provide the information that distinguishes call from resume and return from yield.

nedbat added a commit that referenced this issue Nov 10, 2021
The fix for CTracer is egregious and will need to be updated when there's a
supported way to do it.

The fullcoverage skip is noted in
#1278

The raise_through_with skip is noted in
#1270
nedbat added a commit that referenced this issue Nov 10, 2021
The fix for CTracer is egregious and will need to be updated when there's a
supported way to do it.

The fullcoverage skip is noted in
#1278

The raise_through_with skip is noted in
#1270
nedbat added a commit that referenced this issue Nov 10, 2021
The fix for CTracer is egregious and will need to be updated when there's a
supported way to do it.

The fullcoverage skip is noted in
#1278

The raise_through_with skip is noted in
#1270
nedbat added a commit that referenced this issue Nov 10, 2021
The fix for CTracer is egregious and will need to be updated when there's a
supported way to do it.

The fullcoverage skip is noted in
#1278

The raise_through_with skip is noted in
#1270
nedbat added a commit that referenced this issue Nov 10, 2021
The fix for CTracer is egregious and will need to be updated when there's a
supported way to do it.

The fullcoverage skip is noted in
#1278

The raise_through_with skip is noted in
#1270
nedbat added a commit that referenced this issue Nov 10, 2021
The fix for CTracer is egregious and will need to be updated when there's a
supported way to do it.

The fullcoverage skip is noted in
#1278

The raise_through_with skip is noted in
#1270
nedbat added a commit that referenced this issue Nov 10, 2021
The fix for CTracer is egregious and will need to be updated when there's a
supported way to do it.

The fullcoverage skip is noted in
#1278

The raise_through_with skip is noted in
#1270
@nedbat
Copy link
Owner

nedbat commented Nov 11, 2021

I've worked around the problem in 9765493, but it's not pretty.

@nedbat
Copy link
Owner

nedbat commented Nov 11, 2021

This is now released as part of coverage 6.1.2.

@nedbat nedbat added the fixed label Nov 11, 2021
@nedbat nedbat closed this as completed Nov 11, 2021
@vstinner
Copy link

#if PY_VERSION_HEX >= 0x030B00A0
// 3.11 moved f_lasti into an internal structure. This is totally the wrong way
// to make this work, but it's all I've got until https://bugs.python.org/issue40421
// is resolved.
#include <internal/pycore_frame.h>
#define MyFrame_lasti(f)    ((f)->f_frame->f_lasti * 2)

FYI it is intentional that the internal C API is accessible so debuggers and profilers can inspect structures without having to execute code.

But I agree that adding a public C API would be a better solution for the long term :-)

@nedbat
Copy link
Owner

nedbat commented Nov 11, 2021

@vstinner I don't understand: is internal/pycore_frame.h something I should use, or not? It seems to be named "keep out."

@vstinner
Copy link

In general, you should not use the internal C API, unless you are writing a debugger or a profiler, which is your case :-)

@vstinner
Copy link

If you opt-in for the internal C API, be prepared for incompatible changes at any Python release, major (3.x) or minor (3.x.y).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed
Projects
None yet
Development

No branches or pull requests

3 participants