Run tests with stress GC on and see what happens.

Rice.cmake

@@ -4,8 +4,8 @@ if(NOT TARGET Rice)
   add_library(Rice INTERFACE)
 
   target_include_directories(Rice INTERFACE
-    $<BUILD_INTERFACE:$<$<CONFIG:Debug>:${CMAKE_CURRENT_SOURCE_DIR}>>
-    $<BUILD_INTERFACE:$<$<CONFIG:Release>:${CMAKE_CURRENT_SOURCE_DIR}/include>>
+    $<BUILD_INTERFACE:$<$<CONFIG:Debug>:${CMAKE_CURRENT_LIST_DIR}>>
+    $<BUILD_INTERFACE:$<$<CONFIG:Release>:${CMAKE_CURRENT_LIST_DIR}/include>>
     $<INSTALL_INTERFACE:include>
   )
 

rice/Pin.hpp

@@ -18,21 +18,19 @@ 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;
 
-    //! 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.

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();
   }

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.

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);

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_;

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<typename ...Parameter_Ts>
   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

rice/detail/Anchor.hpp

@@ -51,7 +51,7 @@ namespace Rice
 
     private:
       //! GC-visible Ruby VALUE slot.
-      VALUE value_;
+      VALUE value_ = Qnil;
     };
   }
 }

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

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_T>)
     {
       return this->fromRuby_.convert(result);

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_ && 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<char*>::convert(value);
         }
       }

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);
         }

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

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"

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;
   }
 }

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

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<void(*)(Callback_T2, Callback_T2)>("register_callback", registerTwoCallbacks).
-    define_module_function<char*(*)(int)>("trigger_callback", triggerCallback);
+    define_module_function<char*(*)(int)>("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<Callback_T2>(Return().takeOwnership());
+
+  std::string code = R"(proc1 = Proc.new do
                                   "Proc 1"
                                 end
 
-                        proc2 = Proc.new do 
+                        proc2 = Proc.new do
                                   "Proc 2"
                                 end
 

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<char*>().convert(container.getValue()));
-
-  container.setValue(rb_str_new_cstr("second"));
-
-  rb_gc_start();
-  ASSERT_EQUAL("second", detail::From_Ruby<char*>().convert(container.getValue()));
-}
-
 TESTCASE(copy_container)
 {
   Container c1(rb_str_new_cstr("shared value"));