builtin,coroutines,cgen: fix using coroutines with boehm GC, by using…

… a stack pointer corrector (#20771)

vlib/builtin/builtin_d_gcboehm.c.v

@@ -103,6 +103,7 @@ $if gcboehm_leak ? {
 }
 
 #include <gc.h>
+// #include <gc/gc_mark.h>
 
 // replacements for `malloc()/calloc()`, `realloc()` and `free()`
 // for use with Boehm-GC
@@ -148,11 +149,19 @@ pub struct C.GC_stack_base {
 	// reg_base voidptr
 }
 
-// pub struct C.GC_stack_base{}
-
 fn C.GC_get_stack_base(voidptr)
 fn C.GC_register_my_thread(voidptr) int
 fn C.GC_unregister_my_thread() int
 
+// fn C.GC_get_my_stackbottom(voidptr) voidptr
+// fn C.GC_set_stackbottom(voidptr, voidptr)
+// fn C.GC_push_all_stacks()
+
 fn C.GC_add_roots(voidptr, voidptr)
 fn C.GC_remove_roots(voidptr, voidptr)
+
+// fn C.GC_get_push_other_roots() fn()
+// fn C.GC_set_push_other_roots(fn())
+
+fn C.GC_get_sp_corrector() fn (voidptr, voidptr)
+fn C.GC_set_sp_corrector(fn (voidptr, voidptr))

vlib/coroutines/coroutines.c.v

@@ -8,53 +8,62 @@ import time
 
 #flag -I @VEXEROOT/thirdparty/photon
 #flag @VEXEROOT/thirdparty/photon/photonwrapper.so
-
 #include "photonwrapper.h"
 
+$if windows {
+	#include "processthreadsapi.h"
+} $else {
+	#include <pthread.h>
+}
+#flag -I @VEXEROOT/vlib/coroutines
+#include "sp_corrector.c"
+
 fn C.set_photon_thread_stack_allocator(fn (voidptr, int) voidptr, fn (voidptr, voidptr, int))
-fn C.default_photon_thread_stack_alloc(voidptr, int) voidptr
-fn C.default_photon_thread_stack_dealloc(voidptr, voidptr, int)
+
+// fn C.default_photon_thread_stack_alloc(voidptr, int) voidptr
+// fn C.default_photon_thread_stack_dealloc(voidptr, voidptr, int)
 fn C.new_photon_work_pool(int) voidptr
 fn C.delete_photon_work_pool()
 fn C.init_photon_work_pool(int)
-fn C.init_photon_manual(int, fn ())
-fn C.init_photon_manual2(fn (), fn ())
 fn C.photon_thread_create_and_migrate_to_work_pool(f voidptr, arg voidptr)
 fn C.photon_thread_create(f voidptr, arg voidptr)
 fn C.photon_thread_migrate()
 
 // fn C.photon_thread_migrate(work_pool voidptr)
 fn C.photon_init_default() int
-
 fn C.photon_sleep_s(n int)
 fn C.photon_sleep_ms(n int)
 
+fn C.sp_corrector(voidptr, voidptr)
+
 // sleep is coroutine-safe version of time.sleep()
 pub fn sleep(duration time.Duration) {
 	C.photon_sleep_ms(duration.milliseconds())
 }
 
 fn alloc(_ voidptr, stack_size int) voidptr {
-	// println('## alloc called')
 	unsafe {
-		// thread_id := C.pthread_self()
-		// println('## Thread ID: $thread_id')
-
-		// $if gcboehm ? {
-		// 	mut sb := C.GC_stack_base{}
-		// 	C.GC_get_stack_base(&sb)
-		// 	C.GC_register_my_thread(&sb)
-		// 	// res = C.GC_register_my_thread(&sb)
-		// 	// println('## RES: $res')
-		// }
-
-		// NOTE: when using malloc (GC_MALLOC) we get a segfault
-		// when migrating from a new thread to a work pool thread
-		// stack_ptr := malloc(stack_size)
-		// stack_ptr := C.malloc(stack_size)
-		stack_ptr := C.default_photon_thread_stack_alloc(nil, stack_size)
+		$if gcboehm ? {
+			// TODO: do this only once when the worker thread is created.
+			// I'm currently just doing it here for convenience.
+			// The second time `C.GC_register_my_thread` gets called from the
+			// same thread it will just fail (returning non 0), so it't not
+			// really a problem, it's just not ideal.
+			mut sb := C.GC_stack_base{}
+			C.GC_get_stack_base(&sb)
+			C.GC_register_my_thread(&sb)
+		}
+
+		stack_ptr := malloc(stack_size)
 
 		$if gcboehm ? {
+			// TODO: this wont work if a corouroutine gets moved to a different
+			// thread, so we are using `C.GC_set_sp_corrector` with our own
+			// corrector function which seems to be the best solution for now.
+			// It would probably be more performant if we could hook into photon's context
+			// switching code (currently not possible) or we write our own implementation.
+			// C.GC_set_stackbottom(0, stack_ptr)
+
 			C.GC_add_roots(stack_ptr, charptr(stack_ptr) + stack_size)
 		}
 
@@ -63,21 +72,32 @@ fn alloc(_ voidptr, stack_size int) voidptr {
 }
 
 fn dealloc(_ voidptr, stack_ptr voidptr, stack_size int) {
-	// println('## dealloc called')
 	unsafe {
 		$if gcboehm ? {
+			// TODO: only do this once when the worker thread is killed (not in each coroutine)
+			// come up with a solution for this and `C.GC_register_my_thread` (see alloc above)
 			// C.GC_unregister_my_thread()
+
 			C.GC_remove_roots(stack_ptr, charptr(stack_ptr) + stack_size)
 		}
-		// free(stack_ptr)
-		C.default_photon_thread_stack_dealloc(nil, stack_ptr, stack_size)
+		free(stack_ptr)
 	}
 }
 
 fn init() {
+	// NOTE `sp_corrector` only works for platforms with the stack growing down
+	// MacOs, Win32 and Linux always have stack growing down.
+	// A proper solution is planned (hopefully) for boehm v8.4.0.
+	C.GC_set_sp_corrector(C.sp_corrector)
+	if C.GC_get_sp_corrector() == unsafe { nil } {
+		panic('stack pointer correction unsupported')
+	}
 	C.set_photon_thread_stack_allocator(alloc, dealloc)
 	ret := C.photon_init_default()
 
+	// NOTE: instead of using photon's WorkPool, we could start our own worker threads,
+	// then initialize photon in each of them. If we did that we would need to control
+	// the migration of coroutines across threads etc.
 	if util.nr_jobs >= 1 {
 		C.init_photon_work_pool(util.nr_jobs)
 	}

vlib/coroutines/sp_corrector.c

@@ -0,0 +1,32 @@
+// TODO: convert this to v code? handle all platforms
+// currently if it's not macos or win it defaults to the linux impl
+
+static void sp_corrector(void** sp_ptr, void* tid) {
+    size_t stack_size;
+    char* stack_addr;
+#ifdef __APPLE__
+    stack_size = pthread_get_stacksize_np((pthread_t)tid);
+    stack_addr = (char*) pthread_get_stackaddr_np((pthread_t)tid);
+#elif defined(_WIN64)
+    ULONG_PTR stack_low, stack_high;
+    GetCurrentThreadStackLimits(&stack_low, &stack_high);
+    stack_size = stack_high - stack_low;
+    stack_addr = (char*)stack_low;
+// #elif defined(__linux__)
+//     pthread_attr_t gattr;
+//     pthread_getattr_np((pthread_t)tid, &gattr);
+//     pthread_attr_getstack(&gattr, (void**)&stack_addr, &stack_size);
+//     pthread_attr_destroy(&gattr);
+// #else
+//     assert("unsupported platform");
+#else
+    pthread_attr_t gattr;
+    pthread_getattr_np((pthread_t)tid, &gattr);
+    pthread_attr_getstack(&gattr, (void**)&stack_addr, &stack_size);
+    pthread_attr_destroy(&gattr);
+#endif
+    char sp = (char)*sp_ptr;
+    if(sp <= stack_addr || sp >= stack_addr+stack_size) {
+        *sp_ptr = (void*)stack_addr;
+    }
+}

vlib/v/gen/c/cmain.v

@@ -110,8 +110,9 @@ fn (mut g Gen) gen_c_main_header() {
 			g.writeln('\tGC_set_find_leak(1);')
 		}
 		g.writeln('\tGC_set_pages_executable(0);')
-		// NOTE: required when using GC_MALLOC & GC_register_my_thread
-		// g.writeln('\tGC_allow_register_threads();')
+		if g.pref.use_coroutines {
+			g.writeln('\tGC_allow_register_threads();')
+		}
 		g.writeln('\tGC_INIT();')
 
 		if g.pref.gc_mode in [.boehm_incr, .boehm_incr_opt] {
@@ -247,8 +248,9 @@ pub fn (mut g Gen) gen_c_main_for_tests() {
 			g.writeln('\tGC_set_find_leak(1);')
 		}
 		g.writeln('\tGC_set_pages_executable(0);')
-		// NOTE: required when using GC_MALLOC & GC_register_my_thread
-		// g.writeln('\tGC_allow_register_threads();')
+		if g.pref.use_coroutines {
+			g.writeln('\tGC_allow_register_threads();')
+		}
 		g.writeln('\tGC_INIT();')
 		if g.pref.gc_mode in [.boehm_incr, .boehm_incr_opt] {
 			g.writeln('\tGC_enable_incremental();')