Backport [GR-18163] Fix rb_global_variable() for Float and bignum val…

…ues during the Init_ function to 24.0.1

PullRequest: truffleruby/4218

CHANGELOG.md

@@ -1,3 +1,8 @@
+# 24.0.1
+
+Bug fixes:
+* Fix `rb_global_variable()` for `Float` and bignum values during the `Init_` function (#3478, @eregon).
+
 # 24.0.0
 
 New features:

lib/cext/ABI_check.txt

@@ -1 +1 @@
-13
+14

lib/truffle/truffle/cext.rb

@@ -214,11 +214,15 @@ def init_extension(library, library_path)
     function_name = "Init_#{name}"
 
     init_function = library[function_name]
-    begin
-      Primitive.call_with_c_mutex_and_frame(VOID_TO_VOID_WRAPPER, [init_function], nil, nil)
-    ensure
-      resolve_registered_addresses
-    end
+
+    Primitive.call_with_c_mutex_and_frame(-> {
+      begin
+        Primitive.interop_execute(VOID_TO_VOID_WRAPPER, [init_function])
+      ensure
+        # Resolve while inside the ExtensionCallStackEntry to ensure the preservedObjects are still all alive
+        resolve_registered_addresses
+      end
+    }, [], nil, nil)
   end
 
   def supported?
@@ -2238,15 +2242,19 @@ def rb_gc_register_address(address)
     else
       # Read it immediately if outside Init_ function.
       # This assumes the value is already set when this is called and does not change after that.
-      GC_REGISTERED_ADDRESSES[address] = LIBTRUFFLERUBY.rb_tr_read_VALUE_pointer(address)
+      register_address(address)
     end
   end
 
   def resolve_registered_addresses
-    c_global_variables = Primitive.fiber_c_global_variables
-    c_global_variables.each do |address|
-      GC_REGISTERED_ADDRESSES[address] = LIBTRUFFLERUBY.rb_tr_read_VALUE_pointer(address)
-    end
+    Primitive.fiber_c_global_variables.each { |address| register_address(address) }
+  end
+
+  private def register_address(address)
+    # We save the ValueWrapper here and not the actual value/object, this is important for primitives like double and
+    # not-fixnum-long, as we need to preserve the handle by preserving the ValueWrapper of that handle.
+    # For those cases the primitive cannot itself reference its ValueWrapper, unlike RubyDynamicObject and ImmutableRubyObject.
+    GC_REGISTERED_ADDRESSES[address] = Primitive.cext_to_wrapper LIBTRUFFLERUBY.rb_tr_read_VALUE_pointer(address)
   end
 
   def rb_gc_unregister_address(address)

spec/ruby/optional/capi/ext/gc_spec.c

@@ -8,7 +8,12 @@ extern "C" {
 VALUE registered_tagged_value;
 VALUE registered_reference_value;
 VALUE registered_before_rb_gc_register_address;
-VALUE registered_before_rb_global_variable;
+VALUE registered_before_rb_global_variable_string;
+VALUE registered_before_rb_global_variable_bignum;
+VALUE registered_before_rb_global_variable_float;
+VALUE registered_after_rb_global_variable_string;
+VALUE registered_after_rb_global_variable_bignum;
+VALUE registered_after_rb_global_variable_float;
 VALUE rb_gc_register_address_outside_init;
 
 static VALUE registered_tagged_address(VALUE self) {
@@ -23,8 +28,28 @@ static VALUE get_registered_before_rb_gc_register_address(VALUE self) {
   return registered_before_rb_gc_register_address;
 }
 
-static VALUE get_registered_before_rb_global_variable(VALUE self) {
-  return registered_before_rb_global_variable;
+static VALUE get_registered_before_rb_global_variable_string(VALUE self) {
+  return registered_before_rb_global_variable_string;
+}
+
+static VALUE get_registered_before_rb_global_variable_bignum(VALUE self) {
+  return registered_before_rb_global_variable_bignum;
+}
+
+static VALUE get_registered_before_rb_global_variable_float(VALUE self) {
+  return registered_before_rb_global_variable_float;
+}
+
+static VALUE get_registered_after_rb_global_variable_string(VALUE self) {
+  return registered_after_rb_global_variable_string;
+}
+
+static VALUE get_registered_after_rb_global_variable_bignum(VALUE self) {
+  return registered_after_rb_global_variable_bignum;
+}
+
+static VALUE get_registered_after_rb_global_variable_float(VALUE self) {
+  return registered_after_rb_global_variable_float;
 }
 
 static VALUE gc_spec_rb_gc_register_address(VALUE self) {
@@ -71,17 +96,34 @@ void Init_gc_spec(void) {
   rb_gc_register_address(&registered_tagged_value);
   rb_gc_register_address(&registered_reference_value);
   rb_gc_register_address(&registered_before_rb_gc_register_address);
-  rb_global_variable(&registered_before_rb_global_variable);
+  rb_global_variable(&registered_before_rb_global_variable_string);
+  rb_global_variable(&registered_before_rb_global_variable_bignum);
+  rb_global_variable(&registered_before_rb_global_variable_float);
 
   registered_tagged_value    = INT2NUM(10);
   registered_reference_value = rb_str_new2("Globally registered data");
   registered_before_rb_gc_register_address = rb_str_new_cstr("registered before rb_gc_register_address()");
-  registered_before_rb_global_variable = rb_str_new_cstr("registered before rb_global_variable()");
+
+  registered_before_rb_global_variable_string = rb_str_new_cstr("registered before rb_global_variable()");
+  registered_before_rb_global_variable_bignum = LONG2NUM(INT64_MAX);
+  registered_before_rb_global_variable_float = DBL2NUM(3.14);
+
+  registered_after_rb_global_variable_string = rb_str_new_cstr("registered after rb_global_variable()");
+  rb_global_variable(&registered_after_rb_global_variable_string);
+  registered_after_rb_global_variable_bignum = LONG2NUM(INT64_MAX);
+  rb_global_variable(&registered_after_rb_global_variable_bignum);
+  registered_after_rb_global_variable_float = DBL2NUM(6.28);
+  rb_global_variable(&registered_after_rb_global_variable_float);
 
   rb_define_method(cls, "registered_tagged_address", registered_tagged_address, 0);
   rb_define_method(cls, "registered_reference_address", registered_reference_address, 0);
   rb_define_method(cls, "registered_before_rb_gc_register_address", get_registered_before_rb_gc_register_address, 0);
-  rb_define_method(cls, "registered_before_rb_global_variable", get_registered_before_rb_global_variable, 0);
+  rb_define_method(cls, "registered_before_rb_global_variable_string", get_registered_before_rb_global_variable_string, 0);
+  rb_define_method(cls, "registered_before_rb_global_variable_bignum", get_registered_before_rb_global_variable_bignum, 0);
+  rb_define_method(cls, "registered_before_rb_global_variable_float", get_registered_before_rb_global_variable_float, 0);
+  rb_define_method(cls, "registered_after_rb_global_variable_string", get_registered_after_rb_global_variable_string, 0);
+  rb_define_method(cls, "registered_after_rb_global_variable_bignum", get_registered_after_rb_global_variable_bignum, 0);
+  rb_define_method(cls, "registered_after_rb_global_variable_float", get_registered_after_rb_global_variable_float, 0);
   rb_define_method(cls, "rb_gc_register_address", gc_spec_rb_gc_register_address, 0);
   rb_define_method(cls, "rb_gc_unregister_address", gc_spec_rb_gc_unregister_address, 0);
   rb_define_method(cls, "rb_gc_enable", gc_spec_rb_gc_enable, 0);

spec/ruby/optional/capi/gc_spec.rb

@@ -27,9 +27,36 @@
   end
 
   describe "rb_global_variable" do
-    it "keeps the value alive even if the value is assigned after rb_global_variable() is called" do
+    before :all do
       GC.start
-      @f.registered_before_rb_global_variable.should == "registered before rb_global_variable()"
+    end
+
+    describe "keeps the value alive even if the value is assigned after rb_global_variable() is called" do
+      it "for a string" do
+        @f.registered_before_rb_global_variable_string.should == "registered before rb_global_variable()"
+      end
+
+      it "for a bignum" do
+        @f.registered_before_rb_global_variable_bignum.should == 2**63 - 1
+      end
+
+      it "for a Float" do
+        @f.registered_before_rb_global_variable_float.should == 3.14
+      end
+    end
+
+    describe "keeps the value alive when the value is assigned before rb_global_variable() is called" do
+      it "for a string" do
+        @f.registered_after_rb_global_variable_string.should == "registered after rb_global_variable()"
+      end
+
+      it "for a bignum" do
+        @f.registered_after_rb_global_variable_bignum.should == 2**63 - 1
+      end
+
+      it "for a Float" do
+        @f.registered_after_rb_global_variable_float.should == 6.28
+      end
     end
   end
 

src/main/c/cext/gc.c

@@ -69,8 +69,8 @@ void rb_gc_register_mark_object(VALUE obj) {
 }
 
 void* rb_tr_read_VALUE_pointer(VALUE *pointer) {
-  VALUE value = *pointer;
-  return rb_tr_unwrap(value);
+  // No rb_tr_unwrap() here as the caller actually wants a ValueWrapper or a handle
+  return *pointer;
 }
 
 int rb_during_gc(void) {

src/main/java/org/truffleruby/cext/CExtNodes.java

@@ -1790,11 +1790,10 @@ Object sym2id(RubySymbol symbol,
     public abstract static class WrapValueNode extends PrimitiveArrayArgumentsNode {
 
         @Specialization
-        Object wrapInt(Object value,
+        ValueWrapper wrap(Object value,
                 @Cached WrapNode wrapNode) {
             return wrapNode.execute(value);
         }
-
     }
 
     @Primitive(name = "cext_unwrap")
@@ -1819,6 +1818,21 @@ private String exceptionMessage(Object value) {
         }
     }
 
+    @Primitive(name = "cext_to_wrapper")
+    public abstract static class CExtToWrapperNode extends PrimitiveArrayArgumentsNode {
+        @Specialization
+        ValueWrapper toWrapper(Object value,
+                @Cached UnwrapNode.ToWrapperNode toWrapperNode) {
+            ValueWrapper wrapper = toWrapperNode.execute(this, value);
+            if (wrapper == null) {
+                CompilerDirectives.transferToInterpreterAndInvalidate();
+                throw CompilerDirectives.shouldNotReachHere("ValueWrapper not found for " + value);
+            }
+            return wrapper;
+        }
+    }
+
+
     @CoreMethod(names = "create_mark_list", onSingleton = true, required = 1)
     public abstract static class NewMarkerList extends CoreMethodArrayArgumentsNode {
 
@@ -1836,7 +1850,7 @@ Object createNewMarkList(RubyDynamicObject object,
     public abstract static class AddToMarkList extends CoreMethodArrayArgumentsNode {
 
         @Specialization
-        Object addToMarkList(Object markedObject,
+        Object rbGCMark(Object markedObject,
                 @Cached InlinedBranchProfile noExceptionProfile,
                 @Cached UnwrapNode.ToWrapperNode toWrapperNode) {
             ValueWrapper wrappedValue = toWrapperNode.execute(this, markedObject);

src/main/java/org/truffleruby/cext/ValueWrapper.java

@@ -74,10 +74,10 @@ protected Class<RubyLanguage> getLanguage() {
     @Override
     public String toString() {
         if (object != null) {
-            return object.toString();
+            return "ValueWrapper[" + object + "]";
         } else {
             assert ValueWrapperManager.isTaggedLong(handle);
-            return Long.toString(ValueWrapperManager.untagTaggedLong(handle));
+            return "ValueWrapper[" + ValueWrapperManager.untagTaggedLong(handle) + "]";
         }
     }
 
@@ -92,7 +92,7 @@ protected String toDisplayString(boolean allowSideEffects) {
                 throw TranslateInteropExceptionNode.executeUncached(e);
             }
         } else {
-            return "VALUE: " + toString();
+            return "VALUE: " + this;
         }
     }
 

src/main/java/org/truffleruby/cext/ValueWrapperManager.java

@@ -64,23 +64,13 @@ public final class ValueWrapperManager {
 
     private volatile HandleBlockWeakReference[] blockMap = new HandleBlockWeakReference[0];
 
-    public static HandleBlockHolder getBlockHolder(RubyContext context, RubyLanguage language) {
+    public static HandleBlockHolder getBlockHolder(RubyLanguage language) {
         return language.getCurrentFiber().handleData;
     }
 
-    /* We keep a map of long wrappers that have been generated because various C extensions assume that any given fixnum
-     * will translate to a given VALUE. */
-    public ValueWrapper longWrapper(long value) {
-        return new ValueWrapper(value, UNSET_HANDLE, null);
-    }
-
-    public ValueWrapper doubleWrapper(double value) {
-        return new ValueWrapper(value, UNSET_HANDLE, null);
-    }
-
     @TruffleBoundary
-    public synchronized HandleBlock addToBlockMap(RubyContext context, RubyLanguage language) {
-        HandleBlock block = new HandleBlock(context, language, this);
+    public synchronized HandleBlock addToBlockMap(RubyLanguage language) {
+        HandleBlock block = new HandleBlock(language, this);
         int blockIndex = block.getIndex();
         HandleBlockWeakReference[] map = growMapIfRequired(blockMap, blockIndex);
         blockMap = map;
@@ -90,9 +80,9 @@ public synchronized HandleBlock addToBlockMap(RubyContext context, RubyLanguage
     }
 
     @TruffleBoundary
-    public HandleBlock addToSharedBlockMap(RubyContext context, RubyLanguage language) {
+    public HandleBlock addToSharedBlockMap(RubyLanguage language) {
         synchronized (language) {
-            HandleBlock block = new HandleBlock(context, language, this);
+            HandleBlock block = new HandleBlock(language, this);
             int blockIndex = block.getIndex();
             HandleBlockWeakReference[] map = growMapIfRequired(language.handleBlockSharedMap, blockIndex);
             language.handleBlockSharedMap = map;
@@ -153,7 +143,7 @@ public void freeAllBlocksInMap() {
         }
     }
 
-    public void cleanup(RubyContext context, HandleBlockHolder holder) {
+    public void cleanup(HandleBlockHolder holder) {
         holder.handleBlock = null;
     }
 
@@ -215,7 +205,7 @@ public static final class HandleBlock {
 
         @SuppressWarnings("unused") private Cleanable cleanable;
 
-        public HandleBlock(RubyContext context, RubyLanguage language, ValueWrapperManager manager) {
+        public HandleBlock(RubyLanguage language, ValueWrapperManager manager) {
             HandleBlockAllocator allocator = language.handleBlockAllocator;
             long base = allocator.getFreeBlock();
             this.base = base;
@@ -297,7 +287,7 @@ static long allocateHandleOnKnownThread(Node node, ValueWrapper wrapper) {
                     wrapper,
                     getContext(node),
                     getLanguage(node),
-                    getBlockHolder(getContext(node), getLanguage(node)),
+                    getBlockHolder(getLanguage(node)),
                     false);
         }
 
@@ -311,7 +301,7 @@ static long allocateSharedHandleOnKnownThread(Node node, ValueWrapper wrapper) {
                     wrapper,
                     getContext(node),
                     getLanguage(node),
-                    getBlockHolder(getContext(node), getLanguage(node)),
+                    getBlockHolder(getLanguage(node)),
                     true);
         }
 
@@ -339,10 +329,10 @@ protected static long allocateHandle(Node node, ValueWrapper wrapper, RubyContex
 
             if (block == null || block.isFull()) {
                 if (shared) {
-                    block = context.getValueWrapperManager().addToSharedBlockMap(context, language);
+                    block = context.getValueWrapperManager().addToSharedBlockMap(language);
                     holder.sharedHandleBlock = block;
                 } else {
-                    block = context.getValueWrapperManager().addToBlockMap(context, language);
+                    block = context.getValueWrapperManager().addToBlockMap(language);
                     holder.handleBlock = block;
                 }
 
@@ -356,10 +346,8 @@ protected static boolean isSharedObject(ValueWrapper wrapper) {
     }
 
     public static HandleBlock allocateNewBlock(RubyContext context, RubyLanguage language) {
-        HandleBlockHolder holder = getBlockHolder(context, language);
-        HandleBlock block = holder.handleBlock;
-
-        block = context.getValueWrapperManager().addToBlockMap(context, language);
+        HandleBlockHolder holder = getBlockHolder(language);
+        HandleBlock block = context.getValueWrapperManager().addToBlockMap(language);
 
         holder.handleBlock = block;
         return block;