-
-
Notifications
You must be signed in to change notification settings - Fork 479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
libgap: Remove some GC hazards #35114
Conversation
adjust doctests
also add a workaround for Semigroups (see comment in the code)
`make install` installs files in both paths
The 'packagemanager' package does not load without it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #35114 +/- ##
===========================================
+ Coverage 88.57% 88.58% +0.01%
===========================================
Files 2140 2140
Lines 397273 396964 -309
===========================================
- Hits 351891 351660 -231
+ Misses 45382 45304 -78
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Documentation preview for this PR is ready! 🎉 |
Rebased to match #35093. The CI test failure (log below) seems unrelated:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
📚 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:I also see cases where
capture_stdout
throws errors such assage.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 insidecapture_stdout
. That's because I feared removing the innermost calls might create a new footgun (and I believe nestedGAP_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
⌛ Dependencies