sagemath/sage

Upgrade to Cython 0.19

Closed this issue · 36 comments

jdemeyer commented

Upgrade to Cython 0.19.

spkg: http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.p0.spkg (spkg diff)

upstream:

  1. Bug report about segfaults in libGAP: http://trac.cython.org/cython_trac/ticket/808 (fixed in Cython 0.19)
  2. Bug with g++ on Solaris: http://trac.cython.org/cython_trac/ticket/809 (patch in spkg and submitted upstream)
  3. Bug with g++ on Solaris: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57025 (deny it's a bug)
  4. Bug with cpdef keyword arguments in lazy_import.pyx: http://trac.cython.org/cython_trac/ticket/810

With this spkg, all doctests pass, but the docbuilder (in particular the command ./sage --docbuild reference/combinat html) crashes due to a problem with lazy_import: attachment: sage_crash_W7eVMu.log The Sage library patch works around this problem.

On Itanium, there is a doctest failure

sage -t --long devel/sage/sage/structure/element.pyx
**********************************************************************
File "devel/sage/sage/structure/element.pyx", line 1733, in sage.structure.element.RingElement.__pow__
Failed example:
    p(a,2,1)
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: __pow__ dummy argument not used
Got:
    202
**********************************************************************

This is likely a bug in GCC-4.7.3, but no further investigation has been done. Compiling sage/structure/element.pyx with -Os works around the problem, see patch.

apply attachment: 14452_cython_0.19.patch

Depends on #13826

Upstream: None of the above - read trac for reasoning.

CC: @robertwb

Component: packages: standard

Author: Jeroen Demeyer

Reviewer: Robert Bradshaw

Merged: sage-5.9.rc0

Issue created by migration from https://trac.sagemath.org/ticket/14452

jdemeyer commented

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
 Upgrade to Cython 0.19 when it's released.
+
+**spkg**: [http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg](http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg) (based on [Cython-0.19 beta2](http://mail.python.org/pipermail/cython-devel/2013-April/003564.html))
jdemeyer commented
comment:2

Robert, I think there is a serious Cython bug: the __new__ method of a class might actually use the wrong class: in devel/sage/sage/libs/gap/element.pyx, the code

def GapElement_Record r = GapElement_Record.__new__(GapElement_Record)

generates

__pyx_t_1 = __pyx_tp_new_4sage_4libs_3gap_7element_GapElement(((PyTypeObject *)((PyObject*)__pyx_ptype_4sage_4libs_3gap_7element_GapElement_Record)), ((PyObject *)__pyx_empty_tuple), NULL);

Note the call to the tp_new function of GapElement (a child class), not GapElement_Record.

jdemeyer commented

Upstream: Reported upstream. No feedback yet.

jdemeyer commented

Description changed:

--- 
+++ 
@@ -1,3 +1,5 @@
 Upgrade to Cython 0.19 when it's released.
 
 **spkg**: [http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg](http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg) (based on [Cython-0.19 beta2](http://mail.python.org/pipermail/cython-devel/2013-April/003564.html))
+
+**upstream** bug report causing segfaults in libGAP: [http://trac.cython.org/cython_trac/ticket/808](http://trac.cython.org/cython_trac/ticket/808)
robertwb commented

Changed upstream from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.

jdemeyer commented
comment:5

Attachment: sage_crash_W7eVMu.log

jdemeyer commented

Description changed:

--- 
+++ 
@@ -3,3 +3,5 @@
 **spkg**: [http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg](http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg) (based on [Cython-0.19 beta2](http://mail.python.org/pipermail/cython-devel/2013-April/003564.html))
 
 **upstream** bug report causing segfaults in libGAP: [http://trac.cython.org/cython_trac/ticket/808](http://trac.cython.org/cython_trac/ticket/808)
+
+With this spkg, all doctests pass, but the docbuilder crashes: [attachment: sage_crash_W7eVMu.log](https://github.com/sagemath/sage-prod/files/10657603/sage_crash_W7eVMu.log)
jdemeyer commented

Description changed:

--- 
+++ 
@@ -1,6 +1,6 @@
 Upgrade to Cython 0.19 when it's released.
 
-**spkg**: [http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg](http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg) (based on [Cython-0.19 beta2](http://mail.python.org/pipermail/cython-devel/2013-April/003564.html))
+**spkg**: [http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg](http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg) (based on Cython master)
 
 **upstream** bug report causing segfaults in libGAP: [http://trac.cython.org/cython_trac/ticket/808](http://trac.cython.org/cython_trac/ticket/808)
jdemeyer commented
comment:7

Here is what I found about the docbuilder crash: it works with Cython 0.18, not with current master at 0ee15b1829125b6844ac13f437c2d9bd61a4ceca.

In lazy_import.pyx, somehow self._object becomes NULL in the line

setattr(cls, alias, self._object)

which is compiled to

           __pyx_t_1 = __pyx_v_self->_object;                                      
           __Pyx_INCREF(__pyx_t_1);
           __pyx_t_10 = PyObject_SetAttr(__pyx_v_cls, __pyx_v_alias, __pyx_t_1); if (unlikely(__pyx_t_10 == -1)) {__pyx_filename = __pyx_f[0]; __pyx_lineno = 169; __py
           __Pyx_DECREF(__pyx_t_1); __pyx_t_1 = 0;

I don't see anything suspicious here.

jdemeyer commented
comment:8

Generating just lazy_import.c with Cython-0.18 makes the documentation build.

jdemeyer commented
comment:9

Robert, I feel like I can not do much more to debug the docbuilding since I don't really understand the mechanics of lazy_import.

jdemeyer commented

Description changed:

--- 
+++ 
@@ -4,4 +4,4 @@
 
 **upstream** bug report causing segfaults in libGAP: [http://trac.cython.org/cython_trac/ticket/808](http://trac.cython.org/cython_trac/ticket/808)
 
-With this spkg, all doctests pass, but the docbuilder crashes: [attachment: sage_crash_W7eVMu.log](https://github.com/sagemath/sage-prod/files/10657603/sage_crash_W7eVMu.log)
+With this spkg, all doctests pass, but the docbuilder (in particular the command `./sage --docbuild reference/combinat html`) crashes due to a problem with `lazy_import`: [attachment: sage_crash_W7eVMu.log](https://github.com/sagemath/sage-prod/files/10657603/sage_crash_W7eVMu.log)
jdemeyer commented

Description changed:

--- 
+++ 
@@ -2,6 +2,6 @@
 
 **spkg**: [http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg](http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg) (based on Cython master)
 
-**upstream** bug report causing segfaults in libGAP: [http://trac.cython.org/cython_trac/ticket/808](http://trac.cython.org/cython_trac/ticket/808)
+**upstream** bug report about segfaults in libGAP: [http://trac.cython.org/cython_trac/ticket/808](http://trac.cython.org/cython_trac/ticket/808)
 
 With this spkg, all doctests pass, but the docbuilder (in particular the command `./sage --docbuild reference/combinat html`) crashes due to a problem with `lazy_import`: [attachment: sage_crash_W7eVMu.log](https://github.com/sagemath/sage-prod/files/10657603/sage_crash_W7eVMu.log)
jdemeyer commented

Description changed:

--- 
+++ 
@@ -4,4 +4,4 @@
 
 **upstream** bug report about segfaults in libGAP: [http://trac.cython.org/cython_trac/ticket/808](http://trac.cython.org/cython_trac/ticket/808)
 
-With this spkg, all doctests pass, but the docbuilder (in particular the command `./sage --docbuild reference/combinat html`) crashes due to a problem with `lazy_import`: [attachment: sage_crash_W7eVMu.log](https://github.com/sagemath/sage-prod/files/10657603/sage_crash_W7eVMu.log)
+With this spkg, all doctests pass, but the docbuilder (in particular the command `./sage --docbuild reference/combinat html`) crashes due to a problem with `lazy_import`: [attachment: sage_crash_W7eVMu.log](https://github.com/sagemath/sage-prod/files/10657603/sage_crash_W7eVMu.log) If the file `lazy_import.c` is generated with Cython 0.18, the problem goes away.
robertwb commented
comment:13

I don't see anything suspicious there either. Can you try compiling lazy_import.pyx with -O0? What's the diff of this file between 0.18 and 0.19?

jdemeyer commented
comment:14

Replying to @robertwb:

Can you try compiling lazy_import.pyx with -O0?

I did, and it didn't help. I could try to bisect the Cython git repo for the bug.

jdemeyer commented
comment:15

Results of bisecting:

  • commit 19c0408476515c89a60655b753f08a3bd18eec83 is good

  • commit d44a0845fc91bf7ade61dee431358f375c580f44 is bad

All commits in between simply fail to compile.

jdemeyer commented
comment:16

The relevant change seems to be the line

obj = self._get_object(owner=owner)

which was compiled in the good way as

  __pyx_t_1 = PyObject_GetAttr(((PyObject *)__pyx_v_self), __pyx_n_s___get_object); if (unlikely(!__pyx_t_1)) {__pyx_filename = __pyx_f[0]; __pyx_lineno = 401; __pyx_clineno = __LINE__; goto __pyx_L1_error;}
  __Pyx_GOTREF(__pyx_t_1);
  __pyx_t_2 = PyDict_New(); if (unlikely(!__pyx_t_2)) {__pyx_filename = __pyx_f[0]; __pyx_lineno = 401; __pyx_clineno = __LINE__; goto __pyx_L1_error;}
  __Pyx_GOTREF(((PyObject *)__pyx_t_2));
  if (PyDict_SetItem(__pyx_t_2, ((PyObject *)__pyx_n_s__owner), __pyx_v_owner) < 0) {__pyx_filename = __pyx_f[0]; __pyx_lineno = 401; __pyx_clineno = __LINE__; goto __pyx_L1_error;}
  __pyx_t_3 = PyObject_Call(__pyx_t_1, ((PyObject *)__pyx_empty_tuple), ((PyObject *)__pyx_t_2)); if (unlikely(!__pyx_t_3)) {__pyx_filename = __pyx_f[0]; __pyx_lineno = 401; __pyx_clineno = __LINE__; goto __pyx_L1_error;}
  __Pyx_GOTREF(__pyx_t_3);
  __Pyx_DECREF(__pyx_t_1); __pyx_t_1 = 0;
  __Pyx_DECREF(((PyObject *)__pyx_t_2)); __pyx_t_2 = 0;
  __pyx_v_obj = __pyx_t_3;
  __pyx_t_3 = 0;

and in the bad way as

  __pyx_t_2.__pyx_n = 1;
  __pyx_t_2.owner = __pyx_v_owner;
  __pyx_t_1 = ((struct __pyx_vtabstruct_4sage_4misc_11lazy_import_LazyImport *)__pyx_v_self->__pyx_vtab)->_get_object(__pyx_v_self, 0, &__pyx_t_2); if (unlikely(!__pyx_t_1)) {__pyx_filename = __pyx_f[0]; __pyx_lineno = 401; __pyx_clineno = __LINE__; goto __pyx_L1_error;}
  __Pyx_GOTREF(__pyx_t_1);
  __pyx_v_obj = __pyx_t_1;
  __pyx_t_1 = 0;
jdemeyer commented
comment:17

Alternatively, changing the line

cpdef _get_object(self, owner=None):

to

def _get_object(self, owner=None):

also fixes the problem. I am applying this workaround in attachment: 14452_cython_0.19.patch.

jdemeyer commented

Description changed:

--- 
+++ 
@@ -1,7 +1,9 @@
-Upgrade to Cython 0.19 when it's released.
+Upgrade to Cython 0.19.
 
-**spkg**: [http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg](http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg) (based on Cython master)
+**spkg**: [http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg](http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg)
 
 **upstream** bug report about segfaults in libGAP: [http://trac.cython.org/cython_trac/ticket/808](http://trac.cython.org/cython_trac/ticket/808)
 
-With this spkg, all doctests pass, but the docbuilder (in particular the command `./sage --docbuild reference/combinat html`) crashes due to a problem with `lazy_import`: [attachment: sage_crash_W7eVMu.log](https://github.com/sagemath/sage-prod/files/10657603/sage_crash_W7eVMu.log) If the file `lazy_import.c` is generated with Cython 0.18, the problem goes away.
+With this spkg, all doctests pass, but the docbuilder (in particular the command `./sage --docbuild reference/combinat html`) crashes due to a problem with `lazy_import`: [attachment: sage_crash_W7eVMu.log](https://github.com/sagemath/sage-prod/files/10657603/sage_crash_W7eVMu.log) The Sage library patch works around this problem.
+
+**apply** [attachment: 14452_cython_0.19.patch](https://github.com/sagemath/sage-prod/files/10657605/14452_cython_0.19.patch.gz)
jdemeyer commented

Description changed:

--- 
+++ 
@@ -1,8 +1,11 @@
 Upgrade to Cython 0.19.
 
-**spkg**: [http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg](http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.spkg)
+**spkg**: [http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.p0.spkg](http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.p0.spkg)
 
-**upstream** bug report about segfaults in libGAP: [http://trac.cython.org/cython_trac/ticket/808](http://trac.cython.org/cython_trac/ticket/808)
+**upstream**:
+1. Bug report about segfaults in libGAP: [http://trac.cython.org/cython_trac/ticket/808](http://trac.cython.org/cython_trac/ticket/808) (fixed in Cython 0.19)
+2. Bug with g++ on Solaris: [http://trac.cython.org/cython_trac/ticket/809](http://trac.cython.org/cython_trac/ticket/809) (patch in spkg and submitted upstream)
+3. Bug with g++ on Solaris: [http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57025](http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57025) (deny it's a bug)
 
 With this spkg, all doctests pass, but the docbuilder (in particular the command `./sage --docbuild reference/combinat html`) crashes due to a problem with `lazy_import`: [attachment: sage_crash_W7eVMu.log](https://github.com/sagemath/sage-prod/files/10657603/sage_crash_W7eVMu.log) The Sage library patch works around this problem.
jdemeyer commented

Changed upstream from Fixed upstream, but not in a stable release. to None of the above - read trac for reasoning.

jdemeyer commented

spkg diff

jdemeyer commented
comment:21

Attachment: cython-0.19.p0.diff.gz

jdemeyer commented

Description changed:

--- 
+++ 
@@ -1,6 +1,6 @@
 Upgrade to Cython 0.19.
 
-**spkg**: [http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.p0.spkg](http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.p0.spkg)
+**spkg**: [http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.p0.spkg](http://boxen.math.washington.edu/home/jdemeyer/spkg/cython-0.19.p0.spkg) ([spkg diff](https://github.com/sagemath/sage-prod/files/10657604/cython-0.19.p0.diff.gz))
 
 **upstream**:
 1. Bug report about segfaults in libGAP: [http://trac.cython.org/cython_trac/ticket/808](http://trac.cython.org/cython_trac/ticket/808) (fixed in Cython 0.19)
jdemeyer commented

Description changed:

--- 
+++ 
@@ -9,4 +9,22 @@
 
 With this spkg, all doctests pass, but the docbuilder (in particular the command `./sage --docbuild reference/combinat html`) crashes due to a problem with `lazy_import`: [attachment: sage_crash_W7eVMu.log](https://github.com/sagemath/sage-prod/files/10657603/sage_crash_W7eVMu.log) The Sage library patch works around this problem.
 
+On Itanium, there is a doctest failure
+
+```
+sage -t --long devel/sage/sage/structure/element.pyx
+**********************************************************************
+File "devel/sage/sage/structure/element.pyx", line 1733, in sage.structure.element.RingElement.__pow__
+Failed example:
+    p(a,2,1)
+Expected:
+    Traceback (most recent call last):
+    ...
+    RuntimeError: __pow__ dummy argument not used
+Got:
+    202
+**********************************************************************
+```
+This is likely a bug in GCC-4.7.3, but no further investigation has been done. Compiling `sage/structure/element.pyx` with `-Os` works around the problem, see patch.
+
 **apply** [attachment: 14452_cython_0.19.patch](https://github.com/sagemath/sage-prod/files/10657605/14452_cython_0.19.patch.gz)
jdemeyer commented
comment:23

With these patches, all fine on the buildbot.

jdemeyer commented

Dependencies: #13826

jdemeyer commented
comment:24

Rebased to #13826.

robertwb commented
comment:26

Thanks for debugging this! I filed http://trac.cython.org/cython_trac/ticket/810 about (what I think is) the underlying bug you uncovered with the lazy import stuff.

jdemeyer commented

Merged: sage-5.9.rc0

jdemeyer commented

Reviewer: Robert Bradshaw

jdemeyer commented
comment:28

Replying to @robertwb:

Thanks for debugging this! I filed http://trac.cython.org/cython_trac/ticket/810 about (what I think is) the underlying bug you uncovered with the lazy import stuff.

Do you have a better idea about what precisely is wrong? I isolated the problem, but I don't know why it segfaults.

jdemeyer commented

Description changed:

--- 
+++ 
@@ -6,6 +6,7 @@
 1. Bug report about segfaults in libGAP: [http://trac.cython.org/cython_trac/ticket/808](http://trac.cython.org/cython_trac/ticket/808) (fixed in Cython 0.19)
 2. Bug with g++ on Solaris: [http://trac.cython.org/cython_trac/ticket/809](http://trac.cython.org/cython_trac/ticket/809) (patch in spkg and submitted upstream)
 3. Bug with g++ on Solaris: [http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57025](http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57025) (deny it's a bug)
+4. Bug with `cpdef` keyword arguments in `lazy_import.pyx`: [http://trac.cython.org/cython_trac/ticket/810](http://trac.cython.org/cython_trac/ticket/810)
 
 With this spkg, all doctests pass, but the docbuilder (in particular the command `./sage --docbuild reference/combinat html`) crashes due to a problem with `lazy_import`: [attachment: sage_crash_W7eVMu.log](https://github.com/sagemath/sage-prod/files/10657603/sage_crash_W7eVMu.log) The Sage library patch works around this problem.