Skip to content

Commit

Permalink
gh-35114: libgap: Remove some GC hazards
Browse files Browse the repository at this point in the history
    
### πŸ“š Description

Trac branch `u/gh-collares/gap-gc` from #34701, now migrated to GitHub.
Currently based atop #35093; will rebase once that is merged.

The rest of the description below is copied from #34701:

A refactor in #27946 introduced "unprotected" (not surrounded by
`GAP_Enter`/`GAP_Leave`) `GAP_ValueGlobalVariable` calls. I believe this
might be a GC hazard, because after updating to GAP 4.12.1 I started
seeing aarch64 crashes on NixOS infrastructure such as:

```
#0  0x0000fffff79740e8 in wait4 ()
#1  0x0000fffff5dc6b78 in print_enhanced_backtrace ()
#2  0x0000fffff5dc8190 in sigdie ()
#3  0x0000fffff5dcb1c0 in cysigs_signal_handler ()
#4  0x0000fffff7ffb7cc in __kernel_rt_sigreturn ()
#5  0x0000ffff99a0bf28 in ConvString ()
#6  0x0000000000000000 in ?? ()
#7  0x0000000000000000 in ?? ()
#8  0x0000000000000000 in ?? ()
#9  0x0000ffff99989930 in Pr ()
#10 0x0000ffff9998aa18 in CloseOutput ()
#11 0x0000ffff99884828 in capture_stdout () at /build/sage-
src-9.7/pkgs/sagemath-standard/sage/libs/gap/element.pyx:154
...
```
I also see cases where `capture_stdout` throws errors such as
`sage.libs.gap.util.GAPError: Error, Length: <list> must be a list (not
the integer 255)` and then crashes. Both types of errors are fixed by
this ticket.

Note that I am nesting `GAP_Enter`/`GAP_Leave` calls because I didn't
remove the preexisting calls inside `capture_stdout`. That's because I
feared removing the innermost calls might create a new footgun (and I
believe nested `GAP_Enter`/`GAP_Leave` calls are explicitly supported),
but removing them should cause no problem. Removing them might even be
preferable for performance reasons, I don't know.

Fixes #34701

### πŸ“ Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->

- [x] I have made sure that the title is self-explanatory and the
description concisely explains the PR.
- [x] I have linked an issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### βŒ› Dependencies
<!-- List all open pull requests that this PR logically depends on -->
<!--
- #xyz: short description why this is a dependency
- #abc: ...
-->
- #35093: GAP 4.12.2 upgrade, which touches the same function and should
land first.
    
URL: #35114
Reported by: Mauricio Collares
Reviewer(s): Dima Pasechnik
  • Loading branch information
Release Manager committed Mar 24, 2023
2 parents f18723d + 5f9d256 commit 0d01c38
Showing 1 changed file with 14 additions and 5 deletions.
19 changes: 14 additions & 5 deletions src/sage/libs/gap/element.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,13 @@ cdef char *gap_element_repr(Obj obj):
GAP on the command-line (i.e. when evaluating an expression that returns
that object.
"""

cdef Obj func = GAP_ValueGlobalVariable("ViewObj")
return capture_stdout(func, obj)
cdef Obj func
try:
GAP_Enter()
func = GAP_ValueGlobalVariable("ViewObj")
return capture_stdout(func, obj)
finally:
GAP_Leave()


cdef char *gap_element_str(Obj obj):
Expand All @@ -181,8 +185,13 @@ cdef char *gap_element_str(Obj obj):
slightly different approach more closely mirroring Python's str/repr
difference (though this does not map perfectly onto GAP).
"""
cdef Obj func = GAP_ValueGlobalVariable("Print")
return capture_stdout(func, obj)
cdef Obj func
try:
GAP_Enter()
func = GAP_ValueGlobalVariable("Print")
return capture_stdout(func, obj)
finally:
GAP_Leave()


cdef Obj make_gap_record(sage_dict) except NULL:
Expand Down

0 comments on commit 0d01c38

Please sign in to comment.