Fix ObjectSpace.trace_object_allocations

* It would cause a stack overflow, because we would create a String
  before setting `tracingPaused`.
* Make ObjectSpaceManager thread-safe.

Author: eregon

CHANGELOG.md

@@ -7,6 +7,7 @@ Bug fixes:
 * Removed extra `public` methods on `IO` (#1702).
 * Fixed `Process.kill(signal, Process.pid)` when the signal is trapped as `:IGNORE` (#1702).
 * Fixed `Addrinfo.new(String)` to reliably find the address family (#1702).
+* Fixed `ObjectSpace.trace_object_allocations` (#1456).
 
 Compatibility:
 

lib/truffle/objspace.rb

@@ -184,7 +184,6 @@ def reachable_objects_from_root
 
   def trace_object_allocations
     trace_object_allocations_start
-
     begin
       yield
     ensure

src/main/java/org/truffleruby/core/objectspace/ObjectSpaceManager.java

@@ -40,6 +40,7 @@
 
 import com.oracle.truffle.api.Assumption;
 import com.oracle.truffle.api.CompilerDirectives;
+import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
 import com.oracle.truffle.api.object.DynamicObject;
 import com.oracle.truffle.api.utilities.CyclicAssumption;
 
@@ -48,6 +49,7 @@
 
 import java.lang.management.GarbageCollectorMXBean;
 import java.lang.management.ManagementFactory;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicLong;
 
 /**
@@ -59,9 +61,9 @@ public class ObjectSpaceManager {
     private final RubyContext context;
 
     private final CyclicAssumption tracingAssumption = new CyclicAssumption("objspace-tracing");
-    @CompilerDirectives.CompilationFinal private boolean isTracing = false;
-    private int tracingAssumptionActivations = 0;
-    private boolean tracingPaused = false;
+    @CompilationFinal private boolean isTracing = false;
+    private final AtomicInteger tracingAssumptionActivations = new AtomicInteger(0);
+    private final ThreadLocal<Boolean> tracingPaused = ThreadLocal.withInitial(() -> false);
 
     private final AtomicLong nextObjectID = new AtomicLong(ValueWrapperManager.TAG_MASK + 1);
 
@@ -70,37 +72,19 @@ public ObjectSpaceManager(RubyContext context) {
     }
 
     public void traceAllocationsStart() {
-        tracingAssumptionActivations++;
-
-        if (tracingAssumptionActivations == 1) {
+        if (tracingAssumptionActivations.incrementAndGet() == 1) {
             isTracing = true;
             tracingAssumption.invalidate();
         }
     }
 
     public void traceAllocationsStop() {
-        tracingAssumptionActivations--;
-
-        if (tracingAssumptionActivations == 0) {
+        if (tracingAssumptionActivations.decrementAndGet() == 0) {
             isTracing = false;
             tracingAssumption.invalidate();
         }
     }
 
-    public void traceAllocation(DynamicObject object, DynamicObject classPath, DynamicObject methodId, DynamicObject sourcefile, int sourceline) {
-        if (tracingPaused) {
-            return;
-        }
-
-        tracingPaused = true;
-
-        try {
-            context.send(context.getCoreLibrary().getObjectSpaceModule(), "trace_allocation", object, classPath, methodId, sourcefile, sourceline, getCollectionCount());
-        } finally {
-            tracingPaused = false;
-        }
-    }
-
     public Assumption getTracingAssumption() {
         return tracingAssumption.getAssumption();
     }
@@ -109,6 +93,14 @@ public boolean isTracing() {
         return isTracing;
     }
 
+    public boolean isTracingPaused() {
+        return tracingPaused.get();
+    }
+
+    public void setTracingPaused(boolean tracingPaused) {
+        this.tracingPaused.set(tracingPaused);
+    }
+
     public long getNextObjectID() {
         final long id = nextObjectID.getAndAdd(ValueWrapperManager.TAG_MASK + 1);
 

src/main/java/org/truffleruby/language/objects/AllocateObjectNode.java

@@ -23,8 +23,9 @@
 import com.oracle.truffle.api.source.SourceSection;
 import org.jcodings.specific.UTF8Encoding;
 import org.truffleruby.Layouts;
-import org.truffleruby.core.rope.CodeRange;
+import org.truffleruby.core.objectspace.ObjectSpaceManager;
 import org.truffleruby.core.string.StringNodes;
+import org.truffleruby.core.string.StringOperations;
 import org.truffleruby.language.RubyBaseNode;
 import org.truffleruby.language.arguments.RubyArguments;
 import org.truffleruby.language.control.RaiseException;
@@ -81,10 +82,24 @@ public DynamicObject allocateUncached(DynamicObject classToAllocate, Object[] va
     @CompilerDirectives.TruffleBoundary
     @Specialization(guards = {"!isSingleton(classToAllocate)", "isTracing()"},
                     assumptions = "getTracingAssumption()")
-    public DynamicObject allocateTracing(DynamicObject classToAllocate, Object[] values,
-                                         @Cached("create()") StringNodes.MakeStringNode makeStringNode) {
+    public DynamicObject allocateTracing(DynamicObject classToAllocate, Object[] values) {
         final DynamicObject object = allocate(getInstanceFactory(classToAllocate), values);
 
+        final ObjectSpaceManager objectSpaceManager = getContext().getObjectSpaceManager();
+        if (!objectSpaceManager.isTracingPaused()) {
+            objectSpaceManager.setTracingPaused(true);
+            try {
+                callTraceAllocation(object);
+            } finally {
+                objectSpaceManager.setTracingPaused(false);
+            }
+        }
+
+        return object;
+    }
+
+    @CompilerDirectives.TruffleBoundary
+    private void callTraceAllocation(DynamicObject object) {
         final FrameInstance allocatingFrameInstance;
         final SourceSection allocatingSourceSection;
 
@@ -101,22 +116,23 @@ public DynamicObject allocateTracing(DynamicObject classToAllocate, Object[] val
         final Object allocatingSelf = RubyArguments.getSelf(allocatingFrame);
         final String allocatingMethod = RubyArguments.getMethod(allocatingFrame).getName();
 
-        getContext().getObjectSpaceManager().traceAllocation(
+        getContext().send(coreLibrary().getObjectSpaceModule(), "trace_allocation",
                 object,
-                string(makeStringNode, Layouts.CLASS.getFields(coreLibrary().getLogicalClass(allocatingSelf)).getName()),
+                string(Layouts.CLASS.getFields(coreLibrary().getLogicalClass(allocatingSelf)).getName()),
                 getSymbol(allocatingMethod),
-                string(makeStringNode, getContext().getPath(allocatingSourceSection.getSource())),
-                allocatingSourceSection.getStartLine());
-
-        return object;
+                string(getContext().getPath(allocatingSourceSection.getSource())),
+                allocatingSourceSection.getStartLine(),
+                ObjectSpaceManager.getCollectionCount());
     }
 
     protected DynamicObjectFactory getInstanceFactory(DynamicObject classToAllocate) {
         return Layouts.CLASS.getInstanceFactory(classToAllocate);
     }
 
-    private DynamicObject string(StringNodes.MakeStringNode makeStringNode, String value) {
-        return makeStringNode.executeMake(value, UTF8Encoding.INSTANCE, CodeRange.CR_UNKNOWN);
+    private DynamicObject string(String value) {
+        // No point to use MakeStringNode (which uses AllocateObjectNode) here, as we should not trace the allocation
+        // of Strings used for tracing allocations.
+        return StringOperations.createString(getContext(), StringOperations.encodeRope(value, UTF8Encoding.INSTANCE));
     }
 
     @Specialization(guards = "isSingleton(classToAllocate)")

test/mri/excludes/TestObjSpace.rb

@@ -19,7 +19,6 @@
 exclude :test_memsize_of_root_shared_string, "needs investigation"
 exclude :test_reachable_objects_size, "needs investigation"
 exclude :test_trace_object_allocations, "needs investigation"
-exclude :test_trace_object_allocations_start_stop_clear, "needs investigation"
 exclude :test_count_objects_size_with_wrong_type, "needs investigation"
 exclude :test_dump_addresses_match_dump_all_addresses, "needs investigation"
 exclude :test_dump_all_full, "needs investigation"