Re: Compiler crash when installing Scipy from sources with Cython 0.19

[Pauli Virtanen]

27.04.2013 13:24, Jose Guzman kirjoitti:
> I successfully installed Cython 0.19 and wanted to install Scipy from
> the sources too. When I tried, I found the following error:
>
> warning: ckdtree.pyx:1212:24: the result of using negative indices
> inside of code sections marked as 'wraparound=False' is undefined

You using an unreleased version of Cython --- that message was added
only after 0.19.

Although it prints a warning, the code generated by Cython is correct
here, so I wonder if the wraparound warning is too aggressive.

--
Pauli Virtanen

[Stefan Behnel]

Pauli Virtanen, 27.04.2013 12:54:

> 27.04.2013 13:24, Jose Guzman kirjoitti:
>> I successfully installed Cython 0.19 and wanted to install Scipy from
>> the sources too. When I tried, I found the following error:
>>
>> warning: ckdtree.pyx:1212:24: the result of using negative indices
>> inside of code sections marked as 'wraparound=False' is undefined
>
> You using an unreleased version of Cython --- that message was added
> only after 0.19.

Yes, it's a new warning that should make users aware of fragile code. The
crash was an oversight.

> Although it prints a warning, the code generated by Cython is correct
> here, so I wonder if the wraparound warning is too aggressive.

There certainly is space for improvement.

Stefan

[Stefan Behnel]

Stefan Behnel, 29.04.2013 19:45:

... although there is really no guarantee that the code will continue to be
correct, even if it is now. I'm not even sure it is safe in 0.19. It
certainly depends a lot on your exact code, which makes it difficult to
guess if it's safe in a given situation or not. Thus the visible warning.

If users disable wraparound, Cython is free to optimise away any negative
index checks and/or calculations that it can, and the "normal object"
slicing code makes use of this in Python 2 now.

Stefan

[Nikita Nemkin]

On Tue, 30 Apr 2013 00:02:52 +0600, Stefan Behnel <stef...@behnel.de>
wrote:

After some thinking I believe that static (compile-time computable)
negative indexes should be supported regardless of compilation mode.
The purpose of wraparound=False is to eliminate _potentially_ unnecessary
checks and len() calls.
With a static negative index there is no uncertainity: len() must
be called in any case, there is nothing to eliminate and no reason
to miscompile.


Best regards,
Nikita Nemkin

[Robert Bradshaw]

I think the transformation

print L[-1]

to

cdef int x = -1
print L[x]

or

cdef enum X:
x = -1

[... lots of intervening code, possibly in another file ...]

print L[x]

should not cause any semantic change (and quite possibly a future
optimization would "undo" it which would make distinguishing the cases
even worse). Much cleaner to say that if users disable wraparound, we
will make an effort to eliminate wraparound checks. L[len(L) - 1]
isn't that onerous to write in this mode.

- Robert

[Stefan Behnel]

Robert Bradshaw, 30.04.2013 06:59:

> I think the transformation
>
> print L[-1]
>
> to
>
> cdef int x = -1
> print L[x]
>
> or
>
> cdef enum X:
> x = -1
>
> [... lots of intervening code, possibly in another file ...]
>
> print L[x]
>
> should not cause any semantic change

+1

> and quite possibly a future optimization would "undo" it

I attached a patch attempt that's been resting on my hard drive for a
while. It's not a safe optimisation yet because it hits too many cases. The
main goal was to get rid of redundant variables in closures, which have a
performance impact, especially in generators. Note that the patch
completely lacks tests, especially for all those little corner cases that
can appear in closures.

If someone wants to give it a try, I think it's worth it.

Stefan