Support writing to RData.dfree for native extensions

* Fixes oracle#2830 * Fixes oracle#2732 * Fixes oracle#2165
sophie-kaleba · Feb 16, 2023 · d8d2e2c · d8d2e2c
1 parent 2ecc547
commit d8d2e2c
Show file tree

Hide file tree

Showing 9 changed files with 48 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -66,6 +66,7 @@ Compatibility:
 * Implement `Array#intersect?` (#2831, @nirvdrum).
 * Record the source location in the constant for the `module`/`class` keywords (#2833, @eregon).
 * Fix `File.open` and support `flags` option (#2820, @andrykonchin).
+* Support writing to `RData.dfree` for native extensions (#2830, #2732, #2165, @eregon).
 
 Performance:
 

diff --git a/lib/truffle/truffle/cext.rb b/lib/truffle/truffle/cext.rb
@@ -1481,7 +1481,7 @@ def rb_data_object_wrap(ruby_class, data, mark, free)
     data_holder = Primitive.data_holder_create(data, mark, free)
     Primitive.object_hidden_var_set object, DATA_HOLDER, data_holder
 
-    Primitive.object_space_define_data_finalizer object, free, data_holder unless Truffle::Interop.null?(free)
+    Primitive.object_space_define_data_finalizer object, data_holder unless Truffle::Interop.null?(free)
 
     define_marker object, mark
 
@@ -1496,7 +1496,7 @@ def rb_data_typed_object_wrap(ruby_class, data, data_type, mark, free, size)
     Primitive.object_hidden_var_set object, DATA_HOLDER, data_holder
     Primitive.object_hidden_var_set object, DATA_MEMSIZER, data_sizer(size, data_holder) unless Truffle::Interop.null?(size)
 
-    Primitive.object_space_define_data_finalizer object, free, data_holder unless Truffle::Interop.null?(free)
+    Primitive.object_space_define_data_finalizer object, data_holder unless Truffle::Interop.null?(free)
 
     define_marker object, mark
 

diff --git a/lib/truffle/truffle/cext_structs.rb b/lib/truffle/truffle/cext_structs.rb
@@ -87,8 +87,15 @@ def polyglot_read_member(name)
   end
 
   def polyglot_write_member(name, value)
-    raise Truffle::Interop::UnknownIdentifierException unless name == 'data'
-    Primitive.data_holder_set_data(@data_holder, value)
+    case name
+    when 'data'
+      Primitive.data_holder_set_data(@data_holder, value)
+    when 'dfree'
+      Primitive.data_holder_set_free(@data_holder, value)
+    else
+      raise Truffle::Interop::UnknownIdentifierException
+    end
+
   end
 
   def polyglot_remove_member(name)
@@ -104,7 +111,7 @@ def polyglot_member_readable?(name)
   end
 
   def polyglot_member_modifiable?(name)
-    name == 'data'
+    name == 'data' or name == 'dfree'
   end
 
   def polyglot_member_removable?(name)

diff --git a/src/main/java/org/truffleruby/cext/CExtNodes.java b/src/main/java/org/truffleruby/cext/CExtNodes.java
@@ -2116,11 +2116,20 @@ protected Object getMarker(Nil data) {
     @Primitive(name = "data_holder_get_free")
     public abstract static class DataHolderGetFree extends PrimitiveArrayArgumentsNode {
         @Specialization
-        protected Object getMarker(DataHolder data) {
+        protected Object getFree(DataHolder data) {
             return data.getFree();
         }
     }
 
+    @Primitive(name = "data_holder_set_free")
+    public abstract static class DataHolderSetFree extends PrimitiveArrayArgumentsNode {
+        @Specialization
+        protected Object setFree(DataHolder data, Object dfree) {
+            data.setFree(dfree);
+            return dfree;
+        }
+    }
+
     @Primitive(name = "data_holder_is_holder?")
     public abstract static class DataHolderIsHolder extends PrimitiveArrayArgumentsNode {
 

diff --git a/src/main/java/org/truffleruby/cext/DataHolder.java b/src/main/java/org/truffleruby/cext/DataHolder.java
@@ -18,16 +18,21 @@
 
 import org.truffleruby.RubyLanguage;
 import org.truffleruby.interop.TranslateInteropExceptionNode;
+import org.truffleruby.language.RubyGuards;
 
 /** This object represents a struct pointer in C held by a Ruby object. */
 @ExportLibrary(InteropLibrary.class)
 public final class DataHolder implements TruffleObject {
 
     private Object pointer;
     private final Object marker;
-    private final Object free;
+    private Object free;
 
     public DataHolder(Object pointer, Object marker, Object free) {
+        // All foreign objects, so we don't need to share them for SharedObjects
+        assert RubyGuards.isForeignObject(pointer);
+        assert RubyGuards.isForeignObject(marker);
+        assert RubyGuards.isForeignObject(free);
         this.pointer = pointer;
         this.marker = marker;
         this.free = free;
@@ -38,6 +43,7 @@ public Object getPointer() {
     }
 
     public void setPointer(Object pointer) {
+        assert RubyGuards.isForeignObject(pointer);
         this.pointer = pointer;
     }
 
@@ -49,6 +55,11 @@ public Object getFree() {
         return free;
     }
 
+    public void setFree(Object free) {
+        assert RubyGuards.isForeignObject(free);
+        this.free = free;
+    }
+
     @ExportMessage
     protected boolean hasLanguage() {
         return true;

diff --git a/src/main/java/org/truffleruby/core/DataObjectFinalizationService.java b/src/main/java/org/truffleruby/core/DataObjectFinalizationService.java
@@ -38,15 +38,17 @@ public class DataObjectFinalizationService extends ReferenceProcessingService<Da
     // We need a base node here, it should extend ruby base root node and implement internal root node.
     public static class DataObjectFinalizerRootNode extends RubyBaseRootNode implements InternalRootNode {
 
-        @Child private InteropLibrary nullNode;
+        @Child private InteropLibrary nullDataPointerNode;
+        @Child private InteropLibrary nullDataFreeNode;
         @Child private InteropLibrary callNode;
         private final ConditionProfile ownedProfile = ConditionProfile.create();
 
         public DataObjectFinalizerRootNode(
                 RubyLanguage language) {
             super(language, RubyLanguage.EMPTY_FRAME_DESCRIPTOR, null);
 
-            nullNode = InteropLibrary.getFactory().createDispatched(1);
+            nullDataPointerNode = InteropLibrary.getFactory().createDispatched(1);
+            nullDataFreeNode = InteropLibrary.getFactory().createDispatched(1);
             callNode = InteropLibrary.getFactory().createDispatched(1);
         }
 
@@ -80,11 +82,12 @@ private void runFinalizer(DataObjectFinalizerReference ref) throws Error {
             try {
                 if (!getContext().isFinalizing()) {
                     Object data = ref.dataHolder.getPointer();
-                    if (!nullNode.isNull(data)) {
+                    Object callable = ref.dataHolder.getFree();
+                    if (!nullDataPointerNode.isNull(data) && !nullDataFreeNode.isNull(callable)) {
                         final ExtensionCallStack stack = getLanguage().getCurrentFiber().extensionCallStack;
                         stack.push(false, stack.getSpecialVariables(), stack.getBlock());
                         try {
-                            callNode.execute(ref.callable, data);
+                            callNode.execute(callable, data);
                         } finally {
                             stack.pop();
                         }
@@ -107,18 +110,17 @@ public DataObjectFinalizationService(RubyLanguage language, ReferenceProcessor r
         this(language, referenceProcessor.processingQueue);
     }
 
-    public DataObjectFinalizerReference addFinalizer(RubyContext context, Object object, Object callable,
-            DataHolder dataHolder) {
-        final DataObjectFinalizerReference newRef = createRef(object, callable, dataHolder);
+    public DataObjectFinalizerReference addFinalizer(RubyContext context, Object object, DataHolder dataHolder) {
+        final DataObjectFinalizerReference newRef = createRef(object, dataHolder);
 
         add(newRef);
         context.getReferenceProcessor().processReferenceQueue(this);
 
         return newRef;
     }
 
-    public DataObjectFinalizerReference createRef(Object object, Object callable, DataHolder dataHolder) {
-        return new DataObjectFinalizerReference(object, processingQueue, this, callable, dataHolder);
+    public DataObjectFinalizerReference createRef(Object object, DataHolder dataHolder) {
+        return new DataObjectFinalizerReference(object, processingQueue, this, dataHolder);
     }
 
     public final void drainFinalizationQueue(RubyContext context) {

diff --git a/src/main/java/org/truffleruby/core/DataObjectFinalizerReference.java b/src/main/java/org/truffleruby/core/DataObjectFinalizerReference.java
@@ -17,17 +17,14 @@ public class DataObjectFinalizerReference
         extends
         ReferenceProcessingService.PhantomProcessingReference<DataObjectFinalizerReference, Object> {
 
-    public final Object callable;
     public final DataHolder dataHolder;
 
     DataObjectFinalizerReference(
             Object object,
             ReferenceQueue<? super Object> queue,
             DataObjectFinalizationService service,
-            Object callable,
             DataHolder dataHolder) {
         super(object, queue, service);
-        this.callable = callable;
         this.dataHolder = dataHolder;
     }
 }
diff --git a/src/main/java/org/truffleruby/core/objectspace/ObjectSpaceNodes.java b/src/main/java/org/truffleruby/core/objectspace/ObjectSpaceNodes.java
@@ -260,22 +260,20 @@ private void defineFinalizer(RubyDynamicObject object, Object finalizer) {
     public abstract static class DefineDataObjectFinalizerNode extends PrimitiveArrayArgumentsNode {
 
         @Specialization
-        protected Object defineFinalizer(
-                VirtualFrame frame, RubyDynamicObject object, Object finalizer, DataHolder dataHolder,
+        protected Object defineFinalizer(RubyDynamicObject object, DataHolder dataHolder,
                 @Cached WriteBarrierNode writeBarrierNode,
                 @CachedLibrary(limit = "1") DynamicObjectLibrary objectLibrary) {
             if (!getContext().getReferenceProcessor().processOnMainThread()) {
                 // Share the finalizer, as it might run on a different Thread
                 if (!getContext().getSharedObjects().isSharing()) {
                     startSharing();
                 }
-                writeBarrierNode.executeWriteBarrier(finalizer);
                 writeBarrierNode.executeWriteBarrier(dataHolder);
             }
 
             DataObjectFinalizerReference newRef = getContext()
                     .getDataObjectFinalizationService()
-                    .addFinalizer(getContext(), object, finalizer, dataHolder);
+                    .addFinalizer(getContext(), object, dataHolder);
 
             objectLibrary.put(object, Layouts.DATA_OBJECT_FINALIZER_REF_IDENTIFIER, newRef);
 

diff --git a/src/main/java/org/truffleruby/language/objects/shared/WriteBarrierNode.java b/src/main/java/org/truffleruby/language/objects/shared/WriteBarrierNode.java
@@ -96,7 +96,6 @@ protected void writeBarrierFinalizer(FinalizerReference ref) {
     @Specialization
     @TruffleBoundary
     protected void writeBarrierDataFinalizer(DataObjectFinalizerReference ref) {
-        SharedObjects.writeBarrier(getLanguage(), ref.callable);
         SharedObjects.writeBarrier(getLanguage(), ref.dataHolder);
     }