Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
e0b0c06
Fix segmentation fault in libgap function call
user202729 Aug 15, 2025
4d1f19f
Code modification to avoid calling GapElement.__dealloc__ too early
user202729 Aug 15, 2025
c6b76b6
src/sage/libs/gap/gap_includes.pxd: add InterruptExecStat
orlitzky Aug 17, 2025
f9c186d
src/sage/libs/gap/util.pyx: add gap_sig_on / gap_sig_off
orlitzky Aug 17, 2025
a9473ca
src/sage/libs/gap/util.pyx: replace sig_on / sig_off
orlitzky Aug 17, 2025
fc16eae
src/sage/libs/gap/element.pyx: replace sig_on / sig_off
orlitzky Aug 17, 2025
a887924
src/sage/libs/gap/meson.build: remove cysignals dependency
orlitzky Aug 17, 2025
a219ac9
src/sage/libs/gap/libgap.pyx: update signal handling docs
orlitzky Aug 17, 2025
0b66eba
src/sage/doctest/util.py: handle Ctrl-C from GAP
orlitzky Aug 17, 2025
e1e8d62
src/sage/libs/gap/gap_includes.pxd: drop sig_GAP_Enter()
orlitzky Aug 17, 2025
2ca060f
src/sage/libs/gap/libgap.pyx: rewrite signal handling explanation
orlitzky Aug 17, 2025
7c48f94
src/sage/libs/gap/util.pyx: do KeyboardInterrupt conversion earlier
orlitzky Aug 18, 2025
fb458ad
src/sage/doctest/util.py: update GAP workaround
orlitzky Aug 18, 2025
51c81b0
src/sage/libs/gap: remove redundant KeyboardInterrupt hacks
orlitzky Aug 18, 2025
bab97f6
src/sage/libs/gap/util.pyx: type-safety for InterruptExecStat()
orlitzky Aug 18, 2025
b298ca8
src/sage/doctest/util.py: further simplify GAP workaround
orlitzky Aug 18, 2025
867cf66
src/sage/libs/gap/libgap.pyx: explain error handling
orlitzky Aug 18, 2025
cd93bdf
src/sage/libs/gap/util.pyx: delete old commented debug statement
orlitzky Aug 19, 2025
794b427
src/sage/libs/gap: use public API for is_string()
orlitzky Aug 19, 2025
ee6af07
src/sage/libs/gap/element.pyx: missing GAP_Enter / GAP_Leave
orlitzky Aug 19, 2025
e7bb222
src/sage/libs/gap/element.pyx: drop superclass list -> sage conversion
orlitzky Aug 19, 2025
d9bd3c2
src/sage/libs/gap: be more careful inside make_any_gap_element()
orlitzky Aug 19, 2025
e79bdd7
src/sage/libs/gap/element.pyx: revert list-of-char changes
orlitzky Aug 20, 2025
3220d8f
src/sage/libs/gap/libgap.pyx: more realistic "real" example
orlitzky Aug 20, 2025
24065b5
src/sage/libs/gap/libgap.pyx: clarify "GAP library usage"
orlitzky Aug 20, 2025
041fcdf
src/sage/libs/gap/libgap.pyx: fix module docstring indentation
orlitzky Aug 20, 2025
50157d7
src/sage/libs/gap/libgap.pyx: more module docstring tweaks
orlitzky Aug 21, 2025
b550b36
src/sage/libs/gap/util.pyx: add missing noexcept annotation
orlitzky Aug 21, 2025
677c2f1
src/sage/libs/gap/element.pyx: more tolerance in Ctrl-C tests
orlitzky Aug 21, 2025
1af02c2
src/sage/libs/gap: special case for bool in make_any_gap_element()
orlitzky Aug 21, 2025
f39be09
src/sage/libs/gap/element.pyx: update make_any_gap_element() comment
orlitzky Aug 21, 2025
36da2ca
src/sage/libs/gap/element.pyx: faster list length check
orlitzky Aug 21, 2025
3a6d006
src/sage/libs/gap/libgap.pyx: slightly safer example
orlitzky Aug 21, 2025
8b8c496
src/sage/rings/complex_arb.pyx: add some Ctrl-C tolerance to a doctest
orlitzky Aug 21, 2025
6e74192
src/sage/libs/gap/libgap.pyx: unindent the "Using the .. library" docs
orlitzky Aug 23, 2025
47826c2
src/sage/libs/gap/libgap.pyx: unindent the AUTHORS block
orlitzky Aug 23, 2025
c1adeb9
src/sage/libs/gap/libgap.pyx: backticks for ``libgap-api.h``
orlitzky Aug 23, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/sage/doctest/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -886,6 +886,19 @@ def ensure_interruptible_after(seconds: float, max_wait_after_interrupt: float =
except AlarmInterrupt as e:
e.__traceback__ = None # workaround for https://github.com/python/cpython/pull/129276
alarm_raised = True
except KeyboardInterrupt as e:
from sage.libs.gap.util import GAPError
if isinstance(e.__cause__, GAPError):
# To handle SIGALRM in GAP we install its SIGINT handler
# as the SIGALRM handler. When it gets Ctrl-C, we turn the
# resulting GAPError into a KeyboardInterrupt... but
# there's no real way to distinguish the SIGALRM case
# (doctesting) from the SIGINT case (interactive Ctrl-C)
# except for context.
e.__traceback__ = None
alarm_raised = True
else:
raise
Copy link
Contributor

@user202729 user202729 Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the logic below (raise AlarmInterrupt instead of KeyboardInterrupt) are correctly implemented, this change wouldn't be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've further simplified this down to a one-line change, using the fact that AlarmInterrupt is a subclass of KeyboardInterrupt. Instead of catching AlarmInterrupt, we can catch KeyboardInterrupt, and then check,

  • is it an AlarmInterrupt?
  • does it have "user interrupt" as the message?

I prefer this to hacking AlarmInterrupt into the GAP code at this point because sage.libs.gap is otherwise free of cysignals. To raise the AlarmInterrupt from sage.libs.gap we'd have to,

  1. Add cysignals back as a dep in meson.build
  2. Import AlarmInterrupt
  3. Add the global last_signal variable
  4. Set last_signal whenever a handler is called
  5. Check last_signal when raising a GAPError and convert it to the right thing

Since this is all for the benefit of one doctest method, it just seems easier to add the one line in that doctest method?

finally:
before_cancel_alarm_elapsed = walltime() - start_time
cancel_alarm()
Expand Down
152 changes: 107 additions & 45 deletions src/sage/libs/gap/element.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,11 @@
# ****************************************************************************

from cpython.object cimport Py_EQ, Py_NE, Py_LE, Py_GE, Py_LT, Py_GT
from cysignals.signals cimport sig_on, sig_off

from sage.libs.gap.gap_includes cimport *
from sage.libs.gap.libgap import libgap
from sage.libs.gap.util cimport *
from sage.libs.gap.util import GAPError
from sage.libs.gap.util import GAPError, gap_sig_on, gap_sig_off
from sage.libs.gmp.mpz cimport *
from sage.libs.gmp.pylong cimport mpz_get_pylong
from sage.cpython.string cimport str_to_bytes, char_to_str
Expand Down Expand Up @@ -935,13 +934,21 @@
if self._compare_by_id:
return id(self) == id(other)
cdef GapElement c_other = <GapElement>other
sig_on()

try:
gap_sig_on()
GAP_Enter()
return GAP_EQ(self.value, c_other.value)
except GAPError as e:
if "user interrupt" in str(e):
# Ctrl-C
raise KeyboardInterrupt from e
else:
raise
finally:
GAP_Leave()
sig_off()
gap_sig_off()


cdef bint _compare_less(self, Element other) except -2:
"""
Expand All @@ -957,13 +964,21 @@
if self._compare_by_id:
return id(self) < id(other)
cdef GapElement c_other = <GapElement>other
sig_on()

try:
gap_sig_on()
GAP_Enter()
return GAP_LT(self.value, c_other.value)
except GAPError as e:
if "user interrupt" in str(e):
# Ctrl-C
raise KeyboardInterrupt from e
else:
raise
finally:
GAP_Leave()
sig_off()
gap_sig_off()


cpdef _add_(self, right):
r"""
Expand All @@ -986,12 +1001,18 @@
"""
cdef Obj result
try:
sig_GAP_Enter()
sig_on()
gap_sig_on()
GAP_Enter()
result = GAP_SUM(self.value, (<GapElement>right).value)
sig_off()
except GAPError as e:
if "user interrupt" in str(e):
# Ctrl-C
raise KeyboardInterrupt from e
else:
raise
finally:
GAP_Leave()
gap_sig_off()
return make_any_gap_element(self.parent(), result)

cpdef _sub_(self, right):
Expand All @@ -1014,12 +1035,18 @@
"""
cdef Obj result
try:
sig_GAP_Enter()
sig_on()
gap_sig_on()
GAP_Enter()
result = GAP_DIFF(self.value, (<GapElement>right).value)
sig_off()
except GAPError as e:
if "user interrupt" in str(e):
# Ctrl-C
raise KeyboardInterrupt from e
else:
raise
finally:
GAP_Leave()
gap_sig_off()
return make_any_gap_element(self.parent(), result)

cpdef _mul_(self, right):
Expand All @@ -1043,12 +1070,18 @@
"""
cdef Obj result
try:
sig_GAP_Enter()
sig_on()
gap_sig_on()
GAP_Enter()
result = GAP_PROD(self.value, (<GapElement>right).value)
sig_off()
except GAPError as e:
if "user interrupt" in str(e):
# Ctrl-C
raise KeyboardInterrupt from e
else:
raise
finally:
GAP_Leave()
gap_sig_off()
return make_any_gap_element(self.parent(), result)

cpdef _div_(self, right):
Expand Down Expand Up @@ -1077,12 +1110,18 @@
"""
cdef Obj result
try:
sig_GAP_Enter()
sig_on()
gap_sig_on()
GAP_Enter()
result = GAP_QUO(self.value, (<GapElement>right).value)
sig_off()
except GAPError as e:
if "user interrupt" in str(e):
# Ctrl-C
raise KeyboardInterrupt from e
else:
raise
finally:
GAP_Leave()
gap_sig_off()
return make_any_gap_element(self.parent(), result)

cpdef _mod_(self, right):
Expand All @@ -1104,12 +1143,18 @@
"""
cdef Obj result
try:
sig_GAP_Enter()
sig_on()
gap_sig_on()
GAP_Enter()
result = GAP_MOD(self.value, (<GapElement>right).value)
sig_off()
except GAPError as e:
if "user interrupt" in str(e):
# Ctrl-C
raise KeyboardInterrupt from e
else:
raise
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is some really bad duplication of code. I suggest

  • fold GAP_Enter and GAP_Leave into gap_sig_on and gap_sig_off respectively
  • instead of setting the interrupt handler directly to InterruptExecStat, make custom wrapper function (or hook into cysignals), the custom wrapper function sets a global variable that determine which kind of signal is being raised, then call InterruptExecStat
  • the global variable above need to be reset at appropriate places
  • modify error_handler to, instead of indiscriminately raise GAPError, raise the correct exception by reading the global variable above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's pretty bad but I was trying to keep it as simple as possible for the first iteration. First get it to not crash, then make it pretty.

I don't think we can use a global for the signal type because signals are asynchronous. There's no reason why a SIGINT can't be triggered while handling a SIGALRM, or vice-versa. It's not very likely, but if there's a lesson here it's that we shouldn't count on unlikely things not happening.

We may be able to combine GAP_Enter and gap_sig_on(), though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can use a global for the signal type because signals are asynchronous. There's no reason why a SIGINT can't be triggered while handling a SIGALRM, or vice-versa. It's not very likely, but if there's a lesson here it's that we shouldn't count on unlikely things not happening.

you're right but… I think currently the signal handler is calling PyErr_Restore() or something anyway, and that one isn't reentrant either.

Searching up a bit https://stackoverflow.com/a/3127697/, that's indeed a possibility. But doesn't the same thing happen for cysignals as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may be able to combine GAP_Enter and gap_sig_on(), though.

Even this immediately leads to a segault.

finally:
GAP_Leave()
gap_sig_off()
return make_any_gap_element(self.parent(), result)

cpdef _pow_(self, other):
Expand All @@ -1136,7 +1181,7 @@

sage: a, b = libgap.GL(1000, 3).GeneratorsOfGroup(); g = a * b
sage: from sage.doctest.util import ensure_interruptible_after
sage: with ensure_interruptible_after(0.5): g ^ (2 ^ 10000)

Check failure on line 1184 in src/sage/libs/gap/element.pyx

View workflow job for this annotation

GitHub Actions / test-long (src/sage/[g-o]*)

Failed example:

Failed example:: Exception raised: Traceback (most recent call last): File "sage/libs/gap/element.pyx", line 1200, in sage.libs.gap.element.GapElement._pow_ sage.libs.gap.util.GAPError: Error, user interrupt The above exception was the direct cause of the following exception: KeyboardInterrupt During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/sage/src/sage/doctest/forker.py", line 733, in _run self.compile_and_execute(example, compiler, test.globs) File "/sage/src/sage/doctest/forker.py", line 1157, in compile_and_execute exec(compiled, globs) File "<doctest sage.libs.gap.element.GapElement._pow_[8]>", line 1, in <module> with ensure_interruptible_after(RealNumber('0.5')): g ** (Integer(2) ** Integer(10000)) File "/usr/lib/python3.12/contextlib.py", line 158, in __exit__ self.gen.throw(value) File "/sage/src/sage/doctest/util.py", line 910, in ensure_interruptible_after raise RuntimeError( RuntimeError: Function is not interruptible within 0.5000 seconds, only after 0.8417 seconds

sage: libgap.CyclicGroup(2) ^ 2
Traceback (most recent call last):
Expand All @@ -1151,12 +1196,18 @@
method found for `InverseMutable' on 1 arguments
"""
try:
sig_GAP_Enter()
sig_on()
gap_sig_on()
GAP_Enter() # GAPError raised from here
result = GAP_POW(self.value, (<GapElement>other).value)
sig_off()
except GAPError as e:
if "user interrupt" in str(e):
# Ctrl-C
raise KeyboardInterrupt from e
else:
raise
finally:
GAP_Leave()
gap_sig_off()
return make_any_gap_element(self._parent, result)

cpdef _pow_int(self, other):
Expand Down Expand Up @@ -2498,36 +2549,40 @@
cdef Obj result = NULL
cdef Obj arg_list
cdef int n = len(args)
cdef volatile Obj v2

if n > 0 and n <= 3:
libgap = self.parent()
a = [x if isinstance(x, GapElement) else libgap(x) for x in args]
cdef Obj a[3]

if n <= 3:
if not all(isinstance(x, GapElement) for x in args):
libgap = self.parent()
args = tuple(x if isinstance(x, GapElement) else libgap(x) for x in args)
for i in range(n):
x = args[i]
a[i] = (<GapElement>x).value
else:
arg_list = make_gap_list(args)

try:
sig_GAP_Enter()
sig_on()
gap_sig_on()
GAP_Enter()
if n == 0:
result = GAP_CallFunc0Args(self.value)
elif n == 1:
result = GAP_CallFunc1Args(self.value,
(<GapElement>a[0]).value)
result = GAP_CallFunc1Args(self.value, a[0])
elif n == 2:
result = GAP_CallFunc2Args(self.value,
(<GapElement>a[0]).value,
(<GapElement>a[1]).value)
result = GAP_CallFunc2Args(self.value, a[0], a[1])
elif n == 3:
v2 = (<GapElement>a[2]).value
result = GAP_CallFunc3Args(self.value,
(<GapElement>a[0]).value,
(<GapElement>a[1]).value,
v2)
result = GAP_CallFunc3Args(self.value, a[0], a[1], a[2])
else:
arg_list = make_gap_list(args)
result = GAP_CallFuncList(self.value, arg_list)
sig_off()
except GAPError as e:
if "user interrupt" in str(e):
# Ctrl-C
raise KeyboardInterrupt from e
else:
raise
finally:
GAP_Leave()
gap_sig_off()
if result == NULL:
# We called a procedure that does not return anything
return None
Expand Down Expand Up @@ -3151,13 +3206,20 @@
"""
cdef UInt i = self.record_name_to_index(name)
cdef Obj result
sig_on()

try:
gap_sig_on()
GAP_Enter()
result = ELM_REC(self.value, i)
except GAPError as e:
if "user interrupt" in str(e):
# Ctrl-C
raise KeyboardInterrupt from e
else:
raise
finally:
GAP_Leave()
sig_off()
gap_sig_off()
return make_any_gap_element(self.parent(), result)

def sage(self):
Expand Down
8 changes: 4 additions & 4 deletions src/sage/libs/gap/gap_includes.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@ cdef extern from "gap/calls.h" nogil:


cdef extern from "gap/libgap-api.h" nogil:
"""
#define sig_GAP_Enter() {int t = GAP_Enter(); if (!t) sig_error();}
"""
ctypedef void (*GAP_CallbackFunc)()
void GAP_Initialize(int argc, char ** argv,
GAP_CallbackFunc markBagsCallback, GAP_CallbackFunc errorCallback,
Expand All @@ -40,7 +37,6 @@ cdef extern from "gap/libgap-api.h" nogil:
cdef void GAP_EnterStack()
cdef void GAP_LeaveStack()
cdef int GAP_Enter() except 0
cdef void sig_GAP_Enter()
cdef void GAP_Leave()
cdef int GAP_Error_Setjmp() except 0

Expand Down Expand Up @@ -148,6 +144,10 @@ cdef extern from "gap/stringobj.h" nogil:
Obj NEW_STRING(Int)


cdef extern from "gap/stats.h" nogil:
void InterruptExecStat()


cdef extern from "<structmember.h>" nogil:
"""
/* Hack: Cython 3.0 automatically includes <structmember.h>, which
Expand Down
36 changes: 34 additions & 2 deletions src/sage/libs/gap/libgap.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,43 @@ using the recursive expansion of the
Using the GAP C library from Cython
===================================

.. TODO:: Expand the following text

We are using the GAP API provided by the GAP project since
GAP 4.10.

One unusual aspect of this interface is its signal handling.
Typically, cysignals' ``sig_on()`` and ``sig_off()`` functions are
used to wrap code that may take a long time, and, as a result, may
need to be interrupted with Ctrl-C. Those functions are implemented
with setjmp and longjmp however, and cause hard-to-diagnose issues
arising from their interaction with ``GAP_enter()`` and
``GAP_Leave()`` -- also implemented via setjmp and longjmp. This
has lead to many segfaults when GAP errors are raised, or when
Ctrl-C is used to interrupt GAP computations.

The approach we are using instead is to install GAP's own
``SIGINT`` handler (to catch Ctrl-C) before executing any
long-running GAP code, and then to later uninstall it when the GAP
code has finished. This is accomplished using the
suggestively-named ``gap_sig_on()`` and ``gap_sig_off()``
functions. After you have called ``gap_sig_on()``, if GAP receives
Ctrl-C, it will invoke our custom ``error_handler()`` that will
raise a :exc:`sage.libs.gap.util.GAPError` containing the phrase
"user interrupt". As a result you will often find GAP code
sandwiched between ``gap_sig_on()`` and ``gap_sig_off()``, in a
``try/except`` block, with special handling for these GAP
errors. The goal is to catch and re-raise them as
:exc:`KeyboardInterrupt` so that they can be caught in user code
just like you would with a cysignals interrupt.

Before you attempt to change any of this, please make sure that you
understand the issues that it is intended to fix, e.g.

* https://github.com/sagemath/sage/issues/37026
* https://trofi.github.io/posts/312-the-sagemath-saga.html
* https://github.com/sagemath/sage/pull/40585
* https://github.com/sagemath/sage/pull/40594
* https://github.com/sagemath/sage/issues/40598

AUTHORS:

- William Stein, Robert Miller (2009-06-23): first version
Expand Down
2 changes: 1 addition & 1 deletion src/sage/libs/gap/meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ foreach name, pyx : extension_data
subdir: 'sage/libs/gap',
install: true,
include_directories: [inc_cpython, inc_rings],
dependencies: [py_dep, cysignals, gap, gmp],
dependencies: [py_dep, gap, gmp],
)
endforeach

Loading
Loading