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 segfault with next boehm gc (after v8.2.4) #14130

Conversation

ysbaddaden
Copy link
Contributor

The next release of libgc segfaults while marking the stacks when compiling without MT. Replacing the direct access to GC_stackbottom with the thread-safe version (already used on windows) avoids the problem and doesn't seem to have any negative impact on performance.

The next release of libgc segfaults while marking the stacks when
compiling without MT. Replacing the direct access to `GC_stackbottom`
with the thread-safe version (already used on windows) avoids the
problem and doesn't seem to have any negative impact on performance.
@ysbaddaden ysbaddaden marked this pull request as draft December 21, 2023 17:25
@straight-shoota
Copy link
Member

straight-shoota commented Dec 21, 2023

The thread-safe functions are a relatively recent addition that we upstreamed to libgc. The use of $stackbottom is still required for older versions of libgc (< 8.2.0).

(That's the reason CI is failing on aarch64. We're using an old build image for that.)

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:runtime topic:multithreading labels Dec 21, 2023
@ysbaddaden
Copy link
Contributor Author

Weird, I don't have any segfault running the std_spec while this program will always segfault with the following backtrace (from gdb):

Program received signal SIGSEGV, Segmentation fault.
GC_mark_from (mark_stack_top=0x7efc1758f120, mark_stack=0x7efc1758f000, mark_stack_limit=0x7efc1759f000) at mark.c:843
843               PREFETCH(limit - PREF_DIST*CACHE_LINE_SIZE);
(gdb) bt
#0  GC_mark_from (mark_stack_top=0x7efc1758f120, mark_stack=0x7efc1758f000, mark_stack_limit=0x7efc1759f000) at mark.c:843
#1  0x00005570f1e98528 in GC_mark_some (cold_gc_frame=cold_gc_frame@entry=0x7efc16d7eaa0 "") at mark.c:485
#2  0x00005570f1e8f848 in GC_stopped_mark (stop_func=stop_func@entry=0x5570f1e8f620 <GC_never_stop_func>) at alloc.c:887
#3  0x00005570f1e909a8 in GC_try_to_collect_inner (stop_func=0x5570f1e8f620 <GC_never_stop_func>) at alloc.c:602
#4  0x00005570f1e9179d in GC_collect_or_expand (needed_blocks=needed_blocks@entry=9, flags=flags@entry=0, retry=0) at alloc.c:1685
#5  0x00005570f1e95d80 in GC_alloc_large (lb=<optimized out>, lb@entry=32784, k=k@entry=0, flags=flags@entry=0, align_m1=0) at malloc.c:66
#6  0x00005570f1e960d6 in GC_generic_malloc_aligned (lb=<optimized out>, lb@entry=32768, k=k@entry=0, flags=flags@entry=0, align_m1=<optimized out>, align_m1@entry=0) at malloc.c:271
#7  0x00005570f1e96394 in GC_malloc_kind_aligned_global (lb=32768, k=0, align_m1=0) at malloc.c:352
#8  0x00005570f1e252c6 in malloc_atomic () at /usr/src/src/gc/boehm.cr:139
#9  0x00005570f1e252be in malloc_atomic () at /usr/src/src/gc.cr:88
#10 0x00005570f1e5f0d1 in in_buffer () at /usr/src/src/io/buffered.cr:268
#11 0x00005570f1e5efb3 in fill_buffer () at /usr/src/src/io/buffered.cr:262
#12 0x00005570f1e5e37f in read () at /usr/src/src/io/buffered.cr:84
#13 0x00005570f1e5f5f5 in read_fully? () at /usr/src/src/io.cr:542
#14 0x00005570f1e5f52b in read_fully () at /usr/src/src/io.cr:525
#15 0x00005570f1e64c1b in decode () at /usr/src/src/io/byte_format.cr:123
#16 0x00005570f1e29c90 in from_io () at /usr/src/src/int.cr:781
#17 0x00005570f1e62fcf in read_bytes () at /usr/src/src/io.cr:932
#18 0x00005570f1e010a8 in -> () at /usr/src/src/crystal/system/unix/signal.cr:62
#19 0x00005570f1e4d1b9 in run () at /usr/src/src/fiber.cr:146
#20 0x00005570f1e00e06 in -> () at /usr/src/src/fiber.cr:98
#21 0x0000000000000000 in ?? ()

….0 only

Let's keep support for older GC releases (before 8.2.0), and
manipulating GC_stackbottom seems to be noticeably faster than the
thread explicit methods, so let's only enable it for 8.3.0
@ysbaddaden
Copy link
Contributor Author

I'm now using pkg-config (unless we're on Windows) to detect if we want to use the new API, and keep using legacy otherwise. I still force the new API for preview_mt and Windows.

I also fixed the library name for pkg-config which is bdw-gc.

src/gc/boehm.cr Outdated Show resolved Hide resolved
There doesn't seem to be any noticeable impact on performance, even with lots of allocations (fibers, channels) and fiber swapcontexts.

Co-authored-by: Johannes Müller <straightshoota@gmail.com>
@ysbaddaden
Copy link
Contributor Author

Note: this is another (tiny) step to a thread safe stdlib, and thus towards MT 😁

@ysbaddaden ysbaddaden marked this pull request as ready for review December 28, 2023 22:15
@straight-shoota straight-shoota added this to the 1.11.0 milestone Jan 2, 2024
@straight-shoota straight-shoota merged commit 382041c into crystal-lang:master Jan 3, 2024
56 checks passed
@ysbaddaden ysbaddaden deleted the fix/gc/segfault-with-current-gc-master-branch branch January 4, 2024 09:27
@straight-shoota straight-shoota changed the title Fix: segfault with next boehm gc (after v8.2.4) Fix segfault with next boehm gc (after v8.2.4) Jan 5, 2024
@crysbot
Copy link

crysbot commented Jun 26, 2024

This pull request has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/crystal-no-longer-runs-gc-in-parallel-threads/6892/6

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. topic:multithreading topic:stdlib:runtime
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants