Skip to content
This repository was archived by the owner on Feb 13, 2025. It is now read-only.

The the C-API function PyEval_EvalFrameEx() is broken #166

Closed
akruis opened this issue Jul 31, 2018 · 7 comments
Closed

The the C-API function PyEval_EvalFrameEx() is broken #166

akruis opened this issue Jul 31, 2018 · 7 comments

Comments

@akruis
Copy link

akruis commented Jul 31, 2018

The C-API function PyEval_EvalFrameEx() is completely broken. Thanks to Kevin, who opened #165, I became aware of this problem.

Stackless does not use PyEval_EvalFrameEx(PyFrameObject *f, int throwflag). In theory an extension module can't use this function either, because the documented API provides no way to construct a PyFrameObject. Therefore Stackless did not test PyEval_EvalFrameEx() and untested code becomes buggy.

Nowadays Cython uses PyEval_EvalFrameEx to speed up calling Python functions. Cython uses the undocumented function PyFrame_New() to create a PyFrameObject (a struct) and then Cython fills the struct. Unfortunately the definitions of PyFrameObject differ between Stackless-Python and C-Python. Therefore you need to compile Cython code using Stackless-Python, if you want to run it with Stackless-Python.

The main problem of the current implementation of PyEval_EvalFrameEx is that it exposes Stackless features (i.e. stack unwinding) to an extension module, that is not prepared to handle this situation. Therefore I had to rewrite PyEval_EvalFrameEx.

@kristjanvalur
Copy link
Collaborator

Interesting, this must have been the case for years and years.

@kristjanvalur
Copy link
Collaborator

Is it possible to have PyFrameObject compatible between stackless and non-stackless?

akruis pushed a commit that referenced this issue Aug 1, 2018
The C-API functions PyEval_EvalFrameEx() and PyEval_EvalFrame() were
broken, because Stackless does not use them and didn't test them. This
commit adds test code and makes the functions compatible with C-Python.
It is now save to call them from extension modules, i.e. modules created
by Cython.
@akruis akruis modified the milestones: v2.7.16-slp, v3.6.5-slp Aug 1, 2018
@scoder
Copy link

scoder commented Aug 1, 2018

Nice! Haven't tested it, but thanks for doing this!

It's certainly good for Stackless to have this fixed, but note that we can also disable the usage in Cython. We use several compatibility switches that enable or disable certain alternative implementations in other runtimes, which usually means PyPy. In this case, we could automatically disable the CYTHON_FAST_PYCALL feature flag when we detect a Stackless version where this function is broken. Is there a way to do that?

@akruis
Copy link
Author

akruis commented Aug 1, 2018

For sure it is possible to detect Stackless at compile time and at run time. The compile time check is simple: #ifdef STACKLESS.

Traditionally Stackless tries to be strictly ABI compatible with C-Python. That is, a extension module compiled on C-Python runs on Stackless Python. If we want to preserve this level of compatibility, Cython must detect Stackless at runtime. Then Cython could either disable CYTHON_FAST_PYCALL or use the Stackless layout of PyFrameObject. If Cython uses PyFrameNew() and then only fills the f_localsplus array, a runtime switch is cheap. Pseudo code:

size_t localsplus_offset = IS_STACKLESS ? OFFSET_FOR_STACKLESS : OFFSET_FOR_CPYTHON;
...
fastlocals = f + localsplus_offset;
for (i = 0; i < na; i++) {
        Py_INCREF(*args);
        fastlocals[i] = *args++;
}

Without such a runtime switch, Cython code would crash in interesting ways. In the probably most common case (compiled with C-Python, running on Stackless, i.e. installed from a binary wheel), C-Python f_localsplus has the same offset as Stackless f_code. Filling the f_localsplus array overwrites f_code. I could add a test to PyEval_EvalFrameEx if f_code is really a code object and raise SystemError in case.

Probably, we should discuss details in the Cython issue tracker.

akruis pushed a commit that referenced this issue Aug 5, 2018
Support using PyEval_EvalFrameEx() without a main tasklet. And fix a
potential reference counting problem.
@akruis
Copy link
Author

akruis commented Aug 5, 2018

The recent discussion in #165 showed, that commit d377c06 is not sufficient. PyEval_EvalFrameEx() may be used without entering the Stackless subsystem first. Possible use-cases: using the low level threading API (like the test case in #165 ) or compiling Lib/threading.py with Cython.

Therefore I extended PyEval_EvalFrameEx() to initialize the Stackless subsystem on the fly, analogous to PyStackless_Call_Main(). Additionally I fixed a ref-counting problem (incorrect usage of a borrowed ref).

@akruis
Copy link
Author

akruis commented Aug 16, 2018

@kristjanvalur Your question "Is it possible to have PyFrameObject compatible between stackless and non-stackless?" still needs an answer.

Stackless needs an additional member (f_execute) in PyFrameObject. Therefore it is not possible to use C-Python Frames unmodified. But for PyEval_EvalFrameEx() we have different situation, because this function is not used by Stackless python at all. In issue #168 / pull request #169, I detect and repair broken frames.

akruis pushed a commit that referenced this issue Aug 16, 2018
The C-API functions PyEval_EvalFrameEx() and PyEval_EvalFrame() were
broken, because Stackless does not use them and didn't test them. This
commit adds test code and makes the functions compatible with C-Python.
It is now save to call them from extension modules, i.e. modules created
by Cython.
(cherry picked from commit d377c06)
akruis pushed a commit that referenced this issue Aug 16, 2018
Support using PyEval_EvalFrameEx() without a main tasklet. And fix a
potential reference counting problem.

(cherry picked from commit 187184b)
akruis pushed a commit that referenced this issue Aug 16, 2018
The C-API functions PyEval_EvalFrameEx() and PyEval_EvalFrame() were
broken, because Stackless does not use them and didn't test them. This
commit adds test code and makes the functions compatible with C-Python.
It is now save to call them from extension modules, i.e. modules created
by Cython.
(cherry picked from commit d377c06)
akruis pushed a commit that referenced this issue Aug 16, 2018
Support using PyEval_EvalFrameEx() without a main tasklet. And fix a
potential reference counting problem.

(cherry picked from commit 187184b)
akruis pushed a commit that referenced this issue Aug 16, 2018
The C-API functions PyEval_EvalFrameEx() and PyEval_EvalFrame() were
broken, because Stackless does not use them and didn't test them. This
commit adds test code and makes the functions compatible with C-Python.
It is now save to call them from extension modules, i.e. modules created
by Cython.
(cherry picked from commit d377c06)
akruis pushed a commit that referenced this issue Aug 16, 2018
Support using PyEval_EvalFrameEx() without a main tasklet. And fix a
potential reference counting problem.

(cherry picked from commit 187184b)
@akruis
Copy link
Author

akruis commented Aug 16, 2018

Fix ported to master-slp (4aefdd0), 3.6-slp (070e7f0) and 3.5-slp (7baae97)

@akruis akruis closed this as completed Aug 16, 2018
@akruis akruis mentioned this issue Sep 18, 2018
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants