coveragepy icon indicating copy to clipboard operation
coveragepy copied to clipboard

Use PyFrame_GetLasti()

Open vstinner opened this issue 3 years ago • 15 comments

Replace MyFrame_lasti() with PyFrame_GetLasti(): Python 3.11.0b1 adds PyFrame_GetLasti().

Add pythoncapi_compat.h header file to get PyFrame_GetLasti() on Python 3.10 and older. File copied from: https://github.com/python/pythoncapi_compat

Keep compatibility code for alpha versions of Python 3.11 (alpha 1 to alpha 7). Remove PyFrame_GetLocals(), PyFrame_GetGlobals() and PyFrame_GetBuiltins() in pythoncapi_compat.h which are not compatible with alpha versions of Python 3.11. Hack pythoncapi_compat.h to not define PyFrame_GetLasti() on Python 3.11a1 and newer.

vstinner avatar Apr 08 '22 15:04 vstinner

Add pythoncapi_compat.h header file to get PyFrame_GetLasti()

With this header file, it also becomes possible to drop MyFrame_GetCode() from coverage/ctracer/util.h and use PyFrame_GetCode() on all Python versions. It would allow to fix a reference leak in coverage affecting Python 3.9 and newer: PyFrame_GetCode() returns a strong reference, whereas MyFrame_GetCode() returns a borrow reference on Python 3.8 and older. Currently, coverage always make the assumption that MyFrame_GetCode() returns a borrow reference which is wrong on Python 3.9 and newer.

I chose to restrict this PR to PyFrame_GetLasti() to keep it short.

vstinner avatar Apr 08 '22 15:04 vstinner

Current implementation of PyFrame_GetLasti() in upstream pythoncapi_compat.h:

// bpo-40421 added PyFrame_GetLasti() to Python 3.11.0b1
#if PY_VERSION_HEX < 0x030B00B1 && !defined(PYPY_VERSION)
PYCAPI_COMPAT_STATIC_INLINE(int)
PyFrame_GetLasti(PyFrameObject *frame)
{
#if PY_VERSION_HEX >= 0x030A00A7
    // bpo-27129: Since Python 3.10.0a7, f_lasti is an instruction offset,
    // not a bytes offset anymore. Python uses 16-bit "wordcode" (2 bytes)
    // instructions.
    if (frame->f_lasti < 0) {
        return -1;
    }
    return frame->f_lasti * 2;
#else
    return frame->f_lasti;
#endif
}
#endif

In this PR, I replaced PY_VERSION_HEX < 0x030B00B1 (beta1) with PY_VERSION_HEX < 0x030B00A1 (alpha1) to prevent a compiler error on Python 3.11 alpha versions. On Python 3.11.0 final will be released, coverage code can be simplified to remove support for Python 3.11 alpha versions.

IMO it's better to rely on pythoncapi_compat.h since this code is non trivial.

vstinner avatar Apr 08 '22 16:04 vstinner

@nedbat: Are you ok to use pythoncapi_compat.h? As I wrote, it would allow to fix the coverage MyFrame_GetCode() refleak (I can work on a fix, once this PR is merged).

vstinner avatar Apr 08 '22 16:04 vstinner

@vstinner Yes, I am fine with using pythoncapi_compat.h. But when I try this pull request on main, I still have compile errors:

coverage/ctracer/tracer.c:533:48: error: no member named 'co_code' in 'struct PyCodeObject'
    PyObject * pCode = MyFrame_GetCode(frame)->co_code;
                       ~~~~~~~~~~~~~~~~~~~~~~  ^
coverage/ctracer/tracer.c:534:43: error: no member named 'f_lasti' in 'struct _PyInterpreterFrame'
    real_call = (PyBytes_AS_STRING(pCode)[PyFrame_GetLasti(frame) + 1] == 0);
                                          ^~~~~~~~~~~~~~~~~~~~~~~
coverage/ctracer/util.h:20:47: note: expanded from macro 'PyFrame_GetLasti'
#define PyFrame_GetLasti(f)    ((f)->f_frame->f_lasti * 2)
                                ~~~~~~~~~~~~  ^
coverage/ctracer/tracer.c:701:56: error: no member named 'co_code' in 'struct PyCodeObject'
            PyObject * pCode = MyFrame_GetCode(frame)->co_code;
                               ~~~~~~~~~~~~~~~~~~~~~~  ^
coverage/ctracer/tracer.c:702:25: error: no member named 'f_lasti' in 'struct _PyInterpreterFrame'
            int lasti = PyFrame_GetLasti(frame);
                        ^~~~~~~~~~~~~~~~~~~~~~~
coverage/ctracer/util.h:20:47: note: expanded from macro 'PyFrame_GetLasti'
#define PyFrame_GetLasti(f)    ((f)->f_frame->f_lasti * 2)
                                ~~~~~~~~~~~~  ^

I'm going to work on this, but if you have suggestions, I would welcome them!

nedbat avatar Apr 09 '22 18:04 nedbat

I deleted my comment. I went 1 week in holiday and it is way enough for me to forget the context :-) This PR is supposed to support Python 3.11 alpha versions. I'm not sure what's going on.

vstinner avatar Apr 19 '22 15:04 vstinner

Also, co_code has been gone since https://github.com/python/cpython/commit/2bde6827ea4f136297b2d882480b981ff26262b6. Which is 3.11a7 and up only. For that you'd need the internal _PyCode_GetCode (Huh, I just realised we should probably expose that in CPython's API).

You could access co_code_adaptive directly, but that would include the quickened bytecodes, which don't allow tracing.

Fidget-Spinner avatar May 02 '22 14:05 Fidget-Spinner

Also, co_code has been gone since https://github.com/python/cpython/commit/2bde6827ea4f136297b2d882480b981ff26262b6. Which is 3.11a7 and up only. For that you'd need the internal _PyCode_GetCode (Huh, I just realised we should probably expose that in CPython's API).

No. Please don't use an internal C API to read co_code. This attribute is accessible in Python, so we should provide a public function for that.

vstinner avatar May 02 '22 15:05 vstinner

Or you can already use PyObject_GetAttrString(code, "co_code").

vstinner avatar May 02 '22 15:05 vstinner

See https://github.com/python/cpython/issues/91397 "Move the PyCodeObject structure to the internal C API (make the structure opaque)".

vstinner avatar May 02 '22 15:05 vstinner

No. Please don't use an internal C API to read co_code. This attribute is accessible in Python, so we should provide a public function for that.

Sorry I wasn't trying to suggest people use it. Anyways, its in the internal API and completely private/hidden so its impossible to use :). Also I highly do not recommend accessing the struct member co_code_adaptive directly.

Fidget-Spinner avatar May 02 '22 15:05 Fidget-Spinner

I've solved the co_code problem using PyObject_GetAttrString(code, "co_code") in #1368. Can you take a look to see if there's something I've done wrong?

nedbat avatar May 02 '22 15:05 nedbat

@vstinner I've gotten things to work in 956f0fde, but I would still rather use this approach when you have time to help me with it.

nedbat avatar May 12 '22 19:05 nedbat

@nedbat I have a WIP branch ^1 which uses pythoncapi_compat but it does not support alpha versions but as the beta is already out so supporting alpha versions is less important. It will make coveragepy a little slower because pythoncapi_compat uses strong references instead of current borrowed references though.

kumaraditya303 avatar May 25 '22 07:05 kumaraditya303

@nedbat I have a WIP branch 1 which uses pythoncapi_compat

Can you create a PR for that?

vstinner avatar May 27 '22 11:05 vstinner

I rebased my PR on the master branch and I updated pythoncapi_compat.h. My PR drops support for Python 3.11 alpha versions. IMO it's too complicated to support 3.11 alpha versions and it's not worth it.

I prefer to write a separated PR for PyCode_GetCode(), since it requires to add Py_DECREF(). I prefer to keep this PR easy to review.

vstinner avatar May 27 '22 12:05 vstinner