Re: [Cython] Bad interaction between cimported types and module cleanup
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 > @@ -,7 +,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). Stefan ___ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
Re: [Cython] Bad interaction between cimported types and module cleanup
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 >> @@ -,7 +,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 ___ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
Re: [Cython] Bad interaction between cimported types and module cleanup
On 4 August 2012 17:50, Stefan Behnel wrote: > 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 >>> @@ -,7 +,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. > Nice! > 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. > OK. > 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. > To properly test this we would need to namespace the __cleanup functions registered with atexit, I mean to name them 'cleanup', that way we could loop over registered functions to and call them "by hand". I you agree, I can take care of this and write some tests for module cleanup. -- Lisandro Dalcin --- CIMEC (INTEC/CONICET-UNL) Predio CONICET-Santa Fe Colectora RN 168 Km 472, Paraje El Pozo 3000 Santa Fe, Argentina Tel: +54-342-4511594 (ext 1011) Tel/Fax: +54-342-4511169 ___ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
Re: [Cython] Bad interaction between cimported types and module cleanup
Lisandro Dalcin, 04.08.2012 23:33: > To properly test this we would need to namespace the __cleanup > functions registered with atexit, I mean to name them > 'cleanup' Sure, why not. > that way we could loop over registered > functions to and call them "by hand". I'm not sure if that'll work - wouldn't they still be called another time at exit? IMHO worth trying. The cleanup code should be safe enough to be reentrant, but who knows. If it's not, it might be worth making it safe enough. Then there's also the question *when* to call the cleanup function of a test module. I doubt that it's a good idea to call it from inside the module itself, but calling it from the test runner might work. > I you agree, I can take care of > this and write some tests for module cleanup. Please do. Stefan ___ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
