[Cython] Bug in Cython producing incorrect C code

[mark florisson]

On 26 January 2012 18:51, Vitja Makarov <vitja.makarov at gmail.com> wrote:
> 2012/1/26 mark florisson <markflorisson88 at gmail.com>:
>> On 26 January 2012 06:39, Vitja Makarov <vitja.makarov at gmail.com> wrote:
>>> 2012/1/25 Stefan Behnel <stefan_ml at behnel.de>:
>>>> mark florisson, 24.01.2012 14:53:
>>>>> On 24 January 2012 11:37, Konrad Hinsen wrote:
>>>>>> Compiling the attached Cython file produced the attached C file which
>>>>>> has errors in lines 532-534:
>>>>>>
>>>>>>  __pyx_v_self->xx = None;
>>>>>>  __pyx_v_self->yy = None;
>>>>>>  __pyx_v_self->zz = None;
>>>>>>
>>>>>> There is no C symbol "None", so this doesn't compile.
>>>>>>
>>>>>> I first noticed the bug in Cython 0.15, but it's still in the latest
>>>>>> revision from Github.
>>>>>
>>>>> Hm, it seems the problem is that the call to the builtin float results
>>>>> in SimpleCallNode being replaced with PythonCApiNode, which then
>>>>> generates the result code, but the list of coerced nodes are
>>>>> CloneNodes of the original rhs, and CloneNode does not generate the
>>>>> result code of the original rhs (i.e. allocate and assign to a temp),
>>>>> which results in a None result.
>>>>
>>>> Back to the old idea of separating the type analysis into 1) a basic
>>>> typing, inference and entry creation step and 2) a proper type analysis,
>>>> coercion, etc. step.
>>>>
>>>
>>> Yeah! I think the issue must be fixed before release. We can start
>>> moving slowly in this direction and split
>>> CascadedAssignmentNode.analyse_types into parts:
>>>  - normal analyse_types()/expressions()
>>>  - create clone nodes at some late stage
>>
>> At what stage would the stage 2) proper type analysis take place?
>> Basically nodes may be replaced at any point, and I'm not sure you
>> want to wait until just before code generation to do the coercions
>> (e.g.  GILCheck won't catch coercions to object, although assignment
>> nodes seem to check manually).
>>
>
> That must be run before GilCheck. Stage 2 is "I believe the tree won't
> change much later"
>
>
>> I think this problem can trivially be solved by creating a ProxyNode
>> that should never be replaced by any transform, but it's argument may
>> be replaced. So you wrap self.rhs in a ProxyNode and use that to
>> create your CloneNodes.
>>
>
> Do you mean proxy node to be something like this:
>
> class ProxyNode(object):
>    def __init__(self, obj):
>        object.__setattr__(self, '_obj', obj)
>
>    def __getattr__(self, key):
>        return getattr(self._obj, key)
>
>    def __setattr__(self, key, value):
>        setattr(self._obj, key, value)
>
> That might help but I'm not sure how evil that is.
>
> It also will require TreeVisitor.find_handler() modification. Node
> replacement could be avoided by introducing ProxyProperty()
>
> I see another problem with proxies: CloneNode or its owner may depend
> on content of the argument so when it's changed things can be messed
> up.

Not quite like that. It should be a regular node that is ignored by
transforms but delegates a few methods. e.g.

class ProxyNode(object):
    child_attrs = ['arg']

    def __init__(self, arg):
        self.arg = arg

    def result(self):
        return self.arg.result()

    def generate_result_code(self, code):
        return self.arg.generate_result_code(code)

and that's pretty much it (untested). And CloneNode doesn't depend on
any specific node, nor should any code wrapping its nodes in
ProxyNodes (obviously).

The nodes sole purpose is to create an indirection, so that when
previously self.rhs got replaced, now self.rhs.arg will be replaced,
so self.rhs can be safely shared with other CloneNodes.

> --
> vitja.
> _______________________________________________
> cython-devel mailing list
> cython-devel at python.org
> http://mail.python.org/mailman/listinfo/cython-devel