[tobihan]

GAP_EnterStack() recorded __builtin_frame_address(0) as the stack bottom for GASMAN. On some architectures, such as 32-bit ARM in Thumb mode, this address lies below the local variables of the calling function, so GAP objects referenced only from them were not marked and could be freed while still in use, causing crashes and memory corruption in e.g. SageMath. Use the canonical frame address instead, which is the top of the stack frame.

Assisted-by: Claude Code (Opus 5.5)

Text for release notes

see title

Further details

In Debian we had problems with SageMath randomly crashing on armhf for years. Now Claude produced this patch that should fix the issue.

I ran the new test enterstack on a Debian armhf porterbox with and without the changes to libgap-api.h. With the changes it passed, without the changes I got

Running tst/testlibgap/enterstack ...
Error, Panic: basic object of type '<null>' is unkind
1d0
< 1 42

[tobihan]

With this patch I was also able to run SageMath's full test suite on the armhf porterbox without segfaults, which was not possible before.

[ChrisJefferson]

So, this definitely improves things, there is a real bug, and we want something to fix this problem. I have two issues:

(a) the function is broken in clang on windows, so this PR would break cygwin (and mingw if we support that in future). llvm/llvm-project#227606

(b) If we need this on certain CPUs, we shouldn't fall back to the old broken one when it's not available silently.

[ChrisJefferson]

One way to fix this, following systems like Boehm, would be to get the thread’s stack base using pthread_getattr_np() / pthread_attr_getstack(), or pthread_get_stackaddr_np() on macOS. There are a few platform details to get right. This would scan the whole active thread stack, rather than just the part containing GAP calls, but would avoid missing locals because of the compiler’s frame layout.

@fingolfin , can you remember if there is a reason we don’t do that?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.72%. Comparing base (a778edf) to head (cf69717).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6788   +/-   ##
=======================================
  Coverage   79.72%   79.72%           
=======================================
  Files         684      684           
  Lines      295592   295592           
  Branches     8660     8683   +23     
=======================================
+ Hits       235650   235651    +1     
+ Misses      58126    58124    -2     
- Partials     1816     1817    +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[fingolfin]

@ChrisJefferson most likely because we have no need for it? This interface was largely written by SageMath people and is used only by SageMath AFAIK, and improving it is largely up to them...

[ChrisJefferson]

Just wanted to check I hadn't forgotten (I also haven't paid much attention to this). I think a better option would be to use the functionality which lets you get the very top of the stack -- there are no disadvantages to over-scanning and the stack scanning doesn't take that long, and after some quick poking this still seemed to have issues, from what I could tell.