SEGFAULT when using typed memoryview in structs

[frol]

Here is the reproduction code:

from cpython cimport array
import array


cdef struct MyStruct:
    #float data
    float [:] data


cdef test():
    print("TEST")

    #cdef float data = 1.
    cdef array.array data = array.array('f', [1., 2., 3.])

    cdef MyStruct s = MyStruct(data)

    print(s.data[0])
    print(s)

    cdef list l = [s]
    cdef MyStruct extracted_s = l[0]

    print(extracted_s.data[0])

    # Make sure that `data` survives at least until now
    print(data)
    print("DONE")

test()

Running the code I get printed only "TEST" and s.data[0] value. print(s) and all subsequent print calls crash Python with SEGFAULT (terminated by signal SIGSEGV (Address boundary error)).

Changing float [:] to a single float (uncomment the commented lines in the example above) makes the code work just fine.

I have tried several versions of Cython (0.24.1, 0.23.5, 0.22.1) with Python 3.5.2 and Python 2.7.12 on Arch Linux x64, and Python 2.7.3 on Ubuntu 12.04. The result is always the same - SEGFAULT.

[frol]

As a workaround, I switched to a raw float* pointer.

[c-f-h]

I also hit this. The problem is that Cython does not properly initialize memviewslices in structs, only in the top level scope (function/class).

A workaround seems to be to manually set data = None before you use it, but this will probably still leak memory. This really needs to be fixed.

[scoder]

I think the same restriction as for Python objects applies here. The lifetime of structs cannot generally be controlled by Cython, so it should rather be forbidden to use memoryviews as struct members.

[scoder]

I made this an error here: 0961b9a

[c-f-h]

But the lifetime can be controlled if the struct is a class member. Until now, I manually worked around this by nulling out the memviews on class creation and in dealloc(). Sadly, now you took away this possibility. Can you reconsider this? I'll have to restructure significant parts of my code if this becomes an error.

In general, memviews would be a great data structure, but in practice they limit you so much because they are not actually first class citizens and you aren't allowed to use them anywhere except as function locals and direct (!) class members (not in structs or arrays, e.g.). Wouldn't it be better, in the long run, to try to fix some of these shortcomings and make them more of a first class datatype?

[MarcCote]

I agree with @c-f-h. We also use a similar trick to handle memviews inside struct. Us too found ourselves in a similar situation and would have to significantly rewrite part of our code.

Could we raise a warning instead of an error (at least for 0.28)?

[c-f-h]

I think the following approach would work: distinguish between "POD" (plain old data, as in C++) structs and such which require some sort of memory management (because they contain memviews or other Python objects). POD structs would work exactly as they do now. Non-POD structs would only be allowed in places where the compiler can guarantee that they will be deallocated properly, e.g., on the stack and in class instances.

Does this sound workable? I would consider contributing to a patch, although my knowledge of the Cython codebase is rusty.

[MarcCote]

Maybe I'm missing something obvious but is there any fast way (i.e. without GIL) of creating memviews that are initialized with a float* plus some basic info (strides, shapes, etc)?

[scoder]

I disallowed this because supporting structs with memoryviews specifically as cdef class members would require us to then disallow any assignments from this member to variables (i.e. copy these structs by value), as well as taking the address of the struct, which is more difficult to track. It's much easier to disallow the whole thing. Also, while I understand that you have already structured your code using this "feature", I can't see a reason why supporting this is better than requiring users to make the memoryview a class member rather than a sub-struct member. It just leads to different ways of accessing the view, but IMHO makes it clearer what the responsibilities for the lifetime management are.

Sorry for the inconvenience, but this "feature" was inherently unsafe before. No offence, @MarcCote and @c-f-h , but I wouldn't be surprised if your code bases were also suffering from memory leaks and/or crashes under certain (rare) conditions, similar to what the OP reported. Even if you took good care to avoid this, it's an easy source of bugs for new developers in your teams who are less familiar with the constraints.

@c-f-h, I would consider a pull request that implements this as a new feature.