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

Reserve stack space on non-main threads for crash recovery on Windows #14187

Conversation

HertzDevil
Copy link
Contributor

LibC.SetThreadStackGuarantee only works on the current thread. This PR extends it to other threads created explicitly or via -Dpreview_mt, so that stack overflows on them will print a stack trace properly:

def foo
  x = uninitialized UInt8[512]
  foo
end

Thread.new { foo }.join
Stack overflow (e.g., infinite or very deep recursion)
[0x7ff79edc1fb7] foo at C:\Users\nicet\crystal\crystal\usr\test.cr:28
[0x7ff79edc1fbc] foo at C:\Users\nicet\crystal\crystal\usr\test.cr:28 (14843 times)
[0x7ff79edc1fa9] -> at C:\Users\nicet\crystal\crystal\usr\test.cr:31
[0x7ff79ee365c3] start at C:\Users\nicet\crystal\crystal\src\crystal\system\thread.cr:116
[0x7ff79ee36957] thread_proc at C:\Users\nicet\crystal\crystal\src\crystal\system\win32\thread.cr:30
[0x7ff79edc1fc9] -> at C:\Users\nicet\crystal\crystal\src\crystal\system\win32\thread.cr:15
[0x7ff79ee75766] GC_wait_marker +454 in C:\Users\nicet\crystal\crystal\test.exe
[0x7ff79ee75a7a] GC_call_with_stack_base +26 in C:\Users\nicet\crystal\crystal\test.exe
[0x7ff79eeacfd6] thread_start<unsigned int (__cdecl*)(void *),1> at minkernel\crts\ucrt\src\appcrt\startup\thread.cpp:97
[0x7fff3525257d] BaseThreadInitThunk +29 in C:\WINDOWS\System32\KERNEL32.DLL
[0x7fff3640aa58] RtlUserThreadStart +40 in C:\WINDOWS\SYSTEM32\ntdll.dll

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:windows Windows support based on the MSVC toolchain topic:multithreading labels Jan 8, 2024
@beta-ziliani beta-ziliani added this to the 1.12.0 milestone Jan 8, 2024
@Fryguy
Copy link
Contributor

Fryguy commented Jan 9, 2024

For posterity, can you update the OP to show what the stack trace looked like before your change?

@HertzDevil
Copy link
Contributor Author

Stack overflow (e.g., infinite or very deep recursion)
Stack overflow (e.g., infinite or very deep recursion)

@straight-shoota straight-shoota changed the title Windows: Reserve stack space on non-main threads for crash recovery Reserve stack space on non-main threads for crash recovery on Windows Jan 12, 2024
@straight-shoota straight-shoota merged commit f5844b0 into crystal-lang:master Jan 12, 2024
56 checks passed
@Fryguy
Copy link
Contributor

Fryguy commented Jan 12, 2024

Ah thanks...I thought it was more complicated than that 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:windows Windows support based on the MSVC toolchain topic:multithreading topic:stdlib:concurrency
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants