Skip to content
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

fix: correctly support stack overflows across platforms #982

Merged
merged 13 commits into from
May 22, 2024

Conversation

supervacuus
Copy link
Collaborator

@supervacuus supervacuus commented May 2, 2024

Currently, stack overflows on Windows die somewhere in the handler because we run out of stack space. This is the first attempt to fix the situation by reserving stack space before it is exhausted. This is an analog to sigaltstack() on Linux, but Windows only provides a "reserved amount" interface, and with that, you have fewer options to shoot yourself in the foot.

Fixes #987.
Fixes #977.

@supervacuus
Copy link
Collaborator Author

The macOS tests fail because AppleClang 13.0.0.13000029 seemingly ignores -fno-optimize-sibling-calls. However, locally with 15.0.0.15000309, the tests run successfully. I will add a corresponding @pytest.mark.skipif until we update our macOS runner images (which is a bigger change than I am willing to introduce here).

@supervacuus supervacuus force-pushed the fix/stack_overflows_on_windows_support branch from dee3a4a to 2261b0d Compare May 7, 2024 14:10
@supervacuus
Copy link
Collaborator Author

The macOS tests fail because AppleClang 13.0.0.13000029 seemingly ignores -fno-optimize-sibling-calls. However, locally with 15.0.0.15000309, the tests run successfully. I will add a corresponding @pytest.mark.skipif until we update our macOS runner images (which is a bigger change than I am willing to introduce here).

After rebasing master to fix the GitHub-runner issue, the same problem is apparent with clang from the older NDK.

@supervacuus supervacuus marked this pull request as ready for review May 7, 2024 16:20
@supervacuus
Copy link
Collaborator Author

supervacuus commented May 7, 2024

This has been quite a revealing exercise. That a runtime approach to the sigaltstack fixed the "Old NDK"-build means that the bionic-libc implementations differ between the NDK versions (or rather API system image versions) regarding whether a sigaltstack was installed for each thread. This doesn't necessarily mean this will also be true when these threads are started through the ART. Still, it is noteworthy, especially given the recent influx of issues with older devices.

This started as a Windows fix, but since I tried to make the stack overflow tests run on all builds, we also have improvements on Linux and Android. So, renaming the PR and additional change-log entries might be in order.

cc: @kahest

@supervacuus supervacuus changed the title fix: stack overflows on windows support fix: correctly support stack overflows across platforms May 22, 2024
Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

nice! 🎉

@supervacuus supervacuus merged commit 2bd129a into master May 22, 2024
19 checks passed
@supervacuus supervacuus deleted the fix/stack_overflows_on_windows_support branch May 22, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only set up sigaltstack if there is none (Linux + Android) Stack overflow not sent to backend
2 participants