From e0b0c060c158b3dd53eef1382f9e6c469d81db77 Mon Sep 17 00:00:00 2001 From: user202729 <25191436+user202729@users.noreply.github.com> Date: Sat, 16 Aug 2025 02:28:36 +0700 Subject: [PATCH 01/37] Fix segmentation fault in libgap function call --- src/sage/libs/gap/element.pyx | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index f52a73c2ded..9c7546ac492 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -2498,11 +2498,16 @@ cdef class GapElement_Function(GapElement): 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 n: + libgap = self.parent() + for i in range(n): + x = args[i] + a[i] = ((x if isinstance(x, GapElement) else libgap(x))).value + else: + arg_list = make_gap_list(args) try: sig_GAP_Enter() @@ -2510,20 +2515,12 @@ cdef class GapElement_Function(GapElement): if n == 0: result = GAP_CallFunc0Args(self.value) elif n == 1: - result = GAP_CallFunc1Args(self.value, - (a[0]).value) + result = GAP_CallFunc1Args(self.value, a[0]) elif n == 2: - result = GAP_CallFunc2Args(self.value, - (a[0]).value, - (a[1]).value) + result = GAP_CallFunc2Args(self.value, a[0], a[1]) elif n == 3: - v2 = (a[2]).value - result = GAP_CallFunc3Args(self.value, - (a[0]).value, - (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() finally: From 4d1f19f654cb5c3d061bc96d4ff6c9d77d9cc2dd Mon Sep 17 00:00:00 2001 From: user202729 <25191436+user202729@users.noreply.github.com> Date: Sat, 16 Aug 2025 03:07:17 +0700 Subject: [PATCH 02/37] Code modification to avoid calling GapElement.__dealloc__ too early --- src/sage/libs/gap/element.pyx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index 9c7546ac492..2ef40fd0a69 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -2501,11 +2501,12 @@ cdef class GapElement_Function(GapElement): cdef Obj a[3] if n <= 3: - if n: + 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] = ((x if isinstance(x, GapElement) else libgap(x))).value + a[i] = (x).value else: arg_list = make_gap_list(args) From c6b76b6f94f001032e8cdb808f2df65f30f9a877 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Sat, 16 Aug 2025 22:03:28 -0400 Subject: [PATCH 03/37] src/sage/libs/gap/gap_includes.pxd: add InterruptExecStat We plan to install InterruptExecStat (gap/stats.h) as the SIGINT and SIGALRM handler while GAP code is running, since that seems to work better than mixing cysignals with GAP_Enter/GAP_Leave has. --- src/sage/libs/gap/gap_includes.pxd | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/sage/libs/gap/gap_includes.pxd b/src/sage/libs/gap/gap_includes.pxd index 35107d3d4a0..8aebf8e9204 100644 --- a/src/sage/libs/gap/gap_includes.pxd +++ b/src/sage/libs/gap/gap_includes.pxd @@ -148,6 +148,10 @@ cdef extern from "gap/stringobj.h" nogil: Obj NEW_STRING(Int) +cdef extern from "gap/stats.h" nogil: + void InterruptExecStat() + + cdef extern from "" nogil: """ /* Hack: Cython 3.0 automatically includes , which From f9c186d68c763a00945b594cd508f095be6194f2 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Sat, 16 Aug 2025 22:11:04 -0400 Subject: [PATCH 04/37] src/sage/libs/gap/util.pyx: add gap_sig_on / gap_sig_off Add two new pre/post-GAP functions to enable/disable GAP's own SIGINT (Ctrl-C) handler. These avoid the setjmp/longjmp issues we've encountered with the sig_on() and sig_off() from cysignals. --- src/sage/libs/gap/util.pyx | 40 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/src/sage/libs/gap/util.pyx b/src/sage/libs/gap/util.pyx index a5e3ad8602a..995ccac4175 100644 --- a/src/sage/libs/gap/util.pyx +++ b/src/sage/libs/gap/util.pyx @@ -12,8 +12,9 @@ Utility functions for GAP # http://www.gnu.org/licenses/ #***************************************************************************** -from libc.signal cimport signal, SIGCHLD, SIG_DFL +from libc.signal cimport signal, SIGALRM, SIGCHLD, SIG_DFL, SIGINT from posix.dlfcn cimport dlopen, dlclose, dlerror, RTLD_LAZY, RTLD_GLOBAL +from posix.signal cimport sigaction, sigaction_t, sigemptyset from cpython.exc cimport PyErr_Fetch, PyErr_Restore from cpython.object cimport Py_LT, Py_LE, Py_EQ, Py_NE, Py_GT, Py_GE @@ -180,6 +181,12 @@ MakeReadOnlyGlobal("ERROR_OUTPUT"); MakeImmutable(libgap_errout); """ +# "Global" signal handler info structs. The GAP one we enable/disable +# before/after GAP code. The Sage ones we use to store the existing +# handlers before we do that. +cdef sigaction_t gap_sigint_sa +cdef sigaction_t sage_sigint_sa +cdef sigaction_t sage_sigalrm_sa cdef initialize(): """ @@ -259,11 +266,23 @@ cdef initialize(): argv[argc] = NULL sig_on() - # Initialize GAP but disable their SIGINT handler + # Initialize GAP but disable its signal handlers: we only want + # them to be invoked while libgap code is executing, whereas the + # default would enable them globally. We will explicitly wrap GAP + # computations in gap_sig_on() and gap_sig_off() instead. GAP_Initialize(argc, argv, gasman_callback, error_handler, handleSignals=False) sig_off() + # Configure a SIGINT handler (which can be enabled by calling + # gap_sig_on) to run InterruptExecStat. This is essentially GAP's + # own SIGINT handler (but without the double-Ctrl-C behavior), and + # is less crashy than when we mix cysignals with GAP code. + global gap_sigint_sa + gap_sigint_sa.sa_handler = InterruptExecStat + sigemptyset(&(gap_sigint_sa.sa_mask)) + gap_sigint_sa.sa_flags = 0; + # Disable GAP's SIGCHLD handler ChildStatusChanged(), which calls # waitpid() on random child processes. signal(SIGCHLD, SIG_DFL) @@ -283,6 +302,23 @@ cdef initialize(): f.close() gap_eval('SaveWorkspace("{0}")'.format(f.name)) +cpdef void gap_sig_on() noexcept: + # Install GAP's own SIGINT handler, typically for the duration of + # some libgap commands. We install it for SIGALRM too so that the + # doctest runner can use alarm() to interrupt it. + global gap_sigint_sa + global sage_sigint_sa + global sage_sigalrm_sa + sigaction(SIGINT, &gap_sigint_sa, &sage_sigint_sa) + sigaction(SIGALRM, &gap_sigint_sa, &sage_sigalrm_sa) + +cpdef void gap_sig_off() noexcept: + # Restore the Sage handlers that were saved & overwritten in + # gap_sig_on(). Better make sure the two are paired correctly! + global sage_sigint_sa + global sage_sigalrm_sa + sigaction(SIGINT, &sage_sigint_sa, NULL) + sigaction(SIGALRM, &sage_sigalrm_sa, NULL) ############################################################################ ### Evaluate string in GAP ################################################# From a9473cac4afc8d055813bc059dcbbe66239e918d Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Sat, 16 Aug 2025 22:12:47 -0400 Subject: [PATCH 05/37] src/sage/libs/gap/util.pyx: replace sig_on / sig_off Replace the sig_on() and sig_off() wrappers from cysignals with the new custom gap_sig_on() and gap_sig_off(). We've lost the sig_on() and sig_off() around GAP_initialize() because I don't think we can trust GAP's own signal handler until after we've initialized GAP. --- src/sage/libs/gap/util.pyx | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/sage/libs/gap/util.pyx b/src/sage/libs/gap/util.pyx index 995ccac4175..a70a1cf0692 100644 --- a/src/sage/libs/gap/util.pyx +++ b/src/sage/libs/gap/util.pyx @@ -19,7 +19,6 @@ from posix.signal cimport sigaction, sigaction_t, sigemptyset from cpython.exc cimport PyErr_Fetch, PyErr_Restore from cpython.object cimport Py_LT, Py_LE, Py_EQ, Py_NE, Py_GT, Py_GE from cpython.ref cimport PyObject, Py_XINCREF, Py_XDECREF -from cysignals.signals cimport sig_on, sig_off import os import warnings @@ -265,14 +264,12 @@ cdef initialize(): argv[argc] = NULL - sig_on() # Initialize GAP but disable its signal handlers: we only want # them to be invoked while libgap code is executing, whereas the # default would enable them globally. We will explicitly wrap GAP # computations in gap_sig_on() and gap_sig_off() instead. GAP_Initialize(argc, argv, gasman_callback, error_handler, handleSignals=False) - sig_off() # Configure a SIGINT handler (which can be enabled by calling # gap_sig_on) to run InterruptExecStat. This is essentially GAP's @@ -391,8 +388,9 @@ cdef Obj gap_eval(str gap_string) except? NULL: # so that Cython doesn't deallocate it before GAP is done with # its contents. cmd = str_to_bytes(gap_string + ';\n') - sig_on() + try: + gap_sig_on() GAP_Enter() result = GAP_EvalString(cmd) # We can assume that the result object is a GAP PList (plain list) @@ -423,9 +421,15 @@ cdef Obj gap_eval(str gap_string) except? NULL: # this like returning None) return GAP_ElmList(result, 2) + 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() ############################################################################ From fc16eaeba60c40d82132ef51b1163b8bffbffb0e Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Sat, 16 Aug 2025 22:15:00 -0400 Subject: [PATCH 06/37] src/sage/libs/gap/element.pyx: replace sig_on / sig_off Replace the sig_on() and sig_off() wrappers from cysignals with the new custom gap_sig_on() and gap_sig_off(). This fixes the segfault that occurs after repeated testing of _pow_. --- src/sage/libs/gap/element.pyx | 122 ++++++++++++++++++++++++++-------- 1 file changed, 93 insertions(+), 29 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index 2ef40fd0a69..30393edab07 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -16,12 +16,11 @@ elements. For general information about GAP, you should read the # **************************************************************************** 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 @@ -935,13 +934,21 @@ cdef class GapElement(RingElement): if self._compare_by_id: return id(self) == id(other) cdef GapElement c_other = 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: """ @@ -957,13 +964,21 @@ cdef class GapElement(RingElement): if self._compare_by_id: return id(self) < id(other) cdef GapElement c_other = 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""" @@ -986,12 +1001,18 @@ cdef class GapElement(RingElement): """ cdef Obj result try: - sig_GAP_Enter() - sig_on() + gap_sig_on() + GAP_Enter() result = GAP_SUM(self.value, (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): @@ -1014,12 +1035,18 @@ cdef class GapElement(RingElement): """ cdef Obj result try: - sig_GAP_Enter() - sig_on() + gap_sig_on() + GAP_Enter() result = GAP_DIFF(self.value, (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): @@ -1043,12 +1070,18 @@ cdef class GapElement(RingElement): """ cdef Obj result try: - sig_GAP_Enter() - sig_on() + gap_sig_on() + GAP_Enter() result = GAP_PROD(self.value, (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): @@ -1077,12 +1110,18 @@ cdef class GapElement(RingElement): """ cdef Obj result try: - sig_GAP_Enter() - sig_on() + gap_sig_on() + GAP_Enter() result = GAP_QUO(self.value, (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): @@ -1104,12 +1143,18 @@ cdef class GapElement(RingElement): """ cdef Obj result try: - sig_GAP_Enter() - sig_on() + gap_sig_on() + GAP_Enter() result = GAP_MOD(self.value, (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 _pow_(self, other): @@ -1151,12 +1196,18 @@ cdef class GapElement(RingElement): 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, (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): @@ -2511,8 +2562,8 @@ cdef class GapElement_Function(GapElement): 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: @@ -2523,9 +2574,15 @@ cdef class GapElement_Function(GapElement): result = GAP_CallFunc3Args(self.value, a[0], a[1], a[2]) else: 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 @@ -3149,13 +3206,20 @@ cdef class GapElement_Record(GapElement): """ 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): From a8879247e8cf72e80727a1fdbdf60d3e1d6aefa5 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Sat, 16 Aug 2025 22:02:37 -0400 Subject: [PATCH 07/37] src/sage/libs/gap/meson.build: remove cysignals dependency We are no longer using cysignals for this; instead we have our own gap_sig_on() and gap_sig_off() functions. --- src/sage/libs/gap/meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sage/libs/gap/meson.build b/src/sage/libs/gap/meson.build index def07898f4c..94170dca488 100644 --- a/src/sage/libs/gap/meson.build +++ b/src/sage/libs/gap/meson.build @@ -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 From a219ac9381735ca5ceddbf4ae4fa4f11e32c03e4 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Sat, 16 Aug 2025 22:13:42 -0400 Subject: [PATCH 08/37] src/sage/libs/gap/libgap.pyx: update signal handling docs The way we handle signals in libgap is now unusual, so a few paragraphs have been added to the libgap module docstring explaining how and why it is unusual. --- src/sage/libs/gap/libgap.pyx | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/src/sage/libs/gap/libgap.pyx b/src/sage/libs/gap/libgap.pyx index f5b58a929f3..98267fae085 100644 --- a/src/sage/libs/gap/libgap.pyx +++ b/src/sage/libs/gap/libgap.pyx @@ -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 From 0b66eba7adce1314c9676e3f9c1b970291e64904 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Sat, 16 Aug 2025 22:04:59 -0400 Subject: [PATCH 09/37] src/sage/doctest/util.py: handle Ctrl-C from GAP We've found that GAP's own SIGINT handler is less crashy than if we mix GAP_Enter/GAP_Leave with cysignals' sig_on and sig_off. We've installed that same handler for SIGALRM, but the code that catches it can't tell the difference and converts them both in to KeyboardInterrupt. To retain the ability to doctest this stuff, we have to catch those KeyboardInterrupts and poke at them to see if they arose from GAP. --- src/sage/doctest/util.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/sage/doctest/util.py b/src/sage/doctest/util.py index cf0d0133d94..88c35924247 100644 --- a/src/sage/doctest/util.py +++ b/src/sage/doctest/util.py @@ -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 finally: before_cancel_alarm_elapsed = walltime() - start_time cancel_alarm() From e1e8d6299c765291cc62d1d7b0e0cf89a57db7da Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Sun, 17 Aug 2025 10:20:23 -0400 Subject: [PATCH 10/37] src/sage/libs/gap/gap_includes.pxd: drop sig_GAP_Enter() This function used cysignals for sig_error(), and we have opted not to mix cysignals code with libgap code. It has in any case been replaced by plain GAP_Enter calls. --- src/sage/libs/gap/gap_includes.pxd | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/sage/libs/gap/gap_includes.pxd b/src/sage/libs/gap/gap_includes.pxd index 8aebf8e9204..ccba40269ce 100644 --- a/src/sage/libs/gap/gap_includes.pxd +++ b/src/sage/libs/gap/gap_includes.pxd @@ -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, @@ -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 From 2ca060f41d2a04fa67da6936ccb272e7dfa2b30c Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Sun, 17 Aug 2025 18:55:04 -0400 Subject: [PATCH 11/37] src/sage/libs/gap/libgap.pyx: rewrite signal handling explanation --- src/sage/libs/gap/libgap.pyx | 46 +++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/src/sage/libs/gap/libgap.pyx b/src/sage/libs/gap/libgap.pyx index 98267fae085..171ed4b3287 100644 --- a/src/sage/libs/gap/libgap.pyx +++ b/src/sage/libs/gap/libgap.pyx @@ -163,30 +163,32 @@ Using the GAP C library from Cython 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. + used to wrap code that may take a long time, and as a result, may + need to be interrupted with Ctrl-C. However, it is possible that + interrupting a function execution at an arbitrary location will + lead to inconsistent state. Internally, GAP provides a mechanism + using ``InterruptExecStat``, which sets a flag that tells GAP to + gracefully exit with an error as early as possible. We make use of + this internal mechanism to prevent segmentation fault when GAP + functions are interrupted. + + Specifically, we install GAP's own ``SIGINT`` handler (to catch + Ctrl-C) before executing any long-running GAP code, and then later + reinstall the original handler 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. From 7c48f945dd4408569450f76919e22a350e748a1a Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Mon, 18 Aug 2025 03:07:39 -0400 Subject: [PATCH 12/37] src/sage/libs/gap/util.pyx: do KeyboardInterrupt conversion earlier If we're going to raise a GAPError that resulted from a SIGINT or SIGALRM, we might as well turn it in to a KeyboardInterrupt right away. --- src/sage/libs/gap/util.pyx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/sage/libs/gap/util.pyx b/src/sage/libs/gap/util.pyx index a70a1cf0692..34ad37b4a59 100644 --- a/src/sage/libs/gap/util.pyx +++ b/src/sage/libs/gap/util.pyx @@ -510,7 +510,10 @@ cdef void error_handler() noexcept with gil: # Note that we manually need to deal with refcounts here. Py_XDECREF(exc_type) Py_XDECREF(exc_val) - exc_type = GAPError + if "user interrupt" in msg: + exc_type = KeyboardInterrupt + else: + exc_type = GAPError exc_val = msg Py_XINCREF(exc_type) Py_XINCREF(exc_val) From fb458ad494e7238bc84a4236ce265abb9600c23e Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Mon, 18 Aug 2025 03:08:12 -0400 Subject: [PATCH 13/37] src/sage/doctest/util.py: update GAP workaround We are no longer retaining the __cause__ of KeyboardInterrupts that arise from GAP, but the error message is still there, so we can use that to filter these out. --- src/sage/doctest/util.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/sage/doctest/util.py b/src/sage/doctest/util.py index 88c35924247..4cf8578b2bd 100644 --- a/src/sage/doctest/util.py +++ b/src/sage/doctest/util.py @@ -887,8 +887,7 @@ def ensure_interruptible_after(seconds: float, max_wait_after_interrupt: float = 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): + if "user interrupt" in str(e): # 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 From 51c81b0cd3b8d0afc4e4e04be1ba97d9cbe7e411 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Mon, 18 Aug 2025 12:09:30 -0400 Subject: [PATCH 14/37] src/sage/libs/gap: remove redundant KeyboardInterrupt hacks We are now converting "GAPError: Error, user interrupt" into a KeyboardInterrupt in our libgap error handler, so we don't have to do it at every call site. --- src/sage/libs/gap/element.pyx | 60 ----------------------------------- src/sage/libs/gap/util.pyx | 6 ---- 2 files changed, 66 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index 30393edab07..eaa719698c4 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -939,12 +939,6 @@ cdef class GapElement(RingElement): 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() gap_sig_off() @@ -969,12 +963,6 @@ cdef class GapElement(RingElement): 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() gap_sig_off() @@ -1004,12 +992,6 @@ cdef class GapElement(RingElement): gap_sig_on() GAP_Enter() result = GAP_SUM(self.value, (right).value) - except GAPError as e: - if "user interrupt" in str(e): - # Ctrl-C - raise KeyboardInterrupt from e - else: - raise finally: GAP_Leave() gap_sig_off() @@ -1038,12 +1020,6 @@ cdef class GapElement(RingElement): gap_sig_on() GAP_Enter() result = GAP_DIFF(self.value, (right).value) - except GAPError as e: - if "user interrupt" in str(e): - # Ctrl-C - raise KeyboardInterrupt from e - else: - raise finally: GAP_Leave() gap_sig_off() @@ -1073,12 +1049,6 @@ cdef class GapElement(RingElement): gap_sig_on() GAP_Enter() result = GAP_PROD(self.value, (right).value) - except GAPError as e: - if "user interrupt" in str(e): - # Ctrl-C - raise KeyboardInterrupt from e - else: - raise finally: GAP_Leave() gap_sig_off() @@ -1113,12 +1083,6 @@ cdef class GapElement(RingElement): gap_sig_on() GAP_Enter() result = GAP_QUO(self.value, (right).value) - except GAPError as e: - if "user interrupt" in str(e): - # Ctrl-C - raise KeyboardInterrupt from e - else: - raise finally: GAP_Leave() gap_sig_off() @@ -1146,12 +1110,6 @@ cdef class GapElement(RingElement): gap_sig_on() GAP_Enter() result = GAP_MOD(self.value, (right).value) - except GAPError as e: - if "user interrupt" in str(e): - # Ctrl-C - raise KeyboardInterrupt from e - else: - raise finally: GAP_Leave() gap_sig_off() @@ -1199,12 +1157,6 @@ cdef class GapElement(RingElement): gap_sig_on() GAP_Enter() # GAPError raised from here result = GAP_POW(self.value, (other).value) - except GAPError as e: - if "user interrupt" in str(e): - # Ctrl-C - raise KeyboardInterrupt from e - else: - raise finally: GAP_Leave() gap_sig_off() @@ -2574,12 +2526,6 @@ cdef class GapElement_Function(GapElement): result = GAP_CallFunc3Args(self.value, a[0], a[1], a[2]) else: result = GAP_CallFuncList(self.value, arg_list) - except GAPError as e: - if "user interrupt" in str(e): - # Ctrl-C - raise KeyboardInterrupt from e - else: - raise finally: GAP_Leave() gap_sig_off() @@ -3211,12 +3157,6 @@ cdef class GapElement_Record(GapElement): 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() gap_sig_off() diff --git a/src/sage/libs/gap/util.pyx b/src/sage/libs/gap/util.pyx index 34ad37b4a59..9ead4940765 100644 --- a/src/sage/libs/gap/util.pyx +++ b/src/sage/libs/gap/util.pyx @@ -421,12 +421,6 @@ cdef Obj gap_eval(str gap_string) except? NULL: # this like returning None) return GAP_ElmList(result, 2) - except GAPError as e: - if "user interrupt" in str(e): - # Ctrl-C - raise KeyboardInterrupt from e - else: - raise finally: GAP_Leave() gap_sig_off() From bab97f6c2f25744b10959dc632c4812558500019 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Mon, 18 Aug 2025 13:49:05 -0400 Subject: [PATCH 15/37] src/sage/libs/gap/util.pyx: type-safety for InterruptExecStat() Wrap InterruptExecStat() in a function that takes an int so that the associated function pointer is what sigaction_t.sa_handler expects --- src/sage/libs/gap/util.pyx | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/sage/libs/gap/util.pyx b/src/sage/libs/gap/util.pyx index 9ead4940765..96ef1a082f5 100644 --- a/src/sage/libs/gap/util.pyx +++ b/src/sage/libs/gap/util.pyx @@ -187,6 +187,11 @@ cdef sigaction_t gap_sigint_sa cdef sigaction_t sage_sigint_sa cdef sigaction_t sage_sigalrm_sa +cdef void gap_interrupt_asap(int signum): + # A wrapper around InterruptExecStat(). This tells GAP to raise an + # error at the next opportunity. + InterruptExecStat() + cdef initialize(): """ Initialize the GAP library, if it hasn't already been @@ -276,7 +281,7 @@ cdef initialize(): # own SIGINT handler (but without the double-Ctrl-C behavior), and # is less crashy than when we mix cysignals with GAP code. global gap_sigint_sa - gap_sigint_sa.sa_handler = InterruptExecStat + gap_sigint_sa.sa_handler = gap_interrupt_asap sigemptyset(&(gap_sigint_sa.sa_mask)) gap_sigint_sa.sa_flags = 0; From b298ca80a03c11fded43a8fd72b8b42a7eea0e13 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Mon, 18 Aug 2025 13:59:26 -0400 Subject: [PATCH 16/37] src/sage/doctest/util.py: further simplify GAP workaround Since AlarmInterrupt is a subclass of KeyboardInterrupt, we can catch them both with "except KeyboardInterrupt" and then filter out the ones we want. This reduces a small amount of code duplication. --- src/sage/doctest/util.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/sage/doctest/util.py b/src/sage/doctest/util.py index 4cf8578b2bd..ddc59ddbaa2 100644 --- a/src/sage/doctest/util.py +++ b/src/sage/doctest/util.py @@ -883,17 +883,12 @@ def ensure_interruptible_after(seconds: float, max_wait_after_interrupt: float = try: yield data - except AlarmInterrupt as e: - e.__traceback__ = None # workaround for https://github.com/python/cpython/pull/129276 - alarm_raised = True except KeyboardInterrupt as e: - if "user interrupt" in str(e): - # 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. + # AlarmInterrupt is a subclass of KeyboardInterrupt, so this + # catches both. The "user interrupt" message is a quirk of + # GAP interrupts that result from SIGALRM. + if isinstance(e, AlarmInterrupt) or "user interrupt" in str(e): + # workaround for https://github.com/python/cpython/pull/129276 e.__traceback__ = None alarm_raised = True else: From 867cf66841b63721e7c19e54f1e506d8b5a48f2e Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Mon, 18 Aug 2025 14:53:12 -0400 Subject: [PATCH 17/37] src/sage/libs/gap/libgap.pyx: explain error handling The way errors are handled in this interface is tricky. It is now explained to the best of my ability in the module's docstring, and the (related) paragraphs about signal handling have been updated as well. --- src/sage/libs/gap/libgap.pyx | 101 ++++++++++++++++++++++++++++------- 1 file changed, 82 insertions(+), 19 deletions(-) diff --git a/src/sage/libs/gap/libgap.pyx b/src/sage/libs/gap/libgap.pyx index 171ed4b3287..c348c5fd418 100644 --- a/src/sage/libs/gap/libgap.pyx +++ b/src/sage/libs/gap/libgap.pyx @@ -163,17 +163,84 @@ Using the GAP C library from Cython 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. However, it is possible that - interrupting a function execution at an arbitrary location will - lead to inconsistent state. Internally, GAP provides a mechanism - using ``InterruptExecStat``, which sets a flag that tells GAP to - gracefully exit with an error as early as possible. We make use of - this internal mechanism to prevent segmentation fault when GAP - functions are interrupted. + All GAP library usage should be sandwiched between calls to + ``GAP_Enter()`` and ``GAP_Leave()``. These are macros defined in + ``libgap-api.h`` and must be used carefully because ``GAP_Enter()`` + is defined as two function calls in succession without braces. The + first thing that ``GAP_Enter()`` does is a ``setjmp()`` which plays + an important role in handling errors. The return value from + ``GAP_Enter()`` is non-zero (success) the first time around, and if + an error occurs, execution "jumps" back to ``GAP_Enter()``, this + time with a return value of zero (failure). Due to these + implementation details, we cannot simply check the return value + before executing Cython code; the following *does not* work:: + + if (GAP_Enter()): + # further calls to libgap + GAP_Leave() + + Instead you will see something like:: + + try: + GAP_Enter() + # further calls to libgap + finally: + GAP_Leave() + + How this works is subtle. When GAP is initialized, we install an + ``error_handler()`` callback that GAP invokes on error. This + function sets a python exception using ``PyErr_Restore()``, but so + long as we remain in C, this exception will not actually be + raised. When ``error_handler()`` finishes executing, control + returns to GAP which then jumps back to the previous + ``GAP_Enter()``. It is at this point that we need to raise the + (already set) exception, to prevent re-executing the code that + caused an error. To facilitate this, ``GAP_Enter()`` is wrapped by + Cython, and the wrapper is qualified with ``except 0``. This means + that Cython will treat a return value from the ``GAP_Enter()`` + macro as an error, and raise an exception if one is set. (One will + be if there was an error because our ``error_handler()`` sets + it). Here is a real example:: + + try: + GAP_Enter() + libgap.Sum(1,2,3,4) + finally: + GAP_Leave() + + The call to ``libgap.Sum(1,2,3,4)`` is an error in this case, + because summing four numbers requires them to be passed as a + list. In any case, what happens is, + + #. We call the ``GAP_Enter()`` Cython wrapper, which invokes the + macro, and additionally generates some C code to raise an + exception if that return value is zero (error). But this is the + first pass, so for now the macro returns a non-zero (success) + value. + #. We call ``libgap.Sum(1,2,3,4)``, which is an error. + #. GAP invokes our ``error_handler()``, which creates a + :exc:`sage.libs.gap.util.GAPError`, and sets it active. + #. Control returns to GAP. + #. GAP jumps back to ``GAP_Enter()``. + #. The error branch of ``GAP_Enter()`` is executed. In other words + we proceed from ``GAP_Enter()`` as if it returned zero (error). + #. An exception is raised, because the ``except 0`` qualifier on the + Cython wrapper for ``GAP_Enter()`` specifically checks for zero + and raises any exceptions in that case. + #. Finally, ``GAP_Leave()`` is called to clean up. In a more + realistic example where failure is not guaranteed, this would + also have been run to clean up if no errors were raised. + + Another unusual aspect of the libgap 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. However, it is + possible that interrupting a function execution at an arbitrary + location will lead to inconsistent state. Internally, GAP provides + a mechanism using ``InterruptExecStat``, which sets a flag that + tells GAP to gracefully exit with an error as early as possible. We + make use of this internal mechanism to prevent segmentation faults + when GAP functions are interrupted. Specifically, we install GAP's own ``SIGINT`` handler (to catch Ctrl-C) before executing any long-running GAP code, and then later @@ -181,14 +248,10 @@ Using the GAP C library from Cython 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. + ``error_handler()`` that will set a :exc:`KeyboardInterrupt` + containing the phrase "user interrupt". Eventually (as explained in + the preceding paragraphs), control will jump back to the Cython + wrapper for ``GAP_Enter()``, and this exception will be raised. Before you attempt to change any of this, please make sure that you understand the issues that it is intended to fix, e.g. From cd93bdf359c76491c46e3d14a6be734704bd46dd Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Tue, 19 Aug 2025 11:46:30 -0400 Subject: [PATCH 18/37] src/sage/libs/gap/util.pyx: delete old commented debug statement --- src/sage/libs/gap/util.pyx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/sage/libs/gap/util.pyx b/src/sage/libs/gap/util.pyx index 96ef1a082f5..ea889a62abb 100644 --- a/src/sage/libs/gap/util.pyx +++ b/src/sage/libs/gap/util.pyx @@ -130,7 +130,6 @@ cdef void reference_obj(Obj obj) noexcept: """ cdef ObjWrapper wrapped = wrap_obj(obj) global owned_objects_refcount -# print("reference_obj called "+ crepr(obj) +"\n") if wrapped in owned_objects_refcount: owned_objects_refcount[wrapped] += 1 else: From 794b427bb279c24e2d6e6048651c3cd633abc705 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Tue, 19 Aug 2025 14:20:45 -0400 Subject: [PATCH 19/37] src/sage/libs/gap: use public API for is_string() We have GAP_IsString() in the public API now, it is preferable to using the internal IS_STRING. --- src/sage/libs/gap/element.pyx | 6 +++++- src/sage/libs/gap/gap_includes.pxd | 1 - 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index eaa719698c4..436bed1aa1d 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -1245,7 +1245,11 @@ cdef class GapElement(RingElement): sage: libgap('this is a string').is_string() True """ - return IS_STRING(self.value) + try: + GAP_Enter() + return GAP_IsString(self.value) + finally: + GAP_Leave() def is_permutation(self): r""" diff --git a/src/sage/libs/gap/gap_includes.pxd b/src/sage/libs/gap/gap_includes.pxd index ccba40269ce..a1c84561145 100644 --- a/src/sage/libs/gap/gap_includes.pxd +++ b/src/sage/libs/gap/gap_includes.pxd @@ -139,7 +139,6 @@ cdef extern from "gap/records.h" nogil: cdef extern from "gap/stringobj.h" nogil: - bint IS_STRING(Obj obj) bint IsStringConv(Obj obj) Obj NEW_STRING(Int) From ee6af075634819624d26aaa4d31bbf9f8c63b50a Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Tue, 19 Aug 2025 15:07:29 -0400 Subject: [PATCH 20/37] src/sage/libs/gap/element.pyx: missing GAP_Enter / GAP_Leave The is_list() and is_record() methods call libgap, and need to be sandwiched between GAP_Enter and GAP_Leave. --- src/sage/libs/gap/element.pyx | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index 436bed1aa1d..b7384849f31 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -1201,7 +1201,11 @@ cdef class GapElement(RingElement): sage: libgap.eval('3/2').is_list() False """ - return GAP_IsList(self.value) + try: + GAP_Enter() + return GAP_IsList(self.value) + finally: + GAP_Leave() def is_record(self): r""" @@ -1216,7 +1220,11 @@ cdef class GapElement(RingElement): sage: libgap.eval('rec(a:=1, b:=3)').is_record() True """ - return GAP_IsRecord(self.value) + try: + GAP_Enter() + return GAP_IsRecord(self.value) + finally: + GAP_Leave() cpdef is_bool(self): r""" From e7bb22283096794d7dd671c8421042a13722a5e4 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Tue, 19 Aug 2025 15:15:58 -0400 Subject: [PATCH 21/37] src/sage/libs/gap/element.pyx: drop superclass list -> sage conversion There's already a sage() method in the GapElement_List subclass. If you have a list but you wound up with a generic GapElement instead of a GapElement_List, you probably used the API wrong. --- src/sage/libs/gap/element.pyx | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index b7384849f31..bc9449b317f 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -1391,11 +1391,6 @@ cdef class GapElement(RingElement): x = R.gen() return x**val * R(num) / R(den) - elif self.IsList(): - # May be a list-like collection of some other type of GapElements - # that we can convert - return [item.sage() for item in self.AsList()] - elif self.IsFreeGroup(): from sage.groups.free_group import FreeGroup_class names = tuple(str(g) for g in self.GeneratorsOfGroup()) From d9bd3c2fa158931049a25e5517d9e5e08ed5e29c Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Tue, 19 Aug 2025 14:56:52 -0400 Subject: [PATCH 22/37] src/sage/libs/gap: be more careful inside make_any_gap_element() This function performs a bunch of heuristics to decide what type of GAP element to make. In doing so, it jumps between libgap (C) code sage (python) code, all while inside of a GAP_Enter / GAP_Leave pair. Moreover, some of the functions it calls are wrapped in their own GAP_Enter / GAP_Leave pairs. This commit reorganizes things so that the libgap code, wrapped in GAP_Enter and GAP_Leave, comes first. The libgap code decides what type of GAP element we have. It is then followed by the python case statement that decides what type of Sage element we'll construct. We've also dropped the internal IsStringConv() function in favor of the public GAP_IsString(). The latter does not convert a list of characters to an efficient string representation, but modulo some doctests that have been updated, that should not be a problem: if you don't want a list of characters, don't pass in a list of characters. --- src/sage/libs/gap/element.pyx | 140 ++++++++++++++++++++--------- src/sage/libs/gap/gap_includes.pxd | 3 +- 2 files changed, 99 insertions(+), 44 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index bc9449b317f..c9ad91ef362 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -259,6 +259,18 @@ cdef Obj make_gap_string(sage_string) except NULL: ### generic construction of GapElements #################################### ############################################################################ + +# Custom identifiers for the types of GAP objects that have associated +# GAP_IsFoo() functions in the libgap API. +cdef enum: + OBJ_TYPE_UND = 0 + OBJ_TYPE_INT = 1 + OBJ_TYPE_FLT = 2 + OBJ_TYPE_CHR = 4 + OBJ_TYPE_STR = 8 + OBJ_TYPE_LST = 16 + OBJ_TYPE_REC = 32 + cdef GapElement make_any_gap_element(parent, Obj obj): """ Return the GAP element wrapper of ``obj``. @@ -275,16 +287,31 @@ cdef GapElement make_any_gap_element(parent, Obj obj): sage: type(x) - sage: libgap.eval("['a', 'b', 'c']") # gap strings are also lists of chars + Strings in GAP are lists of characters and vice-versa, but that + isn't an efficient representation. Keep this in mind if you call a + list function on a string; you may want to convert the results + to the more efficient representation afterwards. In the example + below, ``UnorderedTuples()`` doesn't know we are dealing with + strings, so it returns lists of characters:: + + sage: libgap.eval("['a', 'b', 'c']") "abc" - sage: t = libgap.UnorderedTuples('abc', 2); t + sage: t = libgap.UnorderedTuples('abc', 2) + sage: t # GAP still shows you strings... [ "aa", "ab", "ac", "bb", "bc", "cc" ] + sage: t.sage() # but really we have lists of characters + [['a', 'a'], ['a', 'b'], ['a', 'c'], ['b', 'b'], ['b', 'c'], + ['c', 'c']] sage: t[1] "ab" sage: t[1].sage() + ['a', 'b'] + sage: s = t[1].CopyToStringRep() # make an efficient copy + sage: s.sage() + 'ab' + sage: t[1].ConvertToStringRep() # convert the original + sage: t[1].sage() 'ab' - sage: t.sage() - ['aa', 'ab', 'ac', 'bb', 'bc', 'cc'] Check that :issue:`18158` is fixed:: @@ -295,53 +322,82 @@ cdef GapElement make_any_gap_element(parent, Obj obj): sage: irr[1] 0 """ - cdef int num + if obj is NULL: + return make_GapElement(parent, obj) + + # The object's type, if it can be determined from the libgap API. + # Defaults to "undefined." + cdef int obj_type = OBJ_TYPE_UND + # Plan A: Determine the type of ``obj`` using the libgap API. + # Make all libgap-api calls here, between GAP_Enter/GAP_Leave. + # In particular we avoid any *other* functions that may invoke + # GAP_Enter/GAP_Leave. try: GAP_Enter() - if obj is NULL: - return make_GapElement(parent, obj) - num = TNUM_OBJ(obj) if GAP_IsInt(obj): - return make_GapElement_Integer(parent, obj) + obj_type = OBJ_TYPE_INT elif GAP_IsMacFloat(obj): - return make_GapElement_Float(parent, obj) - elif num == T_CYC: - return make_GapElement_Cyclotomic(parent, obj) - elif num == T_FFE: - return make_GapElement_FiniteField(parent, obj) - elif num == T_RAT: - return make_GapElement_Rational(parent, obj) - elif num == T_BOOL: - return make_GapElement_Boolean(parent, obj) - elif num == T_FUNCTION: - return make_GapElement_Function(parent, obj) - elif num == T_PERM2 or num == T_PERM4: - return make_GapElement_Permutation(parent, obj) - elif GAP_IsRecord(obj): - return make_GapElement_Record(parent, obj) - elif GAP_IsList(obj) and GAP_LenList(obj) == 0: - # Empty lists are lists and not strings in Python - return make_GapElement_List(parent, obj) - elif IsStringConv(obj): - # GAP strings are lists, too. Make sure this comes before non-empty make_GapElement_List - return make_GapElement_String(parent, obj) + obj_type = OBJ_TYPE_FLT + elif GAP_IsChar(obj): + obj_type = OBJ_TYPE_CHR + elif GAP_IsString(obj): + # Strings are also lists, so we check for strings (more + # specific) before we check for lists (less specific). + obj_type = OBJ_TYPE_STR elif GAP_IsList(obj): - return make_GapElement_List(parent, obj) - elif GAP_ValueOfChar(obj) != -1: - ch = make_GapElement(parent, obj).IntChar().sage() - return make_GapElement_String(parent, make_gap_string(chr(ch))) - result = make_GapElement(parent, obj) - if num == T_POSOBJ: - if result.IsZmodnZObj(): - return make_GapElement_IntegerMod(parent, obj) - if num == T_COMOBJ: - if result.IsRing(): - return make_GapElement_Ring(parent, obj) - return result + obj_type = OBJ_TYPE_LST + elif GAP_IsRecord(obj): + obj_type = OBJ_TYPE_REC finally: GAP_Leave() + if obj_type == OBJ_TYPE_INT: + return make_GapElement_Integer(parent, obj) + elif obj_type == OBJ_TYPE_FLT: + return make_GapElement_Float(parent, obj) + elif obj_type == OBJ_TYPE_CHR: + # IntChar is a GAP function call; these do their own GAP_Enter + # and GAP_Leave, as does make_gap_string(). + ch = make_GapElement(parent, obj).IntChar().sage() + return make_GapElement_String(parent, make_gap_string(chr(ch))) + elif obj_type == OBJ_TYPE_STR: + return make_GapElement_String(parent, obj) + elif obj_type == OBJ_TYPE_LST: + return make_GapElement_List(parent, obj) + elif obj_type == OBJ_TYPE_REC: + return make_GapElement_Record(parent, obj) + + # Plan B: Use the "private" API to determine the type of ``obj``. + # Since TNUM_OBJ does not invoke the public libgap API, it is not + # wrapped in GAP_Enter / GAP_Leave. + cdef int num = TNUM_OBJ(obj) + if num == T_BOOL: + return make_GapElement_Boolean(parent, obj) + elif num == T_RAT: + return make_GapElement_Rational(parent, obj) + elif num == T_FUNCTION: + return make_GapElement_Function(parent, obj) + elif num == T_CYC: + return make_GapElement_Cyclotomic(parent, obj) + elif num == T_FFE: + return make_GapElement_FiniteField(parent, obj) + elif num == T_PERM2 or num == T_PERM4: + return make_GapElement_Permutation(parent, obj) + + # Plan C: Make a generic element, and then use GAP functions to + # check if it's an IntegerMod or a Ring. + result = make_GapElement(parent, obj) + if num == T_POSOBJ: + if result.IsZmodnZObj(): + return make_GapElement_IntegerMod(parent, obj) + if num == T_COMOBJ: + if result.IsRing(): + return make_GapElement_Ring(parent, obj) + + # Plan D: return the generic element. + return result + ############################################################################ ### GapElement ############################################################# diff --git a/src/sage/libs/gap/gap_includes.pxd b/src/sage/libs/gap/gap_includes.pxd index a1c84561145..ca12f936281 100644 --- a/src/sage/libs/gap/gap_includes.pxd +++ b/src/sage/libs/gap/gap_includes.pxd @@ -87,7 +87,7 @@ cdef extern from "gap/libgap-api.h" nogil: char* GAP_CSTR_STRING(Obj list) Obj GAP_MakeStringWithLen(const char* buf, UInt len) - Int GAP_ValueOfChar(Obj obj) + bint GAP_IsChar(Obj obj) cdef extern from "gap/lists.h" nogil: @@ -139,7 +139,6 @@ cdef extern from "gap/records.h" nogil: cdef extern from "gap/stringobj.h" nogil: - bint IsStringConv(Obj obj) Obj NEW_STRING(Int) From e79bdd7180beabfcf2fd10df780470a678892c65 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Wed, 20 Aug 2025 14:29:37 -0400 Subject: [PATCH 23/37] src/sage/libs/gap/element.pyx: revert list-of-char changes An earlier commit dropped the implicit conversion of (non-empty) lists-of-char to efficient strings in GAP. Here we reinstate the old behavior and the associated doctest because it was backwards- incompatible. --- src/sage/libs/gap/element.pyx | 42 +++++++++++++++++------------------ 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index c9ad91ef362..f210a0dc6a1 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -287,31 +287,16 @@ cdef GapElement make_any_gap_element(parent, Obj obj): sage: type(x) - Strings in GAP are lists of characters and vice-versa, but that - isn't an efficient representation. Keep this in mind if you call a - list function on a string; you may want to convert the results - to the more efficient representation afterwards. In the example - below, ``UnorderedTuples()`` doesn't know we are dealing with - strings, so it returns lists of characters:: - - sage: libgap.eval("['a', 'b', 'c']") + sage: libgap.eval("['a', 'b', 'c']") # gap strings are also lists of chars "abc" - sage: t = libgap.UnorderedTuples('abc', 2) - sage: t # GAP still shows you strings... + sage: t = libgap.UnorderedTuples('abc', 2); t [ "aa", "ab", "ac", "bb", "bc", "cc" ] - sage: t.sage() # but really we have lists of characters - [['a', 'a'], ['a', 'b'], ['a', 'c'], ['b', 'b'], ['b', 'c'], - ['c', 'c']] sage: t[1] "ab" sage: t[1].sage() - ['a', 'b'] - sage: s = t[1].CopyToStringRep() # make an efficient copy - sage: s.sage() - 'ab' - sage: t[1].ConvertToStringRep() # convert the original - sage: t[1].sage() 'ab' + sage: t.sage() + ['aa', 'ab', 'ac', 'bb', 'bc', 'cc'] Check that :issue:`18158` is fixed:: @@ -342,8 +327,7 @@ cdef GapElement make_any_gap_element(parent, Obj obj): elif GAP_IsChar(obj): obj_type = OBJ_TYPE_CHR elif GAP_IsString(obj): - # Strings are also lists, so we check for strings (more - # specific) before we check for lists (less specific). + # Efficient representation only, returns False on lists. obj_type = OBJ_TYPE_STR elif GAP_IsList(obj): obj_type = OBJ_TYPE_LST @@ -364,7 +348,21 @@ cdef GapElement make_any_gap_element(parent, Obj obj): elif obj_type == OBJ_TYPE_STR: return make_GapElement_String(parent, obj) elif obj_type == OBJ_TYPE_LST: - return make_GapElement_List(parent, obj) + result = make_GapElement_List(parent, obj) + if result.IsString() and result.Length() > 0: + # IsString() thinks a list-of-char is a string, but + # GAP_IsString() does not. For backwards compatibility we + # convert list-of-char to an efficient string here, and + # return an instance of the more-specific class. The + # length check is necessary to prevent the conversion of + # all empty lists to empty strings. A related problem is + # that you cannot construct an actual list-of-char here + # if you want to. + result = make_GapElement_String(parent, obj) + result.ConvertToStringRep() + return result + else: + return result elif obj_type == OBJ_TYPE_REC: return make_GapElement_Record(parent, obj) From 3220d8f3888e10c324996d1a3b203d85b8495998 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Wed, 20 Aug 2025 15:24:03 -0400 Subject: [PATCH 24/37] src/sage/libs/gap/libgap.pyx: more realistic "real" example A real example for GAP_Enter / GAP_Leave should only have libgap C code sandwiched between the two. --- src/sage/libs/gap/libgap.pyx | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/sage/libs/gap/libgap.pyx b/src/sage/libs/gap/libgap.pyx index c348c5fd418..92bbac0e49b 100644 --- a/src/sage/libs/gap/libgap.pyx +++ b/src/sage/libs/gap/libgap.pyx @@ -202,22 +202,26 @@ Using the GAP C library from Cython be if there was an error because our ``error_handler()`` sets it). Here is a real example:: - try: - GAP_Enter() - libgap.Sum(1,2,3,4) - finally: - GAP_Leave() - - The call to ``libgap.Sum(1,2,3,4)`` is an error in this case, - because summing four numbers requires them to be passed as a - list. In any case, what happens is, + cpdef void crash_and_burn() except *: + cdef GapElement x = libgap({'a': 1, 'b': 2}) + cdef unsigned int xlen + try: + GAP_Enter() + xlen = GAP_LenList(x.value) + finally: + GAP_Leave() + print(xlen) + + The call to ``GAP_LenList()`` is an error in this case, because + ``x.value`` is a GAP record, not a GAP list. In any case, what + happens is, #. We call the ``GAP_Enter()`` Cython wrapper, which invokes the macro, and additionally generates some C code to raise an exception if that return value is zero (error). But this is the first pass, so for now the macro returns a non-zero (success) value. - #. We call ``libgap.Sum(1,2,3,4)``, which is an error. + #. We call ``GAP_LenList(x.value)``, which is an error. #. GAP invokes our ``error_handler()``, which creates a :exc:`sage.libs.gap.util.GAPError`, and sets it active. #. Control returns to GAP. From 24065b53b5c899b336ec0d1eac6d36b01741af2e Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Wed, 20 Aug 2025 15:29:15 -0400 Subject: [PATCH 25/37] src/sage/libs/gap/libgap.pyx: clarify "GAP library usage" We specificaly mean the GAP C library, declared in libgap-api.h. --- src/sage/libs/gap/libgap.pyx | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/sage/libs/gap/libgap.pyx b/src/sage/libs/gap/libgap.pyx index 92bbac0e49b..ab401ec9248 100644 --- a/src/sage/libs/gap/libgap.pyx +++ b/src/sage/libs/gap/libgap.pyx @@ -163,17 +163,18 @@ Using the GAP C library from Cython We are using the GAP API provided by the GAP project since GAP 4.10. - All GAP library usage should be sandwiched between calls to - ``GAP_Enter()`` and ``GAP_Leave()``. These are macros defined in - ``libgap-api.h`` and must be used carefully because ``GAP_Enter()`` - is defined as two function calls in succession without braces. The - first thing that ``GAP_Enter()`` does is a ``setjmp()`` which plays - an important role in handling errors. The return value from - ``GAP_Enter()`` is non-zero (success) the first time around, and if - an error occurs, execution "jumps" back to ``GAP_Enter()``, this - time with a return value of zero (failure). Due to these - implementation details, we cannot simply check the return value - before executing Cython code; the following *does not* work:: + Calls to the GAP C library (functions declared in libgap-api.h) + should be sandwiched between calls to ``GAP_Enter()`` and + ``GAP_Leave()``. These are macros defined in ``libgap-api.h`` and + must be used carefully because ``GAP_Enter()`` is defined as two + function calls in succession without braces. The first thing that + ``GAP_Enter()`` does is a ``setjmp()`` which plays an important + role in handling errors. The return value from ``GAP_Enter()`` is + non-zero (success) the first time around, and if an error occurs, + execution "jumps" back to ``GAP_Enter()``, this time with a return + value of zero (failure). Due to these implementation details, we + cannot simply check the return value before executing Cython code; + the following *does not* work:: if (GAP_Enter()): # further calls to libgap From 041fcdf9bb560fbf1398d2c3d6b1d7395ff16c45 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Wed, 20 Aug 2025 19:52:34 -0400 Subject: [PATCH 26/37] src/sage/libs/gap/libgap.pyx: fix module docstring indentation Somehow we wound up with a three-space indentation. Make it four. --- src/sage/libs/gap/libgap.pyx | 207 ++++++++++++++++++----------------- 1 file changed, 104 insertions(+), 103 deletions(-) diff --git a/src/sage/libs/gap/libgap.pyx b/src/sage/libs/gap/libgap.pyx index ab401ec9248..06f130ec882 100644 --- a/src/sage/libs/gap/libgap.pyx +++ b/src/sage/libs/gap/libgap.pyx @@ -160,112 +160,113 @@ using the recursive expansion of the Using the GAP C library from Cython =================================== - We are using the GAP API provided by the GAP project since - GAP 4.10. - - Calls to the GAP C library (functions declared in libgap-api.h) - should be sandwiched between calls to ``GAP_Enter()`` and - ``GAP_Leave()``. These are macros defined in ``libgap-api.h`` and - must be used carefully because ``GAP_Enter()`` is defined as two - function calls in succession without braces. The first thing that - ``GAP_Enter()`` does is a ``setjmp()`` which plays an important - role in handling errors. The return value from ``GAP_Enter()`` is - non-zero (success) the first time around, and if an error occurs, - execution "jumps" back to ``GAP_Enter()``, this time with a return - value of zero (failure). Due to these implementation details, we - cannot simply check the return value before executing Cython code; - the following *does not* work:: - - if (GAP_Enter()): - # further calls to libgap - GAP_Leave() - - Instead you will see something like:: - - try: - GAP_Enter() + We are using the GAP API provided by the GAP project since + GAP 4.10. + + Calls to the GAP C library (functions declared in libgap-api.h) + should be sandwiched between calls to ``GAP_Enter()`` and + ``GAP_Leave()``. These are macros defined in ``libgap-api.h`` and + must be used carefully because ``GAP_Enter()`` is defined as two + function calls in succession without braces. The first thing that + ``GAP_Enter()`` does is a ``setjmp()`` which plays an important + role in handling errors. The return value from ``GAP_Enter()`` is + non-zero (success) the first time around, and if an error occurs, + execution "jumps" back to ``GAP_Enter()``, this time with a return + value of zero (failure). Due to these implementation details, we + cannot simply check the return value before executing Cython code; + the following *does not* work:: + + if (GAP_Enter()): # further calls to libgap - finally: GAP_Leave() - How this works is subtle. When GAP is initialized, we install an - ``error_handler()`` callback that GAP invokes on error. This - function sets a python exception using ``PyErr_Restore()``, but so - long as we remain in C, this exception will not actually be - raised. When ``error_handler()`` finishes executing, control - returns to GAP which then jumps back to the previous - ``GAP_Enter()``. It is at this point that we need to raise the - (already set) exception, to prevent re-executing the code that - caused an error. To facilitate this, ``GAP_Enter()`` is wrapped by - Cython, and the wrapper is qualified with ``except 0``. This means - that Cython will treat a return value from the ``GAP_Enter()`` - macro as an error, and raise an exception if one is set. (One will - be if there was an error because our ``error_handler()`` sets - it). Here is a real example:: - - cpdef void crash_and_burn() except *: - cdef GapElement x = libgap({'a': 1, 'b': 2}) - cdef unsigned int xlen - try: - GAP_Enter() - xlen = GAP_LenList(x.value) - finally: - GAP_Leave() - print(xlen) - - The call to ``GAP_LenList()`` is an error in this case, because - ``x.value`` is a GAP record, not a GAP list. In any case, what - happens is, - - #. We call the ``GAP_Enter()`` Cython wrapper, which invokes the - macro, and additionally generates some C code to raise an - exception if that return value is zero (error). But this is the - first pass, so for now the macro returns a non-zero (success) - value. - #. We call ``GAP_LenList(x.value)``, which is an error. - #. GAP invokes our ``error_handler()``, which creates a - :exc:`sage.libs.gap.util.GAPError`, and sets it active. - #. Control returns to GAP. - #. GAP jumps back to ``GAP_Enter()``. - #. The error branch of ``GAP_Enter()`` is executed. In other words - we proceed from ``GAP_Enter()`` as if it returned zero (error). - #. An exception is raised, because the ``except 0`` qualifier on the - Cython wrapper for ``GAP_Enter()`` specifically checks for zero - and raises any exceptions in that case. - #. Finally, ``GAP_Leave()`` is called to clean up. In a more - realistic example where failure is not guaranteed, this would - also have been run to clean up if no errors were raised. - - Another unusual aspect of the libgap 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. However, it is - possible that interrupting a function execution at an arbitrary - location will lead to inconsistent state. Internally, GAP provides - a mechanism using ``InterruptExecStat``, which sets a flag that - tells GAP to gracefully exit with an error as early as possible. We - make use of this internal mechanism to prevent segmentation faults - when GAP functions are interrupted. - - Specifically, we install GAP's own ``SIGINT`` handler (to catch - Ctrl-C) before executing any long-running GAP code, and then later - reinstall the original handler 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 set a :exc:`KeyboardInterrupt` - containing the phrase "user interrupt". Eventually (as explained in - the preceding paragraphs), control will jump back to the Cython - wrapper for ``GAP_Enter()``, and this exception will be raised. - - 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 + Instead you will see something like:: + + try: + GAP_Enter() + # further calls to libgap + finally: + GAP_Leave() + + How this works is subtle. When GAP is initialized, we install an + ``error_handler()`` callback that GAP invokes on error. This + function sets a python exception using ``PyErr_Restore()``, but so + long as we remain in C, this exception will not actually be + raised. When ``error_handler()`` finishes executing, control + returns to GAP which then jumps back to the previous + ``GAP_Enter()``. It is at this point that we need to raise the + (already set) exception, to prevent re-executing the code that + caused an error. To facilitate this, ``GAP_Enter()`` is wrapped by + Cython, and the wrapper is qualified with ``except 0``. This means + that Cython will treat a return value from the ``GAP_Enter()`` + macro as an error, and raise an exception if one is set. (One will + be if there was an error because our ``error_handler()`` sets + it). Here is a real example:: + + cpdef void crash_and_burn() except *: + cdef GapElement x = libgap({'a': 1, 'b': 2}) + cdef unsigned int xlen + try: + GAP_Enter() + xlen = GAP_LenList(x.value) + finally: + GAP_Leave() + print(xlen) + + The call to ``GAP_LenList()`` is an error in this case, because + ``x.value`` is a GAP record, not a GAP list. In any case, what + happens is, + + #. We call the ``GAP_Enter()`` Cython wrapper, which invokes the + macro, and additionally generates some C code to raise an + exception if that return value is zero (error). But this is the + first pass, so for now the macro returns a non-zero (success) + value. + #. We call ``GAP_LenList(x.value)``, which is an error. + #. GAP invokes our ``error_handler()``, which creates a + :exc:`sage.libs.gap.util.GAPError`, and sets it active. + #. Control returns to GAP. + #. GAP jumps back to ``GAP_Enter()``. + #. The error branch of ``GAP_Enter()`` is executed. In other words + we proceed from ``GAP_Enter()`` as if it returned zero (error). + #. An exception is raised, because the ``except 0`` qualifier on the + Cython wrapper for ``GAP_Enter()`` specifically checks for zero + and raises any exceptions in that case. + #. Finally, ``GAP_Leave()`` is called to clean up. In a more + realistic example where failure is not guaranteed, this would + also have been run to clean up if no errors were raised. + + Another unusual aspect of the libgap 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. However, it is + possible that interrupting a function execution at an arbitrary + location will lead to inconsistent state. Internally, GAP provides + a mechanism using ``InterruptExecStat``, which sets a flag that + tells GAP to gracefully exit with an error as early as + possible. We make use of this internal mechanism to prevent + segmentation faults when GAP functions are interrupted. + + Specifically, we install GAP's own ``SIGINT`` handler (to catch + Ctrl-C) before executing any long-running GAP code, and then later + reinstall the original handler 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 set a + :exc:`KeyboardInterrupt` containing the phrase "user + interrupt". Eventually (as explained in the preceding paragraphs), + control will jump back to the Cython wrapper for ``GAP_Enter()``, + and this exception will be raised. + + 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: From 50157d7a974d532928738648d9c82d48fc5df0c2 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Wed, 20 Aug 2025 20:28:47 -0400 Subject: [PATCH 27/37] src/sage/libs/gap/libgap.pyx: more module docstring tweaks Include an example of "correct" interruptible libgap usage, and delete an example of what not to do. Bad examples can be dangerous for people who are skimming the documentation, and are not that interesting anyway (wrong code is easy to write). --- src/sage/libs/gap/libgap.pyx | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/sage/libs/gap/libgap.pyx b/src/sage/libs/gap/libgap.pyx index 06f130ec882..e71d0ab8e78 100644 --- a/src/sage/libs/gap/libgap.pyx +++ b/src/sage/libs/gap/libgap.pyx @@ -172,15 +172,9 @@ Using the GAP C library from Cython role in handling errors. The return value from ``GAP_Enter()`` is non-zero (success) the first time around, and if an error occurs, execution "jumps" back to ``GAP_Enter()``, this time with a return - value of zero (failure). Due to these implementation details, we - cannot simply check the return value before executing Cython code; - the following *does not* work:: - - if (GAP_Enter()): - # further calls to libgap - GAP_Leave() - - Instead you will see something like:: + value of zero (failure). Due to these quirks, naive attempts to + handle the return value of ``GAP_Enter()`` are doomed to fail. + The correct pattern to use is, try: GAP_Enter() @@ -197,11 +191,11 @@ Using the GAP C library from Cython ``GAP_Enter()``. It is at this point that we need to raise the (already set) exception, to prevent re-executing the code that caused an error. To facilitate this, ``GAP_Enter()`` is wrapped by - Cython, and the wrapper is qualified with ``except 0``. This means - that Cython will treat a return value from the ``GAP_Enter()`` - macro as an error, and raise an exception if one is set. (One will - be if there was an error because our ``error_handler()`` sets - it). Here is a real example:: + Cython, and the wrapper is qualified with ``except 0``. This tells + Cython to treat a return value of zero as an error, and raise an + exception if an exception is set. (One will be set if there was an + error because our ``error_handler()`` sets it). Here is a real + example:: cpdef void crash_and_burn() except *: cdef GapElement x = libgap({'a': 1, 'b': 2}) @@ -259,8 +253,18 @@ Using the GAP C library from Cython control will jump back to the Cython wrapper for ``GAP_Enter()``, and this exception will be raised. - Before you attempt to change any of this, please make sure that you - understand the issues that it is intended to fix, e.g. + The safest pattern to use for interruptible libgap code is, + + try: + gap_sig_on() + GAP_Enter() + # further calls to libgap + finally: + GAP_Leave() + gap_sig_off() + + 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 From b550b36b4c7fb2018f6ee1293a26948a56ab0bf6 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Thu, 21 Aug 2025 00:01:02 -0400 Subject: [PATCH 28/37] src/sage/libs/gap/util.pyx: add missing noexcept annotation --- src/sage/libs/gap/util.pyx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sage/libs/gap/util.pyx b/src/sage/libs/gap/util.pyx index ea889a62abb..f23879040b0 100644 --- a/src/sage/libs/gap/util.pyx +++ b/src/sage/libs/gap/util.pyx @@ -186,7 +186,7 @@ cdef sigaction_t gap_sigint_sa cdef sigaction_t sage_sigint_sa cdef sigaction_t sage_sigalrm_sa -cdef void gap_interrupt_asap(int signum): +cdef void gap_interrupt_asap(int signum) noexcept: # A wrapper around InterruptExecStat(). This tells GAP to raise an # error at the next opportunity. InterruptExecStat() From 677c2f103cb5a73ca0400bbae79a81d8ad1bbd0f Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Thu, 21 Aug 2025 01:13:29 -0400 Subject: [PATCH 29/37] src/sage/libs/gap/element.pyx: more tolerance in Ctrl-C tests The check for _pow_ being interruptible by Ctrl-C is subject to some variation due to CPU usage, scheduling, etc. Here we increase the tolerance to 5s, which should be more than enough time to interrupt it, and also increase the size of the problem so that it runs for at least 5s even on fast machines. --- src/sage/libs/gap/element.pyx | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index f210a0dc6a1..7cfefc9562e 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -1189,11 +1189,21 @@ cdef class GapElement(RingElement): TESTS: - Check that this can be interrupted gracefully:: - - sage: a, b = libgap.GL(1000, 3).GeneratorsOfGroup(); g = a * b + Check that this method can be interrupted gracefully. The + repeats build confidence that everything remains sane after an + interruption:: + + sage: # long time + sage: a, b = libgap.GL(1000, 3).GeneratorsOfGroup() + sage: g = a * b + sage: e = libgap(2^400000) sage: from sage.doctest.util import ensure_interruptible_after - sage: with ensure_interruptible_after(0.5): g ^ (2 ^ 10000) + sage: for _ in range(10): + ....: with ensure_interruptible_after(0.1, max_wait_after_interrupt=5): + ....: g._pow_(e) + + Check that a :exc:`sage.libs.gap.util.GAPError` is raised + under error conditions:: sage: libgap.CyclicGroup(2) ^ 2 Traceback (most recent call last): @@ -1201,6 +1211,8 @@ cdef class GapElement(RingElement): GAPError: Error, no method found! Error, no 1st choice method found for `^' on 2 arguments + :: + sage: libgap(3) ^ Infinity Traceback (most recent call last): ... From 1af02c2ddaed237c30ccae145e29692934c226b3 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Thu, 21 Aug 2025 10:51:34 -0400 Subject: [PATCH 30/37] src/sage/libs/gap: special case for bool in make_any_gap_element() We can check for a GAP boolean using only libgap-api.h by testing for one of the three objects: GAP_True, GAP_False, GAP_Fail. --- src/sage/libs/gap/element.pyx | 6 +++--- src/sage/libs/gap/gap_includes.pxd | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index 7cfefc9562e..ce3d8e1d19c 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -309,6 +309,8 @@ cdef GapElement make_any_gap_element(parent, Obj obj): """ if obj is NULL: return make_GapElement(parent, obj) + if (obj is GAP_True) or (obj is GAP_False) or (obj is GAP_Fail): + return make_GapElement_Boolean(parent, obj) # The object's type, if it can be determined from the libgap API. # Defaults to "undefined." @@ -370,9 +372,7 @@ cdef GapElement make_any_gap_element(parent, Obj obj): # Since TNUM_OBJ does not invoke the public libgap API, it is not # wrapped in GAP_Enter / GAP_Leave. cdef int num = TNUM_OBJ(obj) - if num == T_BOOL: - return make_GapElement_Boolean(parent, obj) - elif num == T_RAT: + if num == T_RAT: return make_GapElement_Rational(parent, obj) elif num == T_FUNCTION: return make_GapElement_Function(parent, obj) diff --git a/src/sage/libs/gap/gap_includes.pxd b/src/sage/libs/gap/gap_includes.pxd index ca12f936281..e3b05d88ba6 100644 --- a/src/sage/libs/gap/gap_includes.pxd +++ b/src/sage/libs/gap/gap_includes.pxd @@ -55,6 +55,7 @@ cdef extern from "gap/libgap-api.h" nogil: cdef Obj GAP_True cdef Obj GAP_False + cdef Obj GAP_Fail Obj GAP_CallFuncList(Obj func, Obj args); Obj GAP_CallFuncArray(Obj func, UInt narg, Obj * args); From f39be09cdee2640cd60709a8f4a99726bf962a88 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Thu, 21 Aug 2025 10:55:37 -0400 Subject: [PATCH 31/37] src/sage/libs/gap/element.pyx: update make_any_gap_element() comment We should mention that TNUM_OBJ is safe to use outside of GAP_Enter / GAP_Leave, and that we are only using it because the alternative is too slow. --- src/sage/libs/gap/element.pyx | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index ce3d8e1d19c..4b3a7bcc854 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -369,8 +369,12 @@ cdef GapElement make_any_gap_element(parent, Obj obj): return make_GapElement_Record(parent, obj) # Plan B: Use the "private" API to determine the type of ``obj``. - # Since TNUM_OBJ does not invoke the public libgap API, it is not - # wrapped in GAP_Enter / GAP_Leave. + # This is not kosher, but there are no such functions in + # libgap-api.h, and this is much faster than using the + # corresponding IsFoo() functions on a generic GAPElement. + # TNUM_OBJ involves a series of macros, none of which do any + # memory management or error handling, so this is safe to do + # outside of GAP_Enter / GAP_Leave for now. cdef int num = TNUM_OBJ(obj) if num == T_RAT: return make_GapElement_Rational(parent, obj) @@ -384,7 +388,8 @@ cdef GapElement make_any_gap_element(parent, Obj obj): return make_GapElement_Permutation(parent, obj) # Plan C: Make a generic element, and then use GAP functions to - # check if it's an IntegerMod or a Ring. + # check if it's an IntegerMod or a Ring. This is our penultimate + # resort due to how slow it is. result = make_GapElement(parent, obj) if num == T_POSOBJ: if result.IsZmodnZObj(): From 36da2ca1f9450e822cb98fb06e2f50f9e12f712c Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Thu, 21 Aug 2025 11:18:44 -0400 Subject: [PATCH 32/37] src/sage/libs/gap/element.pyx: faster list length check We can use the official C API for the length of a list in make_any_gap_element() without wasting any time. --- src/sage/libs/gap/element.pyx | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index 4b3a7bcc854..44a9585fc85 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -316,6 +316,10 @@ cdef GapElement make_any_gap_element(parent, Obj obj): # Defaults to "undefined." cdef int obj_type = OBJ_TYPE_UND + # The length of obj, if it's a list. Needed to exclude the empty + # list from the string <=> list-of-char equivalence. + cdef unsigned int list_len + # Plan A: Determine the type of ``obj`` using the libgap API. # Make all libgap-api calls here, between GAP_Enter/GAP_Leave. # In particular we avoid any *other* functions that may invoke @@ -333,6 +337,7 @@ cdef GapElement make_any_gap_element(parent, Obj obj): obj_type = OBJ_TYPE_STR elif GAP_IsList(obj): obj_type = OBJ_TYPE_LST + list_len = GAP_LenList(obj) elif GAP_IsRecord(obj): obj_type = OBJ_TYPE_REC finally: @@ -351,7 +356,7 @@ cdef GapElement make_any_gap_element(parent, Obj obj): return make_GapElement_String(parent, obj) elif obj_type == OBJ_TYPE_LST: result = make_GapElement_List(parent, obj) - if result.IsString() and result.Length() > 0: + if list_len > 0 and result.IsString(): # IsString() thinks a list-of-char is a string, but # GAP_IsString() does not. For backwards compatibility we # convert list-of-char to an efficient string here, and From 3a6d006176bff55e800748cf73ad2b773b7629d8 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Thu, 21 Aug 2025 12:28:41 -0400 Subject: [PATCH 33/37] src/sage/libs/gap/libgap.pyx: slightly safer example In the GAP_Enter / GAP_Leave example, it's probably better to store the GapElement in a python variable (rather than a C variable), just in case python might decide to garbage collect it otherwise. --- src/sage/libs/gap/libgap.pyx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sage/libs/gap/libgap.pyx b/src/sage/libs/gap/libgap.pyx index e71d0ab8e78..119267e6e04 100644 --- a/src/sage/libs/gap/libgap.pyx +++ b/src/sage/libs/gap/libgap.pyx @@ -198,11 +198,11 @@ Using the GAP C library from Cython example:: cpdef void crash_and_burn() except *: - cdef GapElement x = libgap({'a': 1, 'b': 2}) + x = libgap({'a': 1, 'b': 2}) cdef unsigned int xlen try: GAP_Enter() - xlen = GAP_LenList(x.value) + xlen = GAP_LenList((x).value) finally: GAP_Leave() print(xlen) From 8b8c496bc1dc04db911e99bb7c2d2ce392f72695 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Thu, 21 Aug 2025 14:38:35 -0400 Subject: [PATCH 34/37] src/sage/rings/complex_arb.pyx: add some Ctrl-C tolerance to a doctest We have one doctest in this file that is perpetually failing on the CI because a CBF integral takes too long to interrupt. The computation is extremely long-running, though, and "too long" is on the order of deciseconds: everything is fine. This commit increases the wait tolerance from 0.2s to 5s. That should be long enough to avoid all random CPU scheduling issues, and is still much shorter than the time it would take for the integral to complete. --- src/sage/rings/complex_arb.pyx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sage/rings/complex_arb.pyx b/src/sage/rings/complex_arb.pyx index b6f3ed191e8..0cab5ba87e9 100644 --- a/src/sage/rings/complex_arb.pyx +++ b/src/sage/rings/complex_arb.pyx @@ -1187,8 +1187,8 @@ class ComplexBallField(UniqueRepresentation, sage.rings.abc.ComplexBallField): [0.4596976941318602825990633926 +/- ...e-29] sage: from sage.doctest.util import ensure_interruptible_after - sage: with ensure_interruptible_after(0.1): - ....: C = ComplexBallField(1000000) + sage: C = ComplexBallField(1000000) + sage: with ensure_interruptible_after(0.1, max_wait_after_interrupt=5): ....: C.integral(lambda x, _: x.cos() * x.sin(), 0, 1) """ cdef IntegrationContext ctx = IntegrationContext() From 6e7419224907995c0c6cd73075a9a79ee20cb85b Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Sat, 23 Aug 2025 10:37:25 -0400 Subject: [PATCH 35/37] src/sage/libs/gap/libgap.pyx: unindent the "Using the .. library" docs These never should have been indented in the first place; the whole block is being rendered as a quote. --- src/sage/libs/gap/libgap.pyx | 211 +++++++++++++++++------------------ 1 file changed, 104 insertions(+), 107 deletions(-) diff --git a/src/sage/libs/gap/libgap.pyx b/src/sage/libs/gap/libgap.pyx index 119267e6e04..23d71c6ae99 100644 --- a/src/sage/libs/gap/libgap.pyx +++ b/src/sage/libs/gap/libgap.pyx @@ -160,117 +160,114 @@ using the recursive expansion of the Using the GAP C library from Cython =================================== - We are using the GAP API provided by the GAP project since - GAP 4.10. - - Calls to the GAP C library (functions declared in libgap-api.h) - should be sandwiched between calls to ``GAP_Enter()`` and - ``GAP_Leave()``. These are macros defined in ``libgap-api.h`` and - must be used carefully because ``GAP_Enter()`` is defined as two - function calls in succession without braces. The first thing that - ``GAP_Enter()`` does is a ``setjmp()`` which plays an important - role in handling errors. The return value from ``GAP_Enter()`` is - non-zero (success) the first time around, and if an error occurs, - execution "jumps" back to ``GAP_Enter()``, this time with a return - value of zero (failure). Due to these quirks, naive attempts to - handle the return value of ``GAP_Enter()`` are doomed to fail. - The correct pattern to use is, - - try: - GAP_Enter() - # further calls to libgap - finally: - GAP_Leave() - - How this works is subtle. When GAP is initialized, we install an - ``error_handler()`` callback that GAP invokes on error. This - function sets a python exception using ``PyErr_Restore()``, but so - long as we remain in C, this exception will not actually be - raised. When ``error_handler()`` finishes executing, control - returns to GAP which then jumps back to the previous - ``GAP_Enter()``. It is at this point that we need to raise the - (already set) exception, to prevent re-executing the code that - caused an error. To facilitate this, ``GAP_Enter()`` is wrapped by - Cython, and the wrapper is qualified with ``except 0``. This tells - Cython to treat a return value of zero as an error, and raise an - exception if an exception is set. (One will be set if there was an - error because our ``error_handler()`` sets it). Here is a real - example:: - - cpdef void crash_and_burn() except *: - x = libgap({'a': 1, 'b': 2}) - cdef unsigned int xlen - try: - GAP_Enter() - xlen = GAP_LenList((x).value) - finally: - GAP_Leave() - print(xlen) - - The call to ``GAP_LenList()`` is an error in this case, because - ``x.value`` is a GAP record, not a GAP list. In any case, what - happens is, - - #. We call the ``GAP_Enter()`` Cython wrapper, which invokes the - macro, and additionally generates some C code to raise an - exception if that return value is zero (error). But this is the - first pass, so for now the macro returns a non-zero (success) - value. - #. We call ``GAP_LenList(x.value)``, which is an error. - #. GAP invokes our ``error_handler()``, which creates a - :exc:`sage.libs.gap.util.GAPError`, and sets it active. - #. Control returns to GAP. - #. GAP jumps back to ``GAP_Enter()``. - #. The error branch of ``GAP_Enter()`` is executed. In other words - we proceed from ``GAP_Enter()`` as if it returned zero (error). - #. An exception is raised, because the ``except 0`` qualifier on the - Cython wrapper for ``GAP_Enter()`` specifically checks for zero - and raises any exceptions in that case. - #. Finally, ``GAP_Leave()`` is called to clean up. In a more - realistic example where failure is not guaranteed, this would - also have been run to clean up if no errors were raised. - - Another unusual aspect of the libgap 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. However, it is - possible that interrupting a function execution at an arbitrary - location will lead to inconsistent state. Internally, GAP provides - a mechanism using ``InterruptExecStat``, which sets a flag that - tells GAP to gracefully exit with an error as early as - possible. We make use of this internal mechanism to prevent - segmentation faults when GAP functions are interrupted. - - Specifically, we install GAP's own ``SIGINT`` handler (to catch - Ctrl-C) before executing any long-running GAP code, and then later - reinstall the original handler 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 set a - :exc:`KeyboardInterrupt` containing the phrase "user - interrupt". Eventually (as explained in the preceding paragraphs), - control will jump back to the Cython wrapper for ``GAP_Enter()``, - and this exception will be raised. - - The safest pattern to use for interruptible libgap code is, - +We are using the GAP API provided by the GAP project since GAP 4.10. + +Calls to the GAP C library (functions declared in libgap-api.h) should +be sandwiched between calls to ``GAP_Enter()`` and +``GAP_Leave()``. These are macros defined in ``libgap-api.h`` and must +be used carefully because ``GAP_Enter()`` is defined as two function +calls in succession without braces. The first thing that +``GAP_Enter()`` does is a ``setjmp()`` which plays an important role +in handling errors. The return value from ``GAP_Enter()`` is non-zero +(success) the first time around, and if an error occurs, execution +"jumps" back to ``GAP_Enter()``, this time with a return value of zero +(failure). Due to these quirks, naive attempts to handle the return +value of ``GAP_Enter()`` are doomed to fail. The correct pattern to +use is:: + + try: + GAP_Enter() + # further calls to libgap + finally: + GAP_Leave() + +How this works is subtle. When GAP is initialized, we install an +``error_handler()`` callback that GAP invokes on error. This function +sets a python exception using ``PyErr_Restore()``, but so long as we +remain in C, this exception will not actually be raised. When +``error_handler()`` finishes executing, control returns to GAP which +then jumps back to the previous ``GAP_Enter()``. It is at this point +that we need to raise the (already set) exception, to prevent +re-executing the code that caused an error. To facilitate this, +``GAP_Enter()`` is wrapped by Cython, and the wrapper is qualified +with ``except 0``. This tells Cython to treat a return value of zero +as an error, and raise an exception if an exception is set. (One will +be set if there was an error because our ``error_handler()`` sets +it). Here is a real example:: + + cpdef void crash_and_burn() except *: + x = libgap({'a': 1, 'b': 2}) + cdef unsigned int xlen try: - gap_sig_on() GAP_Enter() - # further calls to libgap + xlen = GAP_LenList((x).value) finally: GAP_Leave() - gap_sig_off() - - 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 + print(xlen) + +The call to ``GAP_LenList()`` is an error in this case, because +``x.value`` is a GAP record, not a GAP list. In any case, what happens +is, + +#. We call the ``GAP_Enter()`` Cython wrapper, which invokes the + macro, and additionally generates some C code to raise an + exception if that return value is zero (error). But this is the + first pass, so for now the macro returns a non-zero (success) + value. +#. We call ``GAP_LenList(x.value)``, which is an error. +#. GAP invokes our ``error_handler()``, which creates a + :exc:`sage.libs.gap.util.GAPError`, and sets it active. +#. Control returns to GAP. +#. GAP jumps back to ``GAP_Enter()``. +#. The error branch of ``GAP_Enter()`` is executed. In other words + we proceed from ``GAP_Enter()`` as if it returned zero (error). +#. An exception is raised, because the ``except 0`` qualifier on the + Cython wrapper for ``GAP_Enter()`` specifically checks for zero + and raises any exceptions in that case. +#. Finally, ``GAP_Leave()`` is called to clean up. In a more + realistic example where failure is not guaranteed, this would + also have been run to clean up if no errors were raised. + +Another unusual aspect of the libgap 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. However, it is +possible that interrupting a function execution at an arbitrary +location will lead to inconsistent state. Internally, GAP provides a +mechanism using ``InterruptExecStat``, which sets a flag that tells +GAP to gracefully exit with an error as early as possible. We make use +of this internal mechanism to prevent segmentation faults when GAP +functions are interrupted. + +Specifically, we install GAP's own ``SIGINT`` handler (to catch +Ctrl-C) before executing any long-running GAP code, and then later +reinstall the original handler 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 set a :exc:`KeyboardInterrupt` containing the phrase "user +interrupt". Eventually (as explained in the preceding paragraphs), +control will jump back to the Cython wrapper for ``GAP_Enter()``, and +this exception will be raised. + +The safest pattern to use for interruptible libgap code is:: + + try: + gap_sig_on() + GAP_Enter() + # further calls to libgap + finally: + GAP_Leave() + gap_sig_off() + +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: From 47826c2792b328cc13ee3ef51f82e673209bb4ff Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Sat, 23 Aug 2025 10:39:14 -0400 Subject: [PATCH 36/37] src/sage/libs/gap/libgap.pyx: unindent the AUTHORS block This is being rendered as a quote. --- src/sage/libs/gap/libgap.pyx | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/sage/libs/gap/libgap.pyx b/src/sage/libs/gap/libgap.pyx index 23d71c6ae99..4d11b919634 100644 --- a/src/sage/libs/gap/libgap.pyx +++ b/src/sage/libs/gap/libgap.pyx @@ -271,13 +271,13 @@ you understand the issues that it is intended to fix, e.g. AUTHORS: - - William Stein, Robert Miller (2009-06-23): first version - - Volker Braun, Dmitrii Pasechnik, Ivan Andrus (2011-03-25, Sage Days 29): - almost complete rewrite; first usable version. - - Volker Braun (2012-08-28, GAP/Singular workshop): update to - gap-4.5.5, make it ready for public consumption. - - Dima Pasechnik (2018-09-18, GAP Days): started the port to native - libgap API +- William Stein, Robert Miller (2009-06-23): first version +- Volker Braun, Dmitrii Pasechnik, Ivan Andrus (2011-03-25, Sage Days 29): + almost complete rewrite; first usable version. +- Volker Braun (2012-08-28, GAP/Singular workshop): update to + gap-4.5.5, make it ready for public consumption. +- Dima Pasechnik (2018-09-18, GAP Days): started the port to native + libgap API """ ############################################################################### From c1adeb906efc4618c76682963ca26e2e29207710 Mon Sep 17 00:00:00 2001 From: Michael Orlitzky Date: Sat, 23 Aug 2025 12:53:28 -0400 Subject: [PATCH 37/37] src/sage/libs/gap/libgap.pyx: backticks for ``libgap-api.h`` --- src/sage/libs/gap/libgap.pyx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sage/libs/gap/libgap.pyx b/src/sage/libs/gap/libgap.pyx index 4d11b919634..bcf94a65f27 100644 --- a/src/sage/libs/gap/libgap.pyx +++ b/src/sage/libs/gap/libgap.pyx @@ -162,8 +162,8 @@ Using the GAP C library from Cython We are using the GAP API provided by the GAP project since GAP 4.10. -Calls to the GAP C library (functions declared in libgap-api.h) should -be sandwiched between calls to ``GAP_Enter()`` and +Calls to the GAP C library (functions declared in ``libgap-api.h``) +should be sandwiched between calls to ``GAP_Enter()`` and ``GAP_Leave()``. These are macros defined in ``libgap-api.h`` and must be used carefully because ``GAP_Enter()`` is defined as two function calls in succession without braces. The first thing that