refactor Thread#backtrace_locations and Exception#backtrace_locations to use Backtrace#getBacktraceLocations

Author: Nicolas Laurent

src/main/java/org/truffleruby/core/exception/ExceptionNodes.java

@@ -9,7 +9,6 @@
  */
 package org.truffleruby.core.exception;
 
-import com.oracle.truffle.api.object.DynamicObjectFactory;
 import org.truffleruby.Layouts;
 import org.truffleruby.builtins.CoreModule;
 import org.truffleruby.builtins.CoreMethod;
@@ -182,23 +181,14 @@ private ReadObjectFieldNode getReadCustomBacktraceNode() {
     public abstract static class BacktraceLocationsNode extends CoreMethodArrayArgumentsNode {
 
         @Specialization
-        protected Object backtrace_locations(DynamicObject exception,
-                @Cached("createBinaryProfile()") ConditionProfile hasBacktraceProfile,
-                 @Cached("createBinaryProfile()") ConditionProfile hasLocationsProfile) {
+        protected Object backtraceLocations(DynamicObject exception,
+                 @Cached("createBinaryProfile()") ConditionProfile hasBacktraceProfile,
+                @Cached("createBinaryProfile()") ConditionProfile hasLocationsProfile) {
             if (hasBacktraceProfile.profile(Layouts.EXCEPTION.getBacktrace(exception) != null)) {
                 DynamicObject backtraceLocations = Layouts.EXCEPTION.getBacktraceLocations(exception);
                 if (hasLocationsProfile.profile(backtraceLocations == null)) {
                     Backtrace backtrace = Layouts.EXCEPTION.getBacktrace(exception);
-                    int locationsCount = backtrace.getActivations().length;
-                    Object[] locations = new Object[locationsCount];
-                    DynamicObjectFactory factory = coreLibrary().threadBacktraceLocationFactory;
-                    for (int i = 0; i < locationsCount; i++) {
-                        locations[i] = Layouts.THREAD_BACKTRACE_LOCATION.createThreadBacktraceLocation(
-                                factory,
-                                backtrace,
-                                i);
-                    }
-                    backtraceLocations = createArray(locations, locations.length);
+                    backtraceLocations = backtrace.getBacktraceLocations(GetBacktraceException.UNLIMITED);
                     Layouts.EXCEPTION.setBacktraceLocations(exception, backtraceLocations);
                 }
                 return backtraceLocations;

src/main/java/org/truffleruby/core/kernel/KernelNodes.java

@@ -30,7 +30,6 @@
 import org.truffleruby.builtins.Primitive;
 import org.truffleruby.builtins.PrimitiveArrayArgumentsNode;
 import org.truffleruby.builtins.UnaryCoreMethodNode;
-import org.truffleruby.core.array.ArrayHelpers;
 import org.truffleruby.core.array.ArrayStrategy;
 import org.truffleruby.core.array.ArrayUtils;
 import org.truffleruby.core.basicobject.BasicObjectNodes.ObjectIDNode;

src/main/java/org/truffleruby/core/thread/ThreadNodes.java

@@ -43,7 +43,6 @@
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 
-import com.oracle.truffle.api.object.DynamicObjectFactory;
 import org.jcodings.specific.USASCIIEncoding;
 import org.jcodings.specific.UTF8Encoding;
 import org.truffleruby.Layouts;
@@ -155,64 +154,28 @@ protected DynamicObject backtraceLocations(
         @TruffleBoundary
         private DynamicObject backtraceLocationsInternal(DynamicObject rubyThread, int omit, int length) {
             final Memo<Backtrace> backtraceMemo = new Memo<>(null);
-            final Memo<Integer> stackTraceLengthMemo = new Memo<>(null);
+
+            // We can't set an effective limit when dealing with negative range endings.
+            final int stackTraceElementsLimit = length < 0
+                ? GetBacktraceException.UNLIMITED
+                : omit + length;
 
             final SafepointAction safepointAction = (thread1, currentNode) -> {
                 final Backtrace backtrace = getContext().getCallStack().getBacktrace(this, omit);
                 backtraceMemo.set(backtrace);
-
-                // NOTE (norswap, 18 Dec 2019)
-                //   This is not entirely accurate, as a negative length does entail a limit.
-                //   However this limit can only determined by looking at the length of full
-                //   stack trace, which necessitates passing an exception with the limit set.
-                //   However, that field is only used to fill in the stack trace in the first place.
-                final int stackTraceElementsLimit = length < 0
-                        ? GetBacktraceException.UNLIMITED
-                        : omit + length;
-
-                final GetBacktraceException exception = new GetBacktraceException(this, stackTraceElementsLimit);
-
-                // NOTE(norswap, 20 Dec 2019)
-                //   This will also cause the stack trace to be filled.
-                //   We must call this rather than TruffleStackTrace#getStackTrace because
-                //   it does some additional filtering.
-                // TODO(norswap, 20 Dec 2019)
-                //   Currently, only the filtering at the end is taken into account (as length reduction).
-                //   Filtering in the middle will be ignored because of how we build backtrace locations.
-                // TODO (norswap, 24 Dec 2019)
-                //    Could we actually move this out of the safepointAction?
-                stackTraceLengthMemo.set(backtrace.getActivations(exception).length);
+                // Fill in the stack trace.
+                backtrace.getActivations(new GetBacktraceException(this, stackTraceElementsLimit));
             };
 
-            getContext()
-                    .getSafepointManager()
-                    .pauseRubyThreadAndExecute(rubyThread, this, safepointAction);
+            getContext().getSafepointManager()
+                .pauseRubyThreadAndExecute(rubyThread, this, safepointAction);
 
             // If the thread is dead or aborting the SafepointAction will not run.
-            if (backtraceMemo.get() == null || stackTraceLengthMemo.get() == null)
-                return nil();
-
-            final Backtrace backtrace = backtraceMemo.get();
-            final int stackTraceLength = stackTraceLengthMemo.get();
-
-            if (omit > stackTraceLength) {
+            if (backtraceMemo.get() == null) {
                 return nil();
             }
 
-            // NOTE (norswap, 18 Dec 2019)
-            //  TruffleStackTrace#getStackTrace (hence Backtrace#getActivations too) does not
-            //  always respect the limit, so we need to use Math#min. Haven't yet investigated why.
-            final int locationsLimit = length < 0
-                ? stackTraceLength + 1 + length
-                : Math.min(stackTraceLength, length);
-
-            final Object[] locations = new Object[locationsLimit];
-            final DynamicObjectFactory factory = coreLibrary().threadBacktraceLocationFactory;
-            for (int i = 0; i < locationsLimit; i++) {
-                locations[i] = Layouts.THREAD_BACKTRACE_LOCATION.createThreadBacktraceLocation(
-                    factory, backtrace, i);
-            }
-            return createArray(locations, locations.length);
+            return backtraceMemo.get().getBacktraceLocations(length);
         }
     }