Skip to content

libgap: Remove some GC hazards#35114

Merged
vbraun merged 25 commits intosagemath:developfrom
collares:gap-gc
Mar 26, 2023
Merged

libgap: Remove some GC hazards#35114
vbraun merged 25 commits intosagemath:developfrom
collares:gap-gc

Conversation

@collares
Copy link
Copy Markdown
Contributor

@collares collares commented Feb 13, 2023 •
edited by mkoeppe
Loading

📚 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

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

⌛ Dependencies

antonio-rojas and others added 23 commits February 12, 2023 16:30
@antonio-rojas @dimpase
Adapt to API changes in OpenOutputStream and CloseOutput
8b272d3
@antonio-rojas @dimpase
Disable colored prompt as it breaks the pexpect interface
1c8832f
@antonio-rojas @dimpase
Port NaturalHomomorphism uses
4b5ed43
@antonio-rojas @dimpase
Fix tests with GAP 4.12
b1a8121
@antonio-rojas @dimpase
Adapt test to new is_transitive and is_primitive behavior
b24d42e
@antonio-rojas @dimpase
Mark test as random. With gap 4.12 on x86_64 it is no longer 2
271a5ba
@antonio-rojas @dimpase
Remove test that is now redundant, all seeds give the same answer
8c8d756
@dimpase
install gap 4.12.0
21537e7
@dimpase
upgrade libsemigroup and gap_packages
da558e1 
adjust doctests
@dimpase
add GAP upstream PR 5077
821f9da
@dimpase
also, fix the package names etc
c5410a2
@dimpase
update GAP to 4.12.1
f30806e
@dimpase
install GAP package singular - new dependency of liepring
ca2d08d
@dimpase
moved install of radiroot to gap from gap_packages
41cf212
@dimpase
moved install of autodoc and utils to gap from gap_packages
2f552b0
@dimpase
don't use SAGE_LOCAL/share/gap, switch to GAP's "make install"
32c4a63 
also add a workaround for Semigroups (see comment in the code)
@antonio-rojas @dimpase
Search for files in both lib/gap and share/gap
85849a2 
`make install` installs files in both paths
@antonio-rojas @dimpase
Create lib/gap/bin on install
575c6b2 
The 'packagemanager' package does not load without it
@dimpase
ensure make gap-clean still works
3a73005
@dimpase
with SAGE_CHECK=yes, one needs io installed to prevent hangs
3c1d422
@dimpase
backport GAP PR 5235
b335ec9
@dimpase
update GAP to 4.12.2
5ed4447
@dimpase
bump libsemigroups to v2.3.2
c70ae12
@collares collares mentioned this pull request Feb 13, 2023
Closed
@collares collares changed the title libgap: Fix GC crashes on aarch64 libgap: Remove some GC hazards Feb 13, 2023
@tobiasdiez tobiasdiez mentioned this pull request Feb 15, 2023
Merged
5 tasks
dimpase
dimpase approved these changes Feb 16, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

lgtm

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 1, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (52a81cb) 88.57% compared to head (d458e35) 88.58%.

❗ Current head d458e35 differs from pull request most recent head 5f9d256. Consider uploading reports for the commit 5f9d256 to get more accurate results

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
Impacted Files Coverage Δ
src/sage/coding/linear_code.py 81.12% <ø> (ø)
src/sage/combinat/posets/posets.py 93.98% <ø> (+0.11%) ⬆️
...mbinat/root_system/hecke_algebra_representation.py 93.11% <ø> (ø)
src/sage/combinat/symmetric_group_algebra.py 94.35% <ø> (ø)
src/sage/groups/finitely_presented.py 95.88% <ø> (ø)
src/sage/groups/fqf_orthogonal.py 88.48% <ø> (ø)
src/sage/groups/matrix_gps/finitely_generated.py 90.50% <ø> (ø)
src/sage/groups/perm_gps/permgroup.py 90.56% <ø> (-0.49%) ⬇️
src/sage/tests/gap_packages.py 94.59% <ø> (ø)
src/sage/env.py 88.05% <100.00%> (+0.05%) ⬆️
... and 57 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 1, 2023

Documentation preview for this PR is ready! 🎉
Built with commit: 5f9d256

@collares
Copy link
Copy Markdown
Contributor Author

collares commented Mar 1, 2023

Rebased to match #35093. The CI test failure (log below) seems unrelated:


sage -t --random-seed=101871368592826382261112102728642599044 doc/de/a_tour_of_sage/index.rst
**********************************************************************
File "doc/de/a_tour_of_sage/index.rst", line 66, in doc.de.a_tour_of_sage.index
Failed example:
    integrate(sqrt(x)*sqrt(1+x), x)
Expected:
    1/4*((x + 1)^(3/2)/x^(3/2) + sqrt(x + 1)/sqrt(x))/((x + 1)^2/x^2 - 2*(x + 1)/x + 1) - 1/8*log(sqrt(x + 1)/sqrt(x) + 1) + 1/8*log(sqrt(x + 1)/sqrt(x) - 1)
Got:
    // Giac share root-directory:/sage/local/share/giac/
    // Giac share root-directory:/sage/local/share/giac/
    Added 0 synonyms
    1/4*(2*x - 3)*sqrt(x + 1)*sqrt(x) + sqrt(x + 1)*sqrt(x) + 1/4*log(sqrt(x + 1) - sqrt(x))
**********************************************************************
1 item had failures:
   1 of  32 in doc.de.a_tour_of_sage.index
    [22 tests, 1 failure, 3.30 s]

@dimpase dimpase self-requested a review March 1, 2023 21:49
dimpase
dimpase approved these changes Mar 1, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

lgtm

@vbraun vbraun merged commit 0d01c38 into sagemath:develop Mar 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@dimpase dimpase dimpase approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c: interfaces t: bug

Projects

None yet

Milestone

sage-10.0

Development

Successfully merging this pull request may close these issues.

libgap: Fix GC crashes on aarch64

7 participants

@collares @codecov-commenter @dimpase @vbraun @kwankyu @mkoeppe @antonio-rojas