Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutable default arguments in FusedCFuncDefNode().make_fused_cpdef()/__pyx_fused_cpdef() cause segfaults for functions later assigned/bound to classes? #3370

Closed
will-ca opened this issue Feb 20, 2020 · 3 comments · Fixed by #3398

Comments

@will-ca
Copy link
Contributor

will-ca commented Feb 20, 2020

Whenever the signature generated for __pyx_fused_cpdef() in Cython.Compiler.FusedNode.FusedCFuncDefNode().make_fused_cpdef() contains either a mutable object or an object that contains a mutable object as a default argument, any fused cpdef functions produced by it will segfault once later dynamically bound and called as methods.

A print function placed as the first non-comment line of the function body doesn't seem to be visibly executed, so I assume the crash occurs somewhere within and as a result of how the descriptor protocol is handled by Cython.

Attribute access for the bound function works fine as well as long as it's never called, so I assume the crash occurs somewhere between invoking the descriptor protocol and successfully dispatching the arguments into the function body.

The mutable object does not have to ever be explicitly accessed or modified for the segfault to occur.

This occurs both with classes defined dynamically in Python-space and with cdef classes. It doesn't affect methods that are explicitly defined under the class— but it does still happen when the code accessing and calling the instance method is compiled with the instance statically typed.


This is probably either a specific case of a somewhat more general issue, or some kind of user error on my part. But I'm not familiar enough with Cython or low-level programming languages in general to tell which it is, and if it is a symptom of a larger issue, I don't know enough about how cyfunctions are structured and how they handle the descriptor protocol to be able to narrow it down any further.

I also tried emulating how I might imagine cyfunctions to work using plain def and cdef functions, as well as a cdef class that implements the descriptor protocol. But those all work fine, so this issue, if it is an actual issue, doesn't seem to have any chance of actually affecting normal usage.


The segfault so far seems to require two things to occur:

  • Modification to the Cython source code (but still within parameters that seem valid and work outside of this very specific case— and which may or may not also come up under normal usage or other development).
  • Compilation of a Cython file that defines a fused cpdef function and then binds it as a method by assigning it as a class attribute.

The code that I've modified whenever it's occured is around line 619 of Compiler/FusedNode.py:

def __pyx_fused_cpdef(signatures, args, kwargs, defaults):

The segfaults seem to occur once any of the arguments have default values that either are or include mutable objects:

def __pyx_fused_cpdef(signatures, args, kwargs, defaults, _newkwarg={}):
def __pyx_fused_cpdef(signatures, args, kwargs, defaults={}):
def __pyx_fused_cpdef(signatures, args, kwargs, defaults, _newkwarg=([],)):

The below forms all seem to be fine, IIRC:

def __pyx_fused_cpdef(signatures, args, kwargs, defaults, _newkwarg=()):
def __pyx_fused_cpdef(signatures, args, kwargs, defaults=None):
def __pyx_fused_cpdef(signatures, args, kwargs, defaults, _newkwarg=(None,)):

Normal functions, as well as explicitly defined methods, all seem to work fine even with the problematic forms. The segfault only seems to occur when a stand-alone function is later bound to a class:

fusedtest.py
# distutils: language = c++
import cython

def fused_bind(self, var): # Works.
	return 0

class fused_type_cls:
	meth = fused_bind # Segfaults.

class compiled_fused_type_cls:
	meth = fused_bind # Segfaults.
	def static_meth(self, var): # Works.
		return 0

def fused_type_cls_compiledtest():
	obj = compiled_fused_type_cls()
	v = 1.0
	return (obj.meth(v), obj.static_meth(v)) # Segfaults.
fusedtest.pxd
# distutils: language = c++
import cython

ctypedef fused fused_type_ohtest:
	double
	int

cpdef bint fused_bind(self, fused_type_ohtest var)

cdef class compiled_fused_type_cls:
	cpdef bint static_meth(self, fused_type_ohtest var)

@cython.locals(obj=compiled_fused_type_cls, v=cython.double)
cpdef (bint, bint) fused_type_cls_compiledtest()

This behaviour is already tested for with A().meth() under tests/run/fused_cpdef.pyx as well.

will-ca added a commit to will-ca/cython that referenced this issue Feb 20, 2020
@scoder
Copy link
Contributor

scoder commented Feb 20, 2020

Could be that you're running into a problem related to this.

Note that all the working variants that you're giving above are special because constant tuples and None are really global C constants in Cython. I'll have to dig into the generated C code to see what goes wrong with the dynamic default arguments here.

will-ca added a commit to will-ca/cython that referenced this issue Feb 20, 2020
 by keeping signature index in hopefully-non-colliding global namespace.
@da-woods
Copy link
Contributor

da-woods commented Feb 27, 2020

# Fused functions do not support dynamic defaults, only their specialisations can have them for now.

(I have no real idea for how to fix it though)

@da-woods
Copy link
Contributor

da-woods commented Mar 5, 2020

So the issue as I understand it is:

When a fused node is bound it doesn't copy across its defaults field (I've just linked to the function used for binding a fused node)

__pyx_FusedFunction_descr_get(PyObject *self, PyObject *obj, PyObject *type)

It can't directly copy the func->defaults pointer because when the bound fused node is destructed it decrefs the contents and frees the structure

if (m->defaults) {
PyObject **pydefaults = __Pyx_CyFunction_Defaults(PyObject *, m);
int i;
for (i = 0; i < m->defaults_pyobjects; i++)
Py_XDECREF(pydefaults[i]);
PyObject_Free(m->defaults);
m->defaults = NULL;
}

This is data that it doesn't own.

It can't allocate its own copy of func->defaults because it doesn't know how big it is. It only knows how many PyObject* func->defaults contains (stored as func->defaults_pyobjects).

I think the best option would be to also store func->defaults_size so that __pyx_FusedFunction_descr_get can allocate itself a copy of defaults, but maybe there's other potential fixes that I'm missing? Unless I get another suggestion I'll submit this as a PR in the near future.


Note - this is a real bug beyond just being able to use default arguments in __pyx_fused_cpdef. I can cause a segmentation fault with the following code:

ctypedef fused ID:
    int
    double

def f(self, ID x=0):
    return x+1

cdef class C:
    f = f[int]

C().f()  # segfault

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants