[Cython][FFI] Fix crash when call del operator for handle #17190
Conversation
In case of cython when we create a set function for property then the
following code will be generated:
```
static int __pyx_setprop_4test_9TestClass_handle(PyObject *o, PyObject *v, CYTHON_UNUSED void *x) {
if (v) {
return __pyx_pw_4test_9TestClass_6handle_3__set__(o, v);
}
else {
PyErr_SetString(PyExc_NotImplementedError, "__del__");
return -1;
}
}
```
And when we call operator `del` for this handler, then the memory will
be released and operator `__set__` will be called for NULL object. In
this case an exception that operator `__del__` is not implemented will
be generated. To avoid this problem we need to declare `__del__`
function for each property where we define operator `__set__`.
echuraev
force-pushed
the
echuraev/fix_cython_crash
branch
from
51c37d3 to
0044111
Compare
echuraev
changed the title
|
cc: @tqchen, @vinx13, @Lunderberg, @masahi |
|
can you show a test case that would trigger the error? I am still not very sure what can trigger such error. del function was not supposed to be called explicitly One thing we need to be very careful about is whethere there would be double free issue, since the free function was called in dealloc as well |
|
@tqchen thank you for your review! I'll work on your comments tomorrow. About the test case. Originally, problem appeared in disco. When we run model on multiple GPU and TVM was compiled with cython, then we have such crash: The crash happen here when we try to delete I simplified the problem and we can create a simple cdef class TestClass:
property handle:
def __get__(self):
print("Getter")
return None
def __set__(self, value):
print("Setter")
returnThe property in the code above has the same methods as exists in packed_func.pxi. Next, create a from setuptools import setup
from Cython.Build import cythonize
setup(
ext_modules=cythonize("test.pyx"),
)And build it by the following command: python setup.py build_ext --inplaceAfter that, we can reproduce original issue: $ python3
>>> import test
>>> t = test.TestClass()
>>> del t.handle
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
NotImplementedError: __del__
>>>If we add an implementation of
$ python3
>>> import test
>>> t = test.TestClass()
>>> del t.handle
Dtor
>>>And after adding implementation of |
|
@tqchen I modified As a test for these changes, we can use two tests from |
|
@echuraev thank you! I think in this particular case we should change https://github.com/apache/tvm/blob/main/python/tvm/runtime/disco/session.py#L95 to dref.handle = Noneas in https://github.com/apache/tvm/blob/main/python/tvm/runtime/disco/session.py#L83 So we don't have to add del function. |
|
I think I agree with @tqchen here. The If there are other use cases of |
|
@tqchen, @Lunderberg thank you for your review. Removed implementation of |
|
@tvm-bot rerun |
There was a problem hiding this comment.
Thank you for making the changes, and I think it looks good. Given that this would impact every use of session.load_module in cython, I'm guessing that it would have been caught by the unit tests, if they were run in CI. (Resolving failures exposed by the disco tests in PR #16975 has been on the back burner for me.)
In case of cython when we create a set function for property then the following code will be generated:
And when we call operator
delfor this handler, then the memory will be released and operator__set__will be called for NULL object. In this case an exception that operator__del__is not implemented will be generated. To avoid this problem we need to declare__del__function for each property where we define operator__set__.