diff --git a/ext/fiddle/closure.c b/ext/fiddle/closure.c index 5d936473..8670d79e 100644 --- a/ext/fiddle/closure.c +++ b/ext/fiddle/closure.c @@ -7,6 +7,7 @@ int ruby_thread_has_gvl_p(void); /* from internal.h */ VALUE cFiddleClosure; typedef struct { + VALUE self; void * code; ffi_closure *pcl; ffi_cif cif; @@ -54,12 +55,45 @@ closure_memsize(const void * ptr) return size; } +/* Ruby 2.6 and earlier have no compaction, so marking the reference plainly + * holds it in place there. */ +#ifndef HAVE_RB_GC_MARK_MOVABLE +# define rb_gc_mark_movable rb_gc_mark +#endif + +/* Fiddle::Closure#free and initialize_rescue set the data pointer to NULL. + * The garbage collector does not call these functions with a NULL pointer, + * but this file is the only one in the extension that clears the pointer, + * so both functions check it. */ +static void +closure_mark(void *ptr) +{ + fiddle_closure *closure = ptr; + if (closure) { + rb_gc_mark_movable(closure->self); + } +} + +#ifdef HAVE_RB_GC_MARK_MOVABLE +static void +closure_compact(void *ptr) +{ + fiddle_closure *closure = ptr; + if (closure) { + closure->self = rb_gc_location(closure->self); + } +} +#endif + const rb_data_type_t closure_data_type = { .wrap_struct_name = "fiddle/closure", .function = { - .dmark = 0, + .dmark = closure_mark, .dfree = dealloc, - .dsize = closure_memsize + .dsize = closure_memsize, +#ifdef HAVE_RB_GC_MARK_MOVABLE + .dcompact = closure_compact, +#endif }, .flags = FIDDLE_DEFAULT_TYPED_DATA_FLAGS, }; @@ -76,7 +110,7 @@ with_gvl_callback(void *ptr) { struct callback_args *x = ptr; - VALUE self = (VALUE)x->ctx; + VALUE self = ((fiddle_closure *)x->ctx)->self; VALUE rbargs = rb_iv_get(self, "@args"); VALUE ctype = rb_iv_get(self, "@ctype"); int argc = RARRAY_LENINT(rbargs); @@ -288,6 +322,10 @@ initialize_body(VALUE user_data) TypedData_Get_Struct(data->self, fiddle_closure, &closure_data_type, cl); + /* libffi receives the address of this struct, which xmalloc'd memory keeps + * stable. The trampoline reads cl->self, so the GC must mark and relocate it. */ + RB_OBJ_WRITE(data->self, &cl->self, data->self); + cl->argv = (ffi_type **)xcalloc(argc + 1, sizeof(ffi_type *)); normalized_args = rb_ary_new_capa(argc); @@ -318,9 +356,9 @@ initialize_body(VALUE user_data) #if USE_FFI_CLOSURE_ALLOC result = ffi_prep_closure_loc(pcl, cif, callback, - (void *)(data->self), cl->code); + (void *)cl, cl->code); #else - result = ffi_prep_closure(pcl, cif, callback, (void *)(data->self)); + result = ffi_prep_closure(pcl, cif, callback, (void *)cl); cl->code = (void *)pcl; i = mprotect(pcl, sizeof(*pcl), PROT_READ | PROT_EXEC); if (i) { diff --git a/ext/fiddle/extconf.rb b/ext/fiddle/extconf.rb index 34cd0c3d..e9fd2aa0 100644 --- a/ext/fiddle/extconf.rb +++ b/ext/fiddle/extconf.rb @@ -241,6 +241,7 @@ def enable_debug_build_flag(flags) end have_func("rb_str_to_interned_str") +have_func("rb_gc_mark_movable") # RUBY_VERSION >= 2.7 have_const("RUBY_TYPED_EMBEDDABLE", "ruby.h") # RUBY_VERSION >= 3.3 create_makefile 'fiddle' do |conf| if !libffi diff --git a/test/fiddle/test_closure.rb b/test/fiddle/test_closure.rb index ee9ef044..1a37b6c0 100644 --- a/test/fiddle/test_closure.rb +++ b/test/fiddle/test_closure.rb @@ -172,6 +172,54 @@ def call end end + def test_call_after_compaction + unless GC.respond_to?(:verify_compaction_references) + omit("Need GC.verify_compaction_references") + end + omit("Need CRuby") unless RUBY_ENGINE == "ruby" + require "envutil" unless defined?(EnvUtil) + + # A separate process, because expand_heap: doubles the heap and the pages + # stay. Expanding this process's heap changes how often the collector runs + # afterwards, and test_no_memory_leak reads that as growth. + script = <<~'RUBY' + require "fiddle" + + closure_class = Class.new(Fiddle::Closure) do + def call(a, b) + a + b + end + end + + # The closure must be reachable only through the heap. A local variable is + # pinned by the conservative machine-stack scan. A pinned closure does not + # move, and then this test cannot fail. + holder = [closure_class.new(Fiddle::TYPE_INT, + [Fiddle::TYPE_INT, Fiddle::TYPE_INT])] + begin + begin + GC.verify_compaction_references(expand_heap: true, toward: :empty) + rescue ArgumentError + # Ruby 3.1 and earlier spell expand_heap: as double_heap: + GC.verify_compaction_references(double_heap: true, toward: :empty) + end + rescue NotImplementedError + # A Ruby without compaction cannot move the closure. The call below + # still exercises the plain path. + end + func = Fiddle::Function.new(holder[0].to_i, + [Fiddle::TYPE_INT, Fiddle::TYPE_INT], + Fiddle::TYPE_INT) + puts(func.call(40, 2)) + holder[0].free + RUBY + load_path_args = $LOAD_PATH.flat_map {|path| ["-I", path]} + stdout, stderr, status = EnvUtil.invoke_ruby([*load_path_args, "-e", script], + "", true, true) + assert(status.success?, stderr) + assert_equal("42", stdout.chomp) + end + def test_ractor_shareable omit("Need Ractor") unless defined?(Ractor) closure_class = Class.new(Closure) do