Skip to content

Comments

Pull request for the Cython Debugger addition #1

Merged
4 commits merged intocython:masterfrom
markflorisson:master
Dec 9, 2010
Merged

Pull request for the Cython Debugger addition #1
4 commits merged intocython:masterfrom
markflorisson:master

Conversation

@markflorisson
Copy link
Contributor

A debugging section has already been added to the user guide, see https://sage.math.washington.edu:8091/hudson/job/cython-docs/doclinks/1/src/userguide/debugging.html . Required for the debugger is gdb 7.2 or newer, and python 2.5 or better. It has not yet been tested with Python 3.

@robertwb
Copy link
Contributor

I've started looking at this, and it all looks very good so far. One comment I have is that "debug" is a rather generic name--there could be multiple debuggers (e.g. pdb) and some stuff named debug is about debugging Cython itself, and other stuff about debugging the resulting module. Perhaps gdb-specific references could be named "gdb" rather than "debug?"

@markflorisson
Copy link
Contributor Author

Thanks for the feedback Robert.

We need to ship libpython.py with Cython because libpython.py is not installed as a python module (it's copied as a gdb extension named python-gdb.py into a gdb-specific directory so it can get autoloaded by gdb when doing 'file python') and also because libpython.py is fairly recent and I have also done a lot of additions that may be merged into new python 3 releases (see http://bugs.python.org/issue10566 ) .

Disabled optimization is not strictly needed, but for instance when Cython generates code such as:

some_c_variable = 1;

that corresponds to a line in the user's source code, cygdb won't be able to step to that line because gcc optimizes this out. This may also be a problem when trying to print locals (the variable will not be listed).

In response to the python version support, I think gdb can also link to python2.4, but I'm not sure (and I can't seem to find any docs on this). In any case, the code needs at least python2.5 (and so did libpython.py before I incorporated or started modifying it). So indeed, I'll try to make the inclusion conditionally in setup.py.

One other issue that I've had myself it that users often install only with their normal python version, but not with their debug build. This means they won't be able to use Cython with a python debug build unless they install it for the debug build also. For instance, if you talk about ubuntu, a debug build shares the sys.path with the normal python interpreter, but compiles extension modules to module_d.so instead of module.so. However, the debug version of the interpreter will fall back to module.so when module_d.so is not present, which will result in the error:

$ python-dbg setup.py build_ext --inplace --pyrex-debug
running build_ext
failed to import Cython: /usr/local/lib/python2.6/dist-packages/Cython/Compiler/Scanning.so: undefined symbol: Py_InitModule4_64

I'm afraid many users will not understand this error message and move back to regular python, but Cygdb really needs a debug build to work properly (especially when it comes to python breakpoints, python stepping, python frames in backtraces, etc). Do you think I should document this behaviour in the debugging section of the user guide?

I'll have a look at all these issues and their implementation later today or tomorrow.

@markflorisson
Copy link
Contributor Author

Basically, I think we should consider whether it's a good idea to compile some Cython compiler modules with Cython for speedups, as I'm not sure how great exactly the speed gain is and how important this speed gain is. I think if the Cython compiler could be pure python users will never have to worry about installing Cython (manually) for debug builds (unless the debug builds has a separate sys.path, in which case they have to do this anyway).

In any case, I think the --pyrex-debug (or --pyrex-gdb after renaming the flag?) should never become the default, as it disables optimization for the C/C++ compiler and generates a bunch of xml files. Users that are aware of cygdb can still pass the command line flag or enable debugging for specific extension modules, and they would also be aware of the fact that debug builds work a lot better for debugging.

@robertwb
Copy link
Contributor

OK, sounds like we're justified shipping libpython.py, lets just add a note at the top about that.

We should definitely add a note to the manual about the kinds of errors one might get if one isn't using the debug version of Python. (Is there a way to detect this early and give a nicer message?)

The speed gains of compiling Cython with itself can be non-trivial, so I think are worth keeping for most users. Given that the xml files are external, I could see that eventually it would make sense to generate them by default (not disabling optimization), with the note that disabling optimization is helpful for debugging. (Often one doesn't know ahead of time that one is going to have to debug a program, and having to re-compile is an extra, unwanted step, especially for an end-user.) For now, however, lets keep it like it is.

@markflorisson
Copy link
Contributor Author

If the user isn't using a debug build cygdb should handle errors gracefully, i.e. print things such as '#7 Unknown Frame (compile with -g)' in backtraces for frames it can't obtain information about or "Unable to lookup type PyModuleObject, did you compile python with debugging support (-g)?" when listing globals in a Cython module.

A bigger problem (one that cygdb doesn't handle) are python breakpoints for non-debug builds. Whenever cygdb isn't able to read Python frame information in PyEval_EvalFrameEx during the evaluation of the breakpoint condition it "skips it silently". This means python breakpoints simply don't seem to work. I'll add a note about that in the docs. If you think there should be more notes about particular issues I'll add them.

@markflorisson
Copy link
Contributor Author

By the way, the new commit (c12526e) should (hopefully :) have fixed all the issues you mentioned.

@robertwb
Copy link
Contributor

robertwb commented Dec 3, 2010

I agree that using a non-debug Python build is not ideal, but it could easily be better than nothing (or raw gdb). Perhaps the cygdb script could also print out a warning in this case (with a pointer to the documentation).

I'll hopefully have time to look at this more over the weekend, this is going to be a really great feature!

@robertwb
Copy link
Contributor

robertwb commented Dec 4, 2010

I noticed running the tests that they're much more verbose than they used to be, are you printing the gcc command somewhere?

@robertwb
Copy link
Contributor

robertwb commented Dec 5, 2010

OK, there's something up with the GdbDebuggerTestCase that's resetting stderror (?) and making the tests much more verbose than they should be.

@markflorisson
Copy link
Contributor Author

I'll have a look. It's probably distutils doing that. Basically what GdbDebuggerTestCase should do is compile Cython/Debugger/Tests/codefile with the --inplace and --pyrex-gdb command line options and compile the resulting C file to an importable extension module, all this preferably in a temporary folder. Currently it just uses distutils with Cython's build_ext to do that.

@robertwb
Copy link
Contributor

robertwb commented Dec 5, 2010

Yes, it's probably distutils doing the printing. The issue is that when your tests are run, its output no longer gets suppressed for all subsequent passing tests like it used to, so I wonder if it's a side effect of some stream plumbing or something.

@markflorisson
Copy link
Contributor Author

What I meant was, "it's probably distutils that's touching sys.stderr", because I don't touch it for the test process (only in the debugger subprocesses). But I'll have a closer look at it.

@robertwb
Copy link
Contributor

robertwb commented Dec 5, 2010

Thanks. In the worst case, the solution might be to run those tests in a subprocess to avoid the unwanted side affects, but hopefully there's a better solution.

@markflorisson
Copy link
Contributor Author

I fixed it by not invoking distutils.core.setup. It was not replacing sys.stderr, but it did something. Instead, I used runtests.CythonCompileTestCase. Tests that should be run in gdb are also skipped when gdb >= 7.2 is not available, in which case a warning is issued.

@robertwb
Copy link
Contributor

robertwb commented Dec 9, 2010

Excellent. As I said on the list, I'd love for this to get some more testing, but as it's non-invasive when turned off, it's certainly worth pushing. Thanks!

@robertwb
Copy link
Contributor

robertwb commented Dec 9, 2010

OK, now (on a different machine) I'm getting issues like

https://sage.math.washington.edu:8091/hudson/job/cython-devel-build-py25/528/console

when testing. I'm going to keep the code in but disable the tests for now, and we should make sure in the documentation to mention that this is very new and still somewhat experimental.

@markflorisson
Copy link
Contributor Author

Could you run the test again? It should compile with -fPIC now, which hopefully fixes the issue.

I will update the documentation to have it mention that the functionality is experimental, and 2.6+ compatible. The problem with Python 2.5 is that is doesn't have flags like e.g. Py_TPFLAGS_TYPE_SUBCLASS, which means it's relatively hard to find out the right type you can downcast too (you'd have to walk up the MRO until you find a supported type and default back to "some python object"). The debugger package is omitted for 2.5 now.

Maybe I should also add a section on how to use it with Python 3, what do you think?

@markflorisson
Copy link
Contributor Author

Are we still using TRAC? If so, could we add a Debugger ticket category?

@robertwb
Copy link
Contributor

robertwb commented Dec 9, 2010

OK, 2.6+ sounds safer. Yes, a section on Python 3 would be useful. (I don't use Python 3 myself much yet, but a growing number do.) I added a component to Trac. Soon we'll migrate off, but for the moment I figured that the release is more urgent.

Have you tried any of this on Windows? (I haven't, and we don't have Windows in our buildbot either, so I'd hate to unknowingly break those users.)

@markflorisson
Copy link
Contributor Author

Not yet, but I don't think a lot of windows users use gdb. But I guess I should at least test that the Cython compiler works with (and without) the --gdb flag and that the test suite can still be run. I'll give it a swing later tonight as I'll mostly be unavailable tomorrow and this weekend.

@robertwb
Copy link
Contributor

robertwb commented Dec 9, 2010

Thanks. Yes, for Windows users I'm mostly concerned at this point that it doesn't crash horribly when they try to use it and the tests are OK.

@markflorisson
Copy link
Contributor Author

Well, I "tested" it, which means I tested the '--gdb' flag to 'cython', but I wasn't able to run the test suite because I don't have a C compiler installed, and the gdb I have isn't linked to Python. But Cython produced the right debug files. I really don't expect any issue on Windows and I doubt many people can even get python, cython, a C compiler and gdb with python working on windows, but really, I wouldn't know as I never use it.

I think we can close this issue unless people specifically find problems.

robertwb pushed a commit that referenced this pull request Oct 22, 2015
robertwb pushed a commit that referenced this pull request Mar 14, 2017
IPython magic only import things in `"__all__"`
@soreau soreau mentioned this pull request Sep 10, 2017
scoder pushed a commit that referenced this pull request Oct 27, 2018
scoder pushed a commit that referenced this pull request May 6, 2019
Resync with Cython repo
mattip referenced this pull request in mattip/cython Oct 4, 2019
TEST: update refcounts in tests
r-barnes added a commit to r-barnes/cython that referenced this pull request Apr 14, 2022
Some code I have uses lxml. When I compile and run lxml with LLVM-12's undefined behaviour sanitizer, I see this:
```
lxml/4.6.3/cython_parts=etree.c/etree.c:191228:65: runtime error: applying zero offset to null pointer
    #0 0x7faedc799e7c in __Pyx_PyList_GetSlice lxml/4.6.3/cython_parts=etree.c/etree.c:191228
    cython#1 0x7faedc6b3723 in __pyx_f_4lxml_5etree_9_ErrorLog_copy lxml/4.6.3/cython_parts=etree.c/etree.c:38092
    cython#2 0x7faedc77636e in __pyx_f_4lxml_5etree___copyGlobalErrorLog lxml/4.6.3/cython_parts=etree.c/etree.c:40203
    cython#3 0x7faedc775a5c in __pyx_pf_4lxml_5etree_9LxmlError___init__ lxml/4.6.3/cython_parts=etree.c/etree.c:21080
    cython#4 0x7faedc774dff in __pyx_pw_4lxml_5etree_9LxmlError_1__init__ lxml/4.6.3/cython_parts=etree.c/etree.c:21019
```
which implies that Cython's generating undefined behaviour.

Making an early exit from the modified function (essentially by hoisting an early exit from one level lower in the stack) avoids this.
scoder pushed a commit that referenced this pull request Apr 18, 2022
…lity function (GH-4734)

Some code I have uses lxml. When I compile and run lxml with LLVM-12's undefined behaviour sanitizer, I see this:
```
lxml/4.6.3/cython_parts=etree.c/etree.c:191228:65: runtime error: applying zero offset to null pointer
    #0 0x7faedc799e7c in __Pyx_PyList_GetSlice lxml/4.6.3/cython_parts=etree.c/etree.c:191228
    #1 0x7faedc6b3723 in __pyx_f_4lxml_5etree_9_ErrorLog_copy lxml/4.6.3/cython_parts=etree.c/etree.c:38092
    #2 0x7faedc77636e in __pyx_f_4lxml_5etree___copyGlobalErrorLog lxml/4.6.3/cython_parts=etree.c/etree.c:40203
    #3 0x7faedc775a5c in __pyx_pf_4lxml_5etree_9LxmlError___init__ lxml/4.6.3/cython_parts=etree.c/etree.c:21080
    #4 0x7faedc774dff in __pyx_pw_4lxml_5etree_9LxmlError_1__init__ lxml/4.6.3/cython_parts=etree.c/etree.c:21019
```
which implies that Cython's generating undefined behaviour.

Making an early exit from the modified function (essentially by hoisting an early exit from one level lower in the stack) avoids this.
navytux added a commit to navytux/cython that referenced this pull request Jun 5, 2022
…d of object

object means the argument is always non-NULL valid Python object, while
PyObject* argument can be generally NULL. If the argument is indeed
passed as NULL, and we declare it as object, generated code will crash
while trying to incref it.

Quoting cython#4822:

    object.pxd currently declares `newfunc` as follows:

    ```pyx
    ctypedef object (*newfunc)(cpython.type.type, object, object)  # (type, args, kwargs)
    ```

    which implies that `args` and `kwargs` are always live objects and cannot be NULL.

    However Python can, and does, call tp_new with either args=NULL, or kwargs=NULL or both. And in such cases this leads to segfault in automatically-generated __Pyx_INCREF for args or kw.

    The fix is to change `object` to `PyObject*` for both args and kwargs.

    Please see below for details:

    ```cython
    # cython: language_level=3
    from cpython cimport newfunc, type as cpytype, Py_TYPE

    cdef class X:
        cdef int i
        def __init__(self, i):
            self.i = i
        def __repr__(self):
            return 'X(%d)' % self.i

    cdef newfunc _orig_tp_new = Py_TYPE(X(0)).tp_new

    cdef object _trace_tp_new(cpytype cls, object args, object kw):
        print('_trace_tp_new', cls, args, kw)
        return _orig_tp_new(cls, args, kw)

    Py_TYPE(X(0)).tp_new = _trace_tp_new

    x = X(123)
    print(x)
    ```

    ```console
    (neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ cythonize -i x.pyx
    Compiling /home/kirr/src/tools/go/pygolang/x.pyx because it changed.
    [1/1] Cythonizing /home/kirr/src/tools/go/pygolang/x.pyx
    running build_ext
    building 'x' extension
    ...
    x86_64-linux-gnu-gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O2 -Wall -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -g -fwrapv -O2 -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -I/home/kirr/src/wendelin/venv/py3.venv/include -I/usr/include/python3.9 -c /home/kirr/src/tools/go/pygolang/x.c -o /home/kirr/src/tools/go/pygolang/tmpqkz1r96s/home/kirr/src/tools/go/pygolang/x.o
    x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-z,relro -g -fwrapv -O2 -Wl,-z,relro -g -fwrapv -O2 -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 /home/kirr/src/tools/go/pygolang/tmpqkz1r96s/home/kirr/src/tools/go/pygolang/x.o -o /home/kirr/src/tools/go/pygolang/x.cpython-39-x86_64-linux-gnu.so
    ```

    ```console
    (neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ python -c 'import x'
    Ошибка сегментирования (стек памяти сброшен на диск)
    ```

    ```console
    (neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ gdb python core
    ...
    Reading symbols from python...
    Reading symbols from /usr/lib/debug/.build-id/f9/02f8a561c3abdb9c8d8c859d4243bd8c3f928f.debug...
    [New LWP 218557]
    [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
    Core was generated by `python -c import x'.
    Program terminated with signal SIGSEGV, Segmentation fault.
    #0  _Py_INCREF (op=0x0) at /usr/include/python3.9/object.h:408
    408         op->ob_refcnt++;

    (gdb) bt 5
    #0  _Py_INCREF (op=0x0) at /usr/include/python3.9/object.h:408
    cython#1  __pyx_f_1x__trace_tp_new (__pyx_v_cls=0x7f5ce75e6880 <__pyx_type_1x_X>, __pyx_v_args=(123,), __pyx_v_kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:1986
    cython#2  0x000000000051dd7e in type_call (type=type@entry=0x7f5ce75e6880 <__pyx_type_1x_X>, args=args@entry=(123,), kwds=kwds@entry=0x0)
        at ../Objects/typeobject.c:1014
    cython#3  0x00007f5ce75df8d4 in __Pyx_PyObject_Call (func=<type at remote 0x7f5ce75e6880>, arg=(123,), kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:3414
    cython#4  0x00007f5ce75df276 in __pyx_pymod_exec_x (__pyx_pyinit_module=<optimized out>) at /home/kirr/src/tools/go/pygolang/x.c:3017
    (More stack frames follow...)

    (gdb) f 1
    cython#1  __pyx_f_1x__trace_tp_new (__pyx_v_cls=0x7f5ce75e6880 <__pyx_type_1x_X>, __pyx_v_args=(123,), __pyx_v_kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:1986
    1986      __Pyx_INCREF(__pyx_v_kw);
    ```

-> Change newfunc signature to use PyObject* instead of object to fix it.

With this fix, and test example updates to account for object -> PyObject* change as follows ...

    --- a/x.pyx.kirr
    +++ b/x.pyx
    @@ -1,5 +1,5 @@
     # cython: language_level=3
    -from cpython cimport newfunc, type as cpytype, Py_TYPE
    +from cpython cimport newfunc, type as cpytype, Py_TYPE, PyObject

     cdef class X:
         cdef int i
    @@ -10,8 +10,12 @@ cdef class X:

     cdef newfunc _orig_tp_new = Py_TYPE(X(0)).tp_new

    -cdef object _trace_tp_new(cpytype cls, object args, object kw):
    -    print('_trace_tp_new', cls, args, kw)
    +cdef object xobject(PyObject* x):
    +    return "null"  if x == NULL  else \
    +           <object>x
    +
    +cdef object _trace_tp_new(cpytype cls, PyObject* args, PyObject* kw):
    +    print('_trace_tp_new', cls, xobject(args), xobject(kw))
         return _orig_tp_new(cls, args, kw)

     Py_TYPE(X(0)).tp_new = _trace_tp_new

... it works as expected without crashing:

    $ python -c 'import x'
    _trace_tp_new <type 'x.X'> (123,) null
    X(123)

Fixes: cython#4822
da-woods pushed a commit that referenced this pull request Jul 3, 2022
…d of object (#4823)

object means the argument is always non-NULL valid Python object, while
PyObject* argument can be generally NULL. If the argument is indeed
passed as NULL, and we declare it as object, generated code will crash
while trying to incref it.

Quoting #4822:

    object.pxd currently declares `newfunc` as follows:

    ```pyx
    ctypedef object (*newfunc)(cpython.type.type, object, object)  # (type, args, kwargs)
    ```

    which implies that `args` and `kwargs` are always live objects and cannot be NULL.

    However Python can, and does, call tp_new with either args=NULL, or kwargs=NULL or both. And in such cases this leads to segfault in automatically-generated __Pyx_INCREF for args or kw.

    The fix is to change `object` to `PyObject*` for both args and kwargs.

    Please see below for details:

    ```cython
    # cython: language_level=3
    from cpython cimport newfunc, type as cpytype, Py_TYPE

    cdef class X:
        cdef int i
        def __init__(self, i):
            self.i = i
        def __repr__(self):
            return 'X(%d)' % self.i

    cdef newfunc _orig_tp_new = Py_TYPE(X(0)).tp_new

    cdef object _trace_tp_new(cpytype cls, object args, object kw):
        print('_trace_tp_new', cls, args, kw)
        return _orig_tp_new(cls, args, kw)

    Py_TYPE(X(0)).tp_new = _trace_tp_new

    x = X(123)
    print(x)
    ```

    ```console
    (neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ cythonize -i x.pyx
    Compiling /home/kirr/src/tools/go/pygolang/x.pyx because it changed.
    [1/1] Cythonizing /home/kirr/src/tools/go/pygolang/x.pyx
    running build_ext
    building 'x' extension
    ...
    x86_64-linux-gnu-gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O2 -Wall -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -g -fwrapv -O2 -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -I/home/kirr/src/wendelin/venv/py3.venv/include -I/usr/include/python3.9 -c /home/kirr/src/tools/go/pygolang/x.c -o /home/kirr/src/tools/go/pygolang/tmpqkz1r96s/home/kirr/src/tools/go/pygolang/x.o
    x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-z,relro -g -fwrapv -O2 -Wl,-z,relro -g -fwrapv -O2 -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 /home/kirr/src/tools/go/pygolang/tmpqkz1r96s/home/kirr/src/tools/go/pygolang/x.o -o /home/kirr/src/tools/go/pygolang/x.cpython-39-x86_64-linux-gnu.so
    ```

    ```console
    (neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ python -c 'import x'
    Ошибка сегментирования (стек памяти сброшен на диск)
    ```

    ```console
    (neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ gdb python core
    ...
    Reading symbols from python...
    Reading symbols from /usr/lib/debug/.build-id/f9/02f8a561c3abdb9c8d8c859d4243bd8c3f928f.debug...
    [New LWP 218557]
    [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
    Core was generated by `python -c import x'.
    Program terminated with signal SIGSEGV, Segmentation fault.
    #0  _Py_INCREF (op=0x0) at /usr/include/python3.9/object.h:408
    408         op->ob_refcnt++;

    (gdb) bt 5
    #0  _Py_INCREF (op=0x0) at /usr/include/python3.9/object.h:408
    #1  __pyx_f_1x__trace_tp_new (__pyx_v_cls=0x7f5ce75e6880 <__pyx_type_1x_X>, __pyx_v_args=(123,), __pyx_v_kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:1986
    #2  0x000000000051dd7e in type_call (type=type@entry=0x7f5ce75e6880 <__pyx_type_1x_X>, args=args@entry=(123,), kwds=kwds@entry=0x0)
        at ../Objects/typeobject.c:1014
    #3  0x00007f5ce75df8d4 in __Pyx_PyObject_Call (func=<type at remote 0x7f5ce75e6880>, arg=(123,), kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:3414
    #4  0x00007f5ce75df276 in __pyx_pymod_exec_x (__pyx_pyinit_module=<optimized out>) at /home/kirr/src/tools/go/pygolang/x.c:3017
    (More stack frames follow...)

    (gdb) f 1
    #1  __pyx_f_1x__trace_tp_new (__pyx_v_cls=0x7f5ce75e6880 <__pyx_type_1x_X>, __pyx_v_args=(123,), __pyx_v_kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:1986
    1986      __Pyx_INCREF(__pyx_v_kw);
    ```

-> Change newfunc signature to use PyObject* instead of object to fix it.

With this fix, and test example updates to account for object -> PyObject* change as follows ...

    --- a/x.pyx.kirr
    +++ b/x.pyx
    @@ -1,5 +1,5 @@
     # cython: language_level=3
    -from cpython cimport newfunc, type as cpytype, Py_TYPE
    +from cpython cimport newfunc, type as cpytype, Py_TYPE, PyObject

     cdef class X:
         cdef int i
    @@ -10,8 +10,12 @@ cdef class X:

     cdef newfunc _orig_tp_new = Py_TYPE(X(0)).tp_new

    -cdef object _trace_tp_new(cpytype cls, object args, object kw):
    -    print('_trace_tp_new', cls, args, kw)
    +cdef object xobject(PyObject* x):
    +    return "null"  if x == NULL  else \
    +           <object>x
    +
    +cdef object _trace_tp_new(cpytype cls, PyObject* args, PyObject* kw):
    +    print('_trace_tp_new', cls, xobject(args), xobject(kw))
         return _orig_tp_new(cls, args, kw)

     Py_TYPE(X(0)).tp_new = _trace_tp_new

... it works as expected without crashing:

    $ python -c 'import x'
    _trace_tp_new <type 'x.X'> (123,) null
    X(123)

Fixes: #4822
scoder pushed a commit that referenced this pull request Jul 4, 2022
…d of object (#4823)

object means the argument is always non-NULL valid Python object, while
PyObject* argument can be generally NULL. If the argument is indeed
passed as NULL, and we declare it as object, generated code will crash
while trying to incref it.

Quoting #4822:

    object.pxd currently declares `newfunc` as follows:

    ```pyx
    ctypedef object (*newfunc)(cpython.type.type, object, object)  # (type, args, kwargs)
    ```

    which implies that `args` and `kwargs` are always live objects and cannot be NULL.

    However Python can, and does, call tp_new with either args=NULL, or kwargs=NULL or both. And in such cases this leads to segfault in automatically-generated __Pyx_INCREF for args or kw.

    The fix is to change `object` to `PyObject*` for both args and kwargs.

    Please see below for details:

    ```cython
    # cython: language_level=3
    from cpython cimport newfunc, type as cpytype, Py_TYPE

    cdef class X:
        cdef int i
        def __init__(self, i):
            self.i = i
        def __repr__(self):
            return 'X(%d)' % self.i

    cdef newfunc _orig_tp_new = Py_TYPE(X(0)).tp_new

    cdef object _trace_tp_new(cpytype cls, object args, object kw):
        print('_trace_tp_new', cls, args, kw)
        return _orig_tp_new(cls, args, kw)

    Py_TYPE(X(0)).tp_new = _trace_tp_new

    x = X(123)
    print(x)
    ```

    ```console
    (neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ cythonize -i x.pyx
    Compiling /home/kirr/src/tools/go/pygolang/x.pyx because it changed.
    [1/1] Cythonizing /home/kirr/src/tools/go/pygolang/x.pyx
    running build_ext
    building 'x' extension
    ...
    x86_64-linux-gnu-gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O2 -Wall -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -g -fwrapv -O2 -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -I/home/kirr/src/wendelin/venv/py3.venv/include -I/usr/include/python3.9 -c /home/kirr/src/tools/go/pygolang/x.c -o /home/kirr/src/tools/go/pygolang/tmpqkz1r96s/home/kirr/src/tools/go/pygolang/x.o
    x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-z,relro -g -fwrapv -O2 -Wl,-z,relro -g -fwrapv -O2 -g -ffile-prefix-map=/build/python3.9-RNBry6/python3.9-3.9.2=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 /home/kirr/src/tools/go/pygolang/tmpqkz1r96s/home/kirr/src/tools/go/pygolang/x.o -o /home/kirr/src/tools/go/pygolang/x.cpython-39-x86_64-linux-gnu.so
    ```

    ```console
    (neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ python -c 'import x'
    Ошибка сегментирования (стек памяти сброшен на диск)
    ```

    ```console
    (neo) (py3.venv) (g.env) kirr@deca:~/src/tools/go/pygolang$ gdb python core
    ...
    Reading symbols from python...
    Reading symbols from /usr/lib/debug/.build-id/f9/02f8a561c3abdb9c8d8c859d4243bd8c3f928f.debug...
    [New LWP 218557]
    [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
    Core was generated by `python -c import x'.
    Program terminated with signal SIGSEGV, Segmentation fault.
    #0  _Py_INCREF (op=0x0) at /usr/include/python3.9/object.h:408
    408         op->ob_refcnt++;

    (gdb) bt 5
    #0  _Py_INCREF (op=0x0) at /usr/include/python3.9/object.h:408
    #1  __pyx_f_1x__trace_tp_new (__pyx_v_cls=0x7f5ce75e6880 <__pyx_type_1x_X>, __pyx_v_args=(123,), __pyx_v_kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:1986
    #2  0x000000000051dd7e in type_call (type=type@entry=0x7f5ce75e6880 <__pyx_type_1x_X>, args=args@entry=(123,), kwds=kwds@entry=0x0)
        at ../Objects/typeobject.c:1014
    #3  0x00007f5ce75df8d4 in __Pyx_PyObject_Call (func=<type at remote 0x7f5ce75e6880>, arg=(123,), kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:3414
    #4  0x00007f5ce75df276 in __pyx_pymod_exec_x (__pyx_pyinit_module=<optimized out>) at /home/kirr/src/tools/go/pygolang/x.c:3017
    (More stack frames follow...)

    (gdb) f 1
    #1  __pyx_f_1x__trace_tp_new (__pyx_v_cls=0x7f5ce75e6880 <__pyx_type_1x_X>, __pyx_v_args=(123,), __pyx_v_kw=0x0) at /home/kirr/src/tools/go/pygolang/x.c:1986
    1986      __Pyx_INCREF(__pyx_v_kw);
    ```

-> Change newfunc signature to use PyObject* instead of object to fix it.

With this fix, and test example updates to account for object -> PyObject* change as follows ...

    --- a/x.pyx.kirr
    +++ b/x.pyx
    @@ -1,5 +1,5 @@
     # cython: language_level=3
    -from cpython cimport newfunc, type as cpytype, Py_TYPE
    +from cpython cimport newfunc, type as cpytype, Py_TYPE, PyObject

     cdef class X:
         cdef int i
    @@ -10,8 +10,12 @@ cdef class X:

     cdef newfunc _orig_tp_new = Py_TYPE(X(0)).tp_new

    -cdef object _trace_tp_new(cpytype cls, object args, object kw):
    -    print('_trace_tp_new', cls, args, kw)
    +cdef object xobject(PyObject* x):
    +    return "null"  if x == NULL  else \
    +           <object>x
    +
    +cdef object _trace_tp_new(cpytype cls, PyObject* args, PyObject* kw):
    +    print('_trace_tp_new', cls, xobject(args), xobject(kw))
         return _orig_tp_new(cls, args, kw)

     Py_TYPE(X(0)).tp_new = _trace_tp_new

... it works as expected without crashing:

    $ python -c 'import x'
    _trace_tp_new <type 'x.X'> (123,) null
    X(123)

Fixes: #4822
scoder pushed a commit that referenced this pull request Dec 28, 2023
Address sanitizers consistently give an error on running the "parallel" test, saying that it ends up getting freed after releasing and reacquiring the gil, and thus we can't keep using the same threadstate pointer.

Sanitizer finding:

==28037==ERROR: AddressSanitizer: heap-use-after-free on address 0x6130000325b8 at pc 0x7f9cb3123c0d bp 0x7f9cad6ddc10 sp 0x7f9cad6ddc08
READ of size 8 at 0x6130000325b8 thread T41
    #0 0x7f9cb3123c0c in __Pyx__ExceptionReset <path>/TEST_TMP/run/c/parallel/parallel.c:42891
    #1 0x7f9cb3172593 in __pyx_f_8parallel_parallel_exc_nogil_swallow._omp_fn.0 <path>/TEST_TMP/run/c/parallel/parallel.c:30050
    #2 0x7f9cc66cdd2d  (/lib64/libgomp.so.1+0x21d2d) (BuildId: 94e48b16f615cdab2143a0c2b3f15d0b8d81d0e6)
    #3 0x7f9cc688ff43 in start_thread (/lib64/libc.so.6+0x8ff43) (BuildId: 099807798c1de6cbe241dc4e3dd67a7326a2c29f)
    #4 0x7f9cc69184cb in __clone3 (/lib64/libc.so.6+0x1184cb) (BuildId: 099807798c1de6cbe241dc4e3dd67a7326a2c29f)
scoder pushed a commit that referenced this pull request Dec 28, 2023
Address sanitizers consistently give an error on running the "parallel" test, saying that it ends up getting freed after releasing and reacquiring the gil, and thus we can't keep using the same threadstate pointer.

Sanitizer finding:

==28037==ERROR: AddressSanitizer: heap-use-after-free on address 0x6130000325b8 at pc 0x7f9cb3123c0d bp 0x7f9cad6ddc10 sp 0x7f9cad6ddc08
READ of size 8 at 0x6130000325b8 thread T41
    #0 0x7f9cb3123c0c in __Pyx__ExceptionReset <path>/TEST_TMP/run/c/parallel/parallel.c:42891
    #1 0x7f9cb3172593 in __pyx_f_8parallel_parallel_exc_nogil_swallow._omp_fn.0 <path>/TEST_TMP/run/c/parallel/parallel.c:30050
    #2 0x7f9cc66cdd2d  (/lib64/libgomp.so.1+0x21d2d) (BuildId: 94e48b16f615cdab2143a0c2b3f15d0b8d81d0e6)
    #3 0x7f9cc688ff43 in start_thread (/lib64/libc.so.6+0x8ff43) (BuildId: 099807798c1de6cbe241dc4e3dd67a7326a2c29f)
    #4 0x7f9cc69184cb in __clone3 (/lib64/libc.so.6+0x1184cb) (BuildId: 099807798c1de6cbe241dc4e3dd67a7326a2c29f)
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants