diff options
author | KJ Tsanaktsidis <kj@kjtsanaktsidis.id.au> | 2023-11-12 13:24:55 +1100 |
---|---|---|
committer | KJ Tsanaktsidis <kj@kjtsanaktsidis.id.au> | 2024-01-19 09:55:12 +1100 |
commit | 807714447ef02c77bb0e17fe27d96ee2692264f8 (patch) | |
tree | 89ec73d9c7b67138d73e92b7b04150934e42af12 /thread_pthread.c | |
parent | 08edad31a6d5c9efd86bede1e942a32cff498427 (diff) |
Pass down "stack start" variables from closer to the top of the stack
This commit changes how stack extents are calculated for both the main thread and other threads. Ruby uses the address of a local variable as part of the calculation for machine stack extents: * pthreads uses it as a lower-bound on the start of the stack, because glibc (and maybe other libcs) can store its own data on the stack before calling into user code on thread creation. * win32 uses it as an argument to VirtualQuery, which gets the extent of the memory mapping which contains the variable However, the local being used for this is actually too low (too close to the leaf function call) in both the main thread case and the new thread case. In the main thread case, we have the `INIT_STACK` macro, which is used for pthreads to set the `native_main_thread->stack_start` value. This value is correctly captured at the very top level of the program (in main.c). However, this is _not_ what's used to set the execution context machine stack (`th->ec->machine_stack.stack_start`); that gets set as part of a call to `ruby_thread_init_stack` in `Init_BareVM`, using the address of a local variable allocated _inside_ `Init_BareVM`. This is too low; we need to use a local allocated closer to the top of the program. In the new thread case, the lolcal is allocated inside `native_thread_init_stack`, which is, again, too low. In both cases, this means that we might have VALUEs lying outside the bounds of `th->ec->machine.stack_{start,end}`, which won't be marked correctly by the GC machinery. To fix this, * In the main thread case: We already have `INIT_STACK` at the right level, so just pass that local var to `ruby_thread_init_stack`. * In the new thread case: Allocate the local one level above the call to `native_thread_init_stack` in `call_thread_start_func2`. [Bug #20001] fix
Diffstat (limited to 'thread_pthread.c')
-rw-r--r-- | thread_pthread.c | 32 |
1 files changed, 24 insertions, 8 deletions
diff --git a/thread_pthread.c b/thread_pthread.c index 6d2f55a957..58bc49b17c 100644 --- a/thread_pthread.c +++ b/thread_pthread.c @@ -1962,9 +1962,8 @@ reserve_stack(volatile char *limit, size_t size) # define reserve_stack(limit, size) ((void)(limit), (void)(size)) #endif -#undef ruby_init_stack -void -ruby_init_stack(volatile VALUE *addr) +static void +native_thread_init_main_thread_stack(void *addr) { native_main_thread.id = pthread_self(); @@ -1987,7 +1986,7 @@ ruby_init_stack(volatile VALUE *addr) if (!native_main_thread.stack_start || STACK_UPPER((VALUE *)(void *)&addr, native_main_thread.stack_start > addr, - native_main_thread.stack_start < addr)) { + native_main_thread.stack_start < (VALUE *)addr)) { native_main_thread.stack_start = (VALUE *)addr; } #endif @@ -2049,10 +2048,16 @@ ruby_init_stack(volatile VALUE *addr) {int err = (expr); if (err) {rb_bug_errno(#expr, err);}} static int -native_thread_init_stack(rb_thread_t *th) +native_thread_init_stack(rb_thread_t *th, void *local_in_parent_frame) { rb_nativethread_id_t curr = pthread_self(); + if (!native_main_thread.id) { + /* This thread is the first thread, must be the main thread - + * configure the native_main_thread object */ + native_thread_init_main_thread_stack(local_in_parent_frame); + } + if (pthread_equal(curr, native_main_thread.id)) { th->ec->machine.stack_start = native_main_thread.stack_start; th->ec->machine.stack_maxsize = native_main_thread.stack_maxsize; @@ -2064,8 +2069,8 @@ native_thread_init_stack(rb_thread_t *th) size_t size; if (get_stack(&start, &size) == 0) { - uintptr_t diff = (uintptr_t)start - (uintptr_t)&curr; - th->ec->machine.stack_start = (VALUE *)&curr; + uintptr_t diff = (uintptr_t)start - (uintptr_t)local_in_parent_frame; + th->ec->machine.stack_start = (uintptr_t)local_in_parent_frame; th->ec->machine.stack_maxsize = size - diff; } } @@ -2185,8 +2190,19 @@ native_thread_create_dedicated(rb_thread_t *th) static void call_thread_start_func_2(rb_thread_t *th) { - native_thread_init_stack(th); + /* Capture the address of a local in this stack frame to mark the beginning of the + machine stack for this thread. This is required even if we can tell the real + stack beginning from the pthread API in native_thread_init_stack, because + glibc stores some of its own data on the stack before calling into user code + on a new thread, and replacing that data on fiber-switch would break it (see + bug #13887) */ + VALUE stack_start = 0; + VALUE *stack_start_addr = &stack_start; + native_thread_init_stack(th, stack_start_addr); thread_start_func_2(th, th->ec->machine.stack_start); + + /* Ensure that stack_start really was spilled to the stack */ + RB_GC_GUARD(stack_start) } static void * |