[Cython] Bad interaction between cimported types and module cleanup

[Stefan Behnel]

Lisandro Dalcin, 03.08.2012 00:36:
> Basically, the module cleanup function nullifies the type ptr of
> cimported parent types, but tp_dealloc slots of children types access
> these null pointers, then you get a segfault. Looking at Py_Finalize()
> , atexit functions are run BEFORE a gargabe collection step and the
> destroy of all modules.
> 
> @Stefan, I remember you also had issues with module cleanup and object
> deallocation. Did you find a solution?

Nothing but setting the cleanup code level from 3 down to 2.


> Could you give a quick look at the following patch?

Looks like it should handle this case, yes. Maybe we could explicitly only
apply it to cimported (i.e. non module local) types, but I guess that's
what "tp_dealloc is None" already handles.


> Do you think the
> extra pointer deref for getting Py_TYPE(o)->tp_base->tp_dealloc would
> be a performance regression?

I have no idea, but I doubt that it would be any significant, given that
the call is an indirect one already and that CPython is about to do
something with its heap memory management shortly afterwards, with
potentially a couple of additional DECREF calls in between. All of this
should be costly enough to make the difference negligible.


> diff --git a/Cython/Compiler/ModuleNode.py b/Cython/Compiler/ModuleNode.py
> index 2472de3..8758967 100644
> --- a/Cython/Compiler/ModuleNode.py
> +++ b/Cython/Compiler/ModuleNode.py
> @@ -1111,7 +1111,12 @@ class ModuleNode(Nodes.Node, Nodes.BlockNode):
>          if base_type:
>              tp_dealloc = TypeSlots.get_base_slot_function(scope, tp_slot)
>              if tp_dealloc is None:
> -                tp_dealloc = "%s->tp_dealloc" % base_type.typeptr_cname
> +                if 0:
> +                    # XXX bad interaction between
> +                    # cimported types and module cleanup
> +                    tp_dealloc = "%s->tp_dealloc" % base_type.typeptr_cname
> +                else:
> +                    tp_dealloc = "Py_TYPE(o)->tp_base->tp_dealloc"
>              code.putln(
>                      "%s(o);" % tp_dealloc)
>          else:

Any other opinions on this?

Lisandro, could you open a pull request with only the single line change
and a comment that properly explains why we are doing this?

Stefan