[Cython] Bad interaction between cimported types and module cleanup

[Stefan Behnel]

Stefan Behnel, 04.08.2012 14:59:
> Lisandro Dalcin, 03.08.2012 18:51:
>> diff --git a/Cython/Compiler/ModuleNode.py b/Cython/Compiler/ModuleNode.py
>> index 2472de3..255b047 100644
>> --- a/Cython/Compiler/ModuleNode.py
>> +++ b/Cython/Compiler/ModuleNode.py
>> @@ -1111,7 +1111,11 @@ 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
>> +                # Using the cimported base type pointer interacts
>> +                # badly with module cleanup nullyfing these pointers.
>> +                # Use instead the base type pointer from the
>> +                # instance's type pointer.
>> +                tp_dealloc = "Py_TYPE(o)->tp_base->tp_dealloc"
>>              code.putln(
>>                      "%s(o);" % tp_dealloc)
>>          else:
> 
> Tried it, doesn't work. The problem is that this always goes through the
> actual type of the object, ignoring the type hierarchy completely, which
> kills the tp_dealloc() call chain and runs into an infinite loop starting
> from the second inheritance level (or a crash because of multiple DECREF
> calls on the same fields).

Here is a fix that should handle this correctly.

https://github.com/cython/cython/commit/a8393fa58741c9ae0647e8fdec5fee4ffd91ddf9

Basically, it checks the type pointer of cimported types on tp_traverse(),
tp_clear() and tp_dealloc() and in the unlikely case that they are NULL, it
walks the type hierarchy up to the point where it finds the current
function and then calls the one in the base type.

It is somewhat involved, but I still doubt that it would lead to a real
slow down. Maybe tp_traverse() is the one that's a bit more time critical
than the others because it can be called more often, but the one additional
NULL check in the normal case still shouldn't hurt all that much.

It fixes the case in lxml for me, but I didn't shrink that down to a test
case yet. Lisandro, if you find the time, it would be nice if you could
write one up for the problem you ran into.

Stefan