search

sagemath / cysignals

cysignals: interrupt and signal handling for Cython. Source repository for https://pypi.org/project/cysignals/
GNU Lesser General Public License v3.0
44 stars 23 forks source link

Fix #126 #127

Closed embray closed 3 years ago

embray commented 3 years ago

Includes a regression test. Without this fix, the test causes the doctest runner itself to segfault. Not sure if that's ideal, but it does at least demonstrate the bug, and that it's fixed by this change.

embray commented 3 years ago

Test is failing on Python 2.7, possibly because its design depends in part on dict ordering (and the order in which dict elements are deallocated after breaking a reference cycle). That's just my guess. On older Python 3 versions they are failing for some other reason I think unrelated to this PR.

malb commented 3 years ago

This looks good to me. Shall we just remove the Python 2 tests, it's EOL?

embray commented 3 years ago

@malb See #132 and also #133 where I'm trying to fix the CI for older Python versions at least for now.

embray commented 3 years ago

Re-running tests now that #133 is merged.

dimpase commented 3 years ago

Wow! Can we have a release, for testing purposes?

embray commented 3 years ago

@dimpase Yeah, I'll make a pre-release soon and put a ticket on Sage to test it.

kliem commented 3 years ago

There seems to be some problem with this. I'm testing this on https://trac.sagemath.org/ticket/31455. I'm getting a segementation fault with

sage -t --long --random-seed=0 src/sage/matrix/matrix_mod2_dense.pyx

with the test sage: MatrixSpace(GF(2), 2^30)(1) # optional - memlimit ## line 171 ##

I attached traceback cysignals_matrix_mod2.log

I don't think this is related to anything I did, because the file sage/local/lib/python3.7/site-packages/cysignals/cysignals_config.h is the only thing that I efficiently changed and in my case it is equivalent as -fopenmp does not make a difference.

collares commented 3 years ago

@kliem Quoting #131 comment 0:

[1.10.3 will not contain] the assembly longjmp implementation, which should at a minimum necessitate a new minor release since it's a big addition (and currently does not work for me in Sage, but that's another story).

The longjmp changes were committed back in 2019, but were committed after 1.10.2 and won't be included in 1.10.3. Could they be causing your issue?

kliem commented 3 years ago

I don't know what's causing the issue. I figured it was this, because there are only two closed pull requests, but I didn't see that before then there commits without pull requests.

Edit: Should I just open an issue for this?

collares commented 3 years ago

@embray created a tag for 1.10.3b0 at https://github.com/sagemath/cysignals/releases/tag/1.10.3b0, and it does not include the longjmp changes. Do you see the same problem using this version? See also https://trac.sagemath.org/ticket/31474

embray commented 3 years ago

@kliem Try testing cysignals with the 1.10.x branch which does not include the longjmp changes (which I've observed still cause a lot of problems in Sage)

slel commented 3 years ago

The "releases" page https://github.com/sagemath/cysignals/releases does list 1.10.3, but somehow still shows 1.10.2 as the "Latest release".

dimpase commented 3 years ago

indeed, that's cause the release title was not filled it. Fixed now.