From a127b6585c36fc63eb9270cef0001ab5932e18b5 Mon Sep 17 00:00:00 2001 From: cfis Date: Wed, 21 Jan 2026 01:24:22 -0800 Subject: [PATCH 01/12] Run tests with stress GC on and see what happens. --- test/embed_ruby.cpp | 5 +++++ test/extconf.rb | 3 +++ 2 files changed, 8 insertions(+) diff --git a/test/embed_ruby.cpp b/test/embed_ruby.cpp index 00981cde..5dff102c 100644 --- a/test/embed_ruby.cpp +++ b/test/embed_ruby.cpp @@ -24,6 +24,11 @@ void embed_ruby() ruby_options(2, (char**)opts); #endif + // Enable GC stress to help catch GC-related bugs +#if RICE_RELEASE + rb_funcall(rb_mGC, rb_intern("stress="), 1, Qtrue); +#endif + initialized__ = true; } } diff --git a/test/extconf.rb b/test/extconf.rb index 8a832a30..ba53790c 100644 --- a/test/extconf.rb +++ b/test/extconf.rb @@ -4,6 +4,9 @@ abort "libffi not found" unless have_libffi +# Specify release mode +append_cppflags("-DRICE_RELEASE") + # Totally hack mkmf to make a unittest executable instead of a shared library target_exe = "unittest#{RbConfig::CONFIG['EXEEXT']}" $cleanfiles << target_exe From f24307a428a0ff91a9716f6b29bee2dda17b1bad Mon Sep 17 00:00:00 2001 From: Charlie Savage Date: Thu, 22 Jan 2026 20:30:48 -0800 Subject: [PATCH 02/12] Fix tests that fail with GC.stress = true --- test/test_Callback.cpp | 13 +++++++++---- test/test_Stl_SharedPtr.cpp | 5 ++++- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/test/test_Callback.cpp b/test/test_Callback.cpp index 2cea3af7..45bdc0dd 100644 --- a/test/test_Callback.cpp +++ b/test/test_Callback.cpp @@ -151,7 +151,8 @@ namespace char* triggerCallback(int callBackIndex) { Callback_T2 callback = callbacks[callBackIndex]; - return callback(); + char* result = callback(); + return result; } } @@ -160,13 +161,17 @@ TESTCASE(MultipleCallbacks) { Module m = define_module("TestingMultipleCallbacks"); m.define_module_function("register_callback", registerTwoCallbacks). - define_module_function("trigger_callback", triggerCallback); + define_module_function("trigger_callback", triggerCallback, Return().takeOwnership()); - std::string code = R"(proc1 = Proc.new do + // The callback returns char* which we need to take ownership of otherwise it will be freed + // when the Ruby string from the proc gets GCed. + define_callback(Return().takeOwnership()); + + std::string code = R"(proc1 = Proc.new do "Proc 1" end - proc2 = Proc.new do + proc2 = Proc.new do "Proc 2" end diff --git a/test/test_Stl_SharedPtr.cpp b/test/test_Stl_SharedPtr.cpp index 0f2f2e77..4441f56d 100644 --- a/test/test_Stl_SharedPtr.cpp +++ b/test/test_Stl_SharedPtr.cpp @@ -244,7 +244,11 @@ TESTCASE(ShareOwnership2) ASSERT_EQUAL(0, Factory::instance_.use_count()); m.module_eval(code); +#if RICE_RELEASE + ASSERT_EQUAL(1, Factory::instance_.use_count()); +#else ASSERT_EQUAL(11, Factory::instance_.use_count()); +#endif rb_gc_start(); ASSERT_EQUAL(1, Factory::instance_.use_count()); @@ -253,7 +257,6 @@ TESTCASE(ShareOwnership2) ASSERT_EQUAL(0, MyClass::moveConstructorCalls); ASSERT_EQUAL(0, MyClass::destructorCalls); ASSERT_EQUAL(9, Factory::instance_->flag); - } TESTCASE(PtrParameter) From 02780859e3b093f73be71b49b6d279c897d8736c Mon Sep 17 00:00:00 2001 From: Charlie Savage Date: Thu, 22 Jan 2026 20:31:04 -0800 Subject: [PATCH 03/12] Fix path to inclues. --- Rice.cmake | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Rice.cmake b/Rice.cmake index ef60d0b8..a4831637 100644 --- a/Rice.cmake +++ b/Rice.cmake @@ -4,8 +4,8 @@ if(NOT TARGET Rice) add_library(Rice INTERFACE) target_include_directories(Rice INTERFACE - $:${CMAKE_CURRENT_SOURCE_DIR}>> - $:${CMAKE_CURRENT_SOURCE_DIR}/include>> + $:${CMAKE_CURRENT_LIST_DIR}>> + $:${CMAKE_CURRENT_LIST_DIR}/include>> $ ) From 42d143ec6f73bb26cda6ebe9665e3160bbdfa8b7 Mon Sep 17 00:00:00 2001 From: Charlie Savage Date: Thu, 22 Jan 2026 20:31:40 -0800 Subject: [PATCH 04/12] Include Anchor/Pin sooner so they can be used by C++ api objects --- rice/rice.hpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/rice/rice.hpp b/rice/rice.hpp index 1d0875e2..acb92c16 100644 --- a/rice/rice.hpp +++ b/rice/rice.hpp @@ -43,6 +43,12 @@ #include "detail/RubyType.hpp" #include "detail/Parameter.hpp" +// Code to register Ruby objects with GC +#include "detail/Anchor.hpp" +#include "detail/Anchor.ipp" +#include "Pin.hpp" +#include "Pin.ipp" + // C++ API declarations #include "cpp_api/Encoding.hpp" #include "cpp_api/Identifier.hpp" @@ -117,10 +123,6 @@ #include "detail/NativeMethod.ipp" #include "detail/NativeProc.hpp" #include "detail/NativeProc.ipp" -#include "detail/Anchor.hpp" -#include "detail/Anchor.ipp" -#include "Pin.hpp" -#include "Pin.ipp" #include "detail/NativeCallback.hpp" #include "detail/NativeCallback.ipp" #include "detail/Proc.ipp" From 916df8058e6343627f346daae81a8d430ac71ebf Mon Sep 17 00:00:00 2001 From: Charlie Savage Date: Thu, 22 Jan 2026 20:33:21 -0800 Subject: [PATCH 05/12] Improve the Pin api --- rice/Pin.hpp | 5 +---- rice/Pin.ipp | 7 +------ rice/detail/Anchor.hpp | 2 +- rice/detail/Anchor.ipp | 11 ++++++++--- rice/detail/NativeCallback.ipp | 2 +- 5 files changed, 12 insertions(+), 15 deletions(-) diff --git a/rice/Pin.hpp b/rice/Pin.hpp index 7a30500a..fe36aab6 100644 --- a/rice/Pin.hpp +++ b/rice/Pin.hpp @@ -28,11 +28,8 @@ namespace Rice Pin(Pin&&) noexcept = default; Pin& operator=(Pin&&) noexcept = default; - //! Replace the pinned Ruby VALUE. - void set(VALUE value); - //! Retrieve the pinned Ruby VALUE. - VALUE get() const; + VALUE value() const; private: //! Shared ownership of the internal GC anchor. diff --git a/rice/Pin.ipp b/rice/Pin.ipp index e72c0ce6..50533bb9 100644 --- a/rice/Pin.ipp +++ b/rice/Pin.ipp @@ -5,12 +5,7 @@ namespace Rice { } - inline void Pin::set(VALUE value) - { - this->anchor_->set(value); - } - - inline VALUE Pin::get() const + inline VALUE Pin::value() const { return this->anchor_->get(); } diff --git a/rice/detail/Anchor.hpp b/rice/detail/Anchor.hpp index bf7c59cb..caef56e6 100644 --- a/rice/detail/Anchor.hpp +++ b/rice/detail/Anchor.hpp @@ -51,7 +51,7 @@ namespace Rice private: //! GC-visible Ruby VALUE slot. - VALUE value_; + VALUE value_ = Qnil; }; } } diff --git a/rice/detail/Anchor.ipp b/rice/detail/Anchor.ipp index bc9df57d..8f1c9a89 100644 --- a/rice/detail/Anchor.ipp +++ b/rice/detail/Anchor.ipp @@ -4,16 +4,21 @@ namespace Rice { inline Anchor::Anchor(VALUE value) : value_(value) { - Anchor::registerExitHandler(); - detail::protect(rb_gc_register_address, &this->value_); + if (value != Qnil) + { + Anchor::registerExitHandler(); + detail::protect(rb_gc_register_address, &this->value_); + } } inline Anchor::~Anchor() { - if (Anchor::enabled_) + if (Anchor::enabled_ && this->value_ != Qnil) { detail::protect(rb_gc_unregister_address, &this->value_); } + // Ruby auto detects VALUEs in the stack, so make sure up in case this object is on the stack + this->value_ = Qnil; } inline VALUE Anchor::get() const diff --git a/rice/detail/NativeCallback.ipp b/rice/detail/NativeCallback.ipp index 2be85e03..f6bb6d49 100644 --- a/rice/detail/NativeCallback.ipp +++ b/rice/detail/NativeCallback.ipp @@ -221,7 +221,7 @@ namespace Rice::detail convertToRuby(args)... }; static Identifier id("call"); - VALUE result = detail::protect(rb_funcallv, this->proc_.get(), id.id(), (int)sizeof...(Parameter_Ts), values.data()); + VALUE result = detail::protect(rb_funcallv, this->proc_.value(), id.id(), (int)sizeof...(Parameter_Ts), values.data()); if constexpr (!std::is_void_v) { return this->fromRuby_.convert(result); From 5349a53421a3cf5ffa29fe95355ca4b7fefd0425 Mon Sep 17 00:00:00 2001 From: Charlie Savage Date: Thu, 22 Jan 2026 20:39:55 -0800 Subject: [PATCH 06/12] Update char* <-> Ruby string conversions --- rice/detail/from_ruby.ipp | 20 ++++++++++++++++---- rice/detail/to_ruby.ipp | 14 ++++++++------ 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/rice/detail/from_ruby.ipp b/rice/detail/from_ruby.ipp index ad05d79b..6114d814 100644 --- a/rice/detail/from_ruby.ipp +++ b/rice/detail/from_ruby.ipp @@ -455,13 +455,25 @@ namespace Rice::detail } case RUBY_T_STRING: { - // WARNING - this shares the Ruby string memory directly with C++. value really should be frozen. - // Maybe we should enforce that? Note the user can always create a Buffer to allocate new memory. - return rb_string_value_cstr(&value); + if (this->arg_->isOwner()) + { + // Warning - the receiver needs to free this string! + // TODO - raise an exception if the string has null values? + long len = RSTRING_LEN(value); + char* result = (char*)malloc(len + 1); + memcpy(result, RSTRING_PTR(value), len); + result[len] = '\0'; + return result; + } + else + { + // WARNING - this shares the Ruby string memory directly with C++. value really should be frozen. + // Maybe we should enforce that? Note the user can always create a Buffer to allocate new memory. + return rb_string_value_cstr(&value); + } } default: { - char* rb_string_value_cstr(volatile VALUE * ptr); return FromRubyFundamental::convert(value); } } diff --git a/rice/detail/to_ruby.ipp b/rice/detail/to_ruby.ipp index 16a29f08..81e6a9d1 100644 --- a/rice/detail/to_ruby.ipp +++ b/rice/detail/to_ruby.ipp @@ -271,16 +271,18 @@ namespace Rice } else if (this->arg_ && this->arg_->isOwner()) { - // This copies the buffer but does not free it. So Ruby is not really - // taking ownership of it. But there isn't a Ruby API for creating a string - // from an existing buffer and later freeing it. - return protect(rb_usascii_str_new_cstr, data); + // This copies the buffer but does not free it + VALUE result = protect(rb_usascii_str_new_cstr, data); + // And free the char* since we were told to take "ownership" + // TODO - is this a good idea? + //free(data); + return result; } else { // Does NOT copy the passed in buffer and does NOT free it when the string is GCed - long size = (long)strlen(data); - VALUE result = protect(rb_str_new_static, data, size); + long len = (long)strlen(data); + VALUE result = protect(rb_str_new_static, data, len); // Freeze the object so Ruby can't modify the C string return rb_obj_freeze(result); } From 021a4d35f9ccda159f17bcf1d685a60e2d25b599 Mon Sep 17 00:00:00 2001 From: Charlie Savage Date: Thu, 22 Jan 2026 20:40:02 -0800 Subject: [PATCH 07/12] Spelling --- rice/cpp_api/Module.hpp | 2 +- rice/cpp_api/Module.ipp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rice/cpp_api/Module.hpp b/rice/cpp_api/Module.hpp index 53135974..405d717b 100644 --- a/rice/cpp_api/Module.hpp +++ b/rice/cpp_api/Module.hpp @@ -24,7 +24,7 @@ namespace Rice //! Construct a Module from an existing Module object. Module(VALUE v); - //! Construct a Module from an string that references a Module + //! Construct a Module from a string that references a Module Module(std::string name, Object under = rb_cObject); //! Return the name of the module. diff --git a/rice/cpp_api/Module.ipp b/rice/cpp_api/Module.ipp index cc2b563a..65cc712d 100644 --- a/rice/cpp_api/Module.ipp +++ b/rice/cpp_api/Module.ipp @@ -16,7 +16,7 @@ namespace Rice } } - //! Construct a Module from an string that references a Module + //! Construct a Module from a string that references a Module inline Module::Module(std::string name, Object under) { VALUE result = under.const_get(name); From 9d520d4812b9bfde8529985d723a6db9c1edccc7 Mon Sep 17 00:00:00 2001 From: Charlie Savage Date: Thu, 22 Jan 2026 20:40:09 -0800 Subject: [PATCH 08/12] API updates --- rice/cpp_api/Object.hpp | 12 ++++++------ rice/cpp_api/Object.ipp | 33 +++++++++++++++++++++++++++++---- rice/forward_declares.ipp | 2 +- 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/rice/cpp_api/Object.hpp b/rice/cpp_api/Object.hpp index 4ea62ed9..5945b8c3 100644 --- a/rice/cpp_api/Object.hpp +++ b/rice/cpp_api/Object.hpp @@ -39,20 +39,20 @@ namespace Rice // Having this conversion also prevents accidental conversion to // undesired integral types (e.g. long or int) by making the // conversion ambiguous. - bool test() const { return RTEST(value_); } + bool test() const; //! Returns false if the object is nil or false; returns true //! otherwise. - operator bool() const { return test(); } + operator bool() const; //! Returns true if the object is nil, false otherwise. - bool is_nil() const { return NIL_P(value_); } + bool is_nil() const; //! Implicit conversion to VALUE. - operator VALUE() const { return value_; } + operator VALUE() const; //! Explicitly get the encapsulated VALUE. - VALUE value() const { return value_; } + VALUE value() const; //! Get the class of an object. /*! \return the object's Class. @@ -254,7 +254,7 @@ namespace Rice protected: //! Set the encapsulated value. - void set_value(VALUE v); + void set_value(VALUE value); private: volatile VALUE value_; diff --git a/rice/cpp_api/Object.ipp b/rice/cpp_api/Object.ipp index e08b8681..1de99d3e 100644 --- a/rice/cpp_api/Object.ipp +++ b/rice/cpp_api/Object.ipp @@ -27,6 +27,31 @@ namespace Rice return *this; } + inline bool Object::test() const + { + return RTEST(this->value()); + } + + inline Object::operator bool() const + { + return this->test(); + } + + inline bool Object::is_nil() const + { + return NIL_P(this->value()); + } + + inline Object::operator VALUE() const + { + return this->value(); + } + + inline VALUE Object::value() const + { + return this->value_; + } + template inline Object Object::call(Identifier id, Parameter_Ts... args) const { @@ -63,13 +88,13 @@ namespace Rice inline bool Object::is_equal(const Object& other) const { - VALUE result = detail::protect(rb_equal, this->value_, other.value_); + VALUE result = detail::protect(rb_equal, this->value(), other.value()); return RB_TEST(result); } inline bool Object::is_eql(const Object& other) const { - VALUE result = detail::protect(rb_eql, this->value_, other.value_); + VALUE result = detail::protect(rb_eql, this->value(), other.value()); return RB_TEST(result); } @@ -125,9 +150,9 @@ namespace Rice return detail::protect(rb_attr_get, this->value(), name.id()); } - inline void Object::set_value(VALUE v) + inline void Object::set_value(VALUE value) { - value_ = v; + value_ = value; } inline Object Object::const_get(Identifier name) const diff --git a/rice/forward_declares.ipp b/rice/forward_declares.ipp index 1e8a5f51..b5ba3c54 100644 --- a/rice/forward_declares.ipp +++ b/rice/forward_declares.ipp @@ -6,7 +6,7 @@ namespace Rice // These methods cannot be defined where they are declared due to circular dependencies inline Class Object::class_of() const { - return detail::protect(rb_class_of, value_); + return detail::protect(rb_class_of, this->value()); } inline String Object::to_s() const From 5a8fbf9598ce230abd14b82ca7639215b9a145cc Mon Sep 17 00:00:00 2001 From: Charlie Savage Date: Thu, 22 Jan 2026 21:36:10 -0800 Subject: [PATCH 09/12] Check if arg is set. --- rice/detail/from_ruby.ipp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rice/detail/from_ruby.ipp b/rice/detail/from_ruby.ipp index 6114d814..9d11bda9 100644 --- a/rice/detail/from_ruby.ipp +++ b/rice/detail/from_ruby.ipp @@ -455,7 +455,7 @@ namespace Rice::detail } case RUBY_T_STRING: { - if (this->arg_->isOwner()) + if (this->arg_ && this->arg_->isOwner()) { // Warning - the receiver needs to free this string! // TODO - raise an exception if the string has null values? From 862cbd6064a33753777c1fef2f44e5f490c683a1 Mon Sep 17 00:00:00 2001 From: Charlie Savage Date: Thu, 22 Jan 2026 21:36:19 -0800 Subject: [PATCH 10/12] Update for changed API --- test/test_Pin.cpp | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/test/test_Pin.cpp b/test/test_Pin.cpp index cdeb3bb1..73e08261 100644 --- a/test/test_Pin.cpp +++ b/test/test_Pin.cpp @@ -28,12 +28,7 @@ namespace VALUE getValue() const { - return pin_.get(); - } - - void setValue(VALUE value) - { - pin_.set(value); + return pin_.value(); } private: @@ -45,7 +40,7 @@ TESTCASE(prevents_gc_collection) { Container* container = nullptr; - // Create container with a Ruby string in a nested scope + // Create a Container with a Ruby string in a nested scope { VALUE str = rb_str_new_cstr("prevent gc test"); container = new Container(str); @@ -61,19 +56,6 @@ TESTCASE(prevents_gc_collection) delete container; } -TESTCASE(update_pinned_value) -{ - Container container(rb_str_new_cstr("first")); - - rb_gc_start(); - ASSERT_EQUAL("first", detail::From_Ruby().convert(container.getValue())); - - container.setValue(rb_str_new_cstr("second")); - - rb_gc_start(); - ASSERT_EQUAL("second", detail::From_Ruby().convert(container.getValue())); -} - TESTCASE(copy_container) { Container c1(rb_str_new_cstr("shared value")); From 56cdc82906e813810412360f7376bb5e6af7026f Mon Sep 17 00:00:00 2001 From: cfis Date: Thu, 22 Jan 2026 21:55:22 -0800 Subject: [PATCH 11/12] Disable gc stress for now, still one bug that needs to be fixed --- test/embed_ruby.cpp | 2 +- test/test_Stl_SharedPtr.cpp | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/embed_ruby.cpp b/test/embed_ruby.cpp index 5dff102c..3c897ad6 100644 --- a/test/embed_ruby.cpp +++ b/test/embed_ruby.cpp @@ -26,7 +26,7 @@ void embed_ruby() // Enable GC stress to help catch GC-related bugs #if RICE_RELEASE - rb_funcall(rb_mGC, rb_intern("stress="), 1, Qtrue); + // rb_funcall(rb_mGC, rb_intern("stress="), 1, Qtrue); #endif initialized__ = true; diff --git a/test/test_Stl_SharedPtr.cpp b/test/test_Stl_SharedPtr.cpp index 4441f56d..671977eb 100644 --- a/test/test_Stl_SharedPtr.cpp +++ b/test/test_Stl_SharedPtr.cpp @@ -244,11 +244,11 @@ TESTCASE(ShareOwnership2) ASSERT_EQUAL(0, Factory::instance_.use_count()); m.module_eval(code); -#if RICE_RELEASE - ASSERT_EQUAL(1, Factory::instance_.use_count()); -#else +//#if RICE_RELEASE +// ASSERT_EQUAL(2, Factory::instance_.use_count()); +//#else ASSERT_EQUAL(11, Factory::instance_.use_count()); -#endif +//#endif rb_gc_start(); ASSERT_EQUAL(1, Factory::instance_.use_count()); From be4646dcc1322034ae10c3c75d54c303b8a427e2 Mon Sep 17 00:00:00 2001 From: cfis Date: Thu, 22 Jan 2026 21:55:35 -0800 Subject: [PATCH 12/12] Clean up comments. --- rice/Pin.hpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/rice/Pin.hpp b/rice/Pin.hpp index fe36aab6..e60207de 100644 --- a/rice/Pin.hpp +++ b/rice/Pin.hpp @@ -18,13 +18,14 @@ namespace Rice class Pin { public: - //! Construct a strong pin for a Ruby VALUE. - explicit Pin(VALUE value); + //! Construct a pin from a Ruby VALUE. + Pin(VALUE value); - // Shared-anchor semantics. + // Copying Pin(const Pin&) = default; Pin& operator=(const Pin&) = default; + // Moving Pin(Pin&&) noexcept = default; Pin& operator=(Pin&&) noexcept = default;