General discussion about the Cython source code generator for Python extensions

Lisandro Dalcin | 3 Aug 2012 00:36

Picon

[Cython] Bad interaction between cimported types and module cleanup

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.

 <at> Stefan, I remember you also had issues with module cleanup and object
deallocation. Did you find a solution?

Could you give a quick look at the following patch? Do you think the
extra pointer deref for getting Py_TYPE(o)->tp_base->tp_dealloc would
be a performance regression? BTW, The "if 0" branch is the original
code, the else branch is my proposed fix.

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
 <at>  <at>  -1111,7 +1111,12  <at>  <at>  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(

(Continue reading)

Stefan Behnel | 3 Aug 2012 06:04

Picon

Re: [Cython] Bad interaction between cimported types and module cleanup

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.
> 
>  <at> 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

(Continue reading)

Stefan Behnel | 3 Aug 2012 06:07

Picon

Re: [Cython] Bad interaction between cimported types and module cleanup

Stefan Behnel, 03.08.2012 06:04:
> Lisandro, could you open a pull request with only the single line change
> and a comment that properly explains why we are doing this?

A code comment, just to be clear.

Stefan

Lisandro Dalcin | 3 Aug 2012 18:51

Picon

Re: [Cython] Bad interaction between cimported types and module cleanup

On 3 August 2012 01:07, Stefan Behnel <stefan_ml@...> wrote:
> Stefan Behnel, 03.08.2012 06:04:
>> Lisandro, could you open a pull request with only the single line change
>> and a comment that properly explains why we are doing this?
>
> A code comment, just to be clear.
>

Is the comment blow clear enough for you?

Stefan, I've never used github seriously, and making a pull request
will require time I really do not have. The patch is really trivial,
any chance to push it directly? There is a policy is to use pull
requests for every change (sorry, I've not been following Cython
development closely)?

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
 <at>  <at>  -1111,7 +1111,11  <at>  <at>  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"

(Continue reading)

Stefan Behnel | 4 Aug 2012 08:52

Picon

Re: [Cython] Bad interaction between cimported types and module cleanup

Lisandro Dalcin, 03.08.2012 18:51:
>> Stefan Behnel, 03.08.2012 06:04:
>>> Lisandro, could you open a pull request with only the single line change
>>> and a comment that properly explains why we are doing this?
> 
> Is the comment blow clear enough for you?

Thanks. I fixed it up a little. :)

> Stefan, I've never used github seriously, and making a pull request
> will require time I really do not have.

github is really cool here, you can basically edit a source file in place
and then send it in as a pull request.

> The patch is really trivial,
> any chance to push it directly?

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

> There is a policy is to use pull
> requests for every change (sorry, I've not been following Cython
> development closely)?

Sort-of. It makes it easy to see in the history who wrote a patch and who
applied it, usually after reviewing it. It's more meant to give proper
credit to people than to force them into bureaucratics, though.

Stefan

(Continue reading)

Stefan Behnel | 4 Aug 2012 14:59

Picon

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
>  <at>  <at>  -1111,7 +1111,11  <at>  <at>  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

Stefan Behnel | 4 Aug 2012 22:50

Picon

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
>>  <at>  <at>  -1111,7 +1111,11  <at>  <at>  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

(Continue reading)

Lisandro Dalcin | 4 Aug 2012 23:33

Picon

Re: [Cython] Bad interaction between cimported types and module cleanup

On 4 August 2012 17:50, Stefan Behnel <stefan_ml@...> 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
>>>  <at>  <at>  -1111,7 +1111,11  <at>  <at>  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

(Continue reading)

Stefan Behnel | 4 Aug 2012 23:45

Picon

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
> '__<package>_<module>_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

Return

Return to gmane.comp.python.cython.devel.

Advertisement

Project Web Page

General discussion about the Cython source code generator for Python extensions

Search Archive

Language

Change language

Options

Current view: Threads only / Showing only 20 lines / Not hiding cited text.
Change to All messages, whole messages, or hide cited text.

Post a message
NNTP Newsgroup
Classic Gmane web interface
XML

XML

RSS Feed
List Information

About Gmane