Merge pull request #139 from synecdoche/GR-63795

[Backport] [Oracle GraalVM] [GR-63795] Backport to 23.1: Verify that recurring callbacks are allocation free.

Author: zakkak

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/SubstrateJVM.java

@@ -24,7 +24,6 @@
  */
 package com.oracle.svm.core.jfr;
 
-import java.lang.reflect.Field;
 import java.util.List;
 
 import org.graalvm.compiler.api.replacements.Fold;
@@ -187,16 +186,6 @@ public static JfrLogging getJfrLogging() {
         return get().jfrLogging;
     }
 
-    public static Object getHandler(Class<? extends jdk.internal.event.Event> eventClass) {
-        try {
-            Field f = eventClass.getDeclaredField("eventHandler");
-            f.setAccessible(true);
-            return f.get(null);
-        } catch (NoSuchFieldException | IllegalArgumentException | IllegalAccessException e) {
-            throw new InternalError("Could not access event handler");
-        }
-    }
-
     @Uninterruptible(reason = "Prevent races with VM operations that start/stop recording.", callerMustBe = true)
     protected boolean isRecording() {
         return recording;

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Safepoint.java

@@ -180,17 +180,25 @@ private static long getSafepointPromptnessFailureNanos() {
         return Options.SafepointPromptnessFailureNanos.getValue().longValue();
     }
 
-    /**
-     * Used to wrap exceptions that are explicitly thrown by recurring callbacks.
-     */
-    static class SafepointException extends RuntimeException {
-        private static final long serialVersionUID = 1L;
-
-        final Throwable inner;
-
-        SafepointException(Throwable inner) {
-            this.inner = inner;
+    @Uninterruptible(reason = "Must not contain safepoint checks.")
+    private static void slowPathSafepointCheck(int newStatus, boolean callerHasJavaFrameAnchor, boolean popFrameAnchor) throws Throwable {
+        try {
+            slowPathSafepointCheck0(newStatus, callerHasJavaFrameAnchor, popFrameAnchor);
+        } catch (ThreadingSupportImpl.SafepointException e) {
+            /* This exception is intended to be thrown from safepoint checks, at one's own risk */
+            throw ThreadingSupportImpl.RecurringCallbackTimer.getAndClearPendingException();
+        } catch (Throwable ex) {
+            /*
+             * The foreign call from snippets to this method does not have an exception edge. So we
+             * could miss an exception handler if we unwind an exception from this method.
+             *
+             * Any exception coming out of a safepoint would be surprising to users. There is a good
+             * reason why Thread.stop() has been deprecated a long time ago (we do not support it on
+             * Substrate VM).
+             */
+            VMError.shouldNotReachHere(ex);
         }
+        exitSlowPathCheck();
     }
 
     /**
@@ -202,7 +210,7 @@ static class SafepointException extends RuntimeException {
      * </ul>
      **/
     @Uninterruptible(reason = "Must not contain safepoint checks.")
-    private static void slowPathSafepointCheck(int newStatus, boolean callerHasJavaFrameAnchor, boolean popFrameAnchor) {
+    private static void slowPathSafepointCheck0(int newStatus, boolean callerHasJavaFrameAnchor, boolean popFrameAnchor) {
         final IsolateThread myself = CurrentIsolate.getCurrentThread();
 
         SafepointListenerSupport.singleton().beforeSlowpathSafepointCheck();
@@ -412,32 +420,9 @@ public static void slowPathSafepointCheck() throws Throwable {
             Safepoint.setSafepointRequested(THREAD_REQUEST_RESET);
             return;
         }
-        VMError.guarantee(StatusSupport.isStatusJava(), "Attempting to do a safepoint check when not in Java mode");
 
-        try {
-            /*
-             * Block on mutex held by thread that requested safepoint, i.e., transition to native
-             * code.
-             */
-            slowPathSafepointCheck(StatusSupport.STATUS_IN_JAVA, false, false);
-
-        } catch (SafepointException se) {
-            /* This exception is intended to be thrown from safepoint checks, at one's own risk */
-            throw se.inner;
-
-        } catch (Throwable ex) {
-            /*
-             * The foreign call from snippets to this method does not have an exception edge. So we
-             * could miss an exception handler if we unwind an exception from this method.
-             *
-             * Any exception coming out of a safepoint would be surprising to users. There is a good
-             * reason why Thread.stop() has been deprecated a long time ago (we do not support it on
-             * Substrate VM).
-             */
-            VMError.shouldNotReachHere(ex);
-        }
-
-        exitSlowPathCheck();
+        VMError.guarantee(StatusSupport.isStatusJava(), "Attempting to do a safepoint check when not in Java mode");
+        slowPathSafepointCheck(StatusSupport.STATUS_IN_JAVA, false, false);
     }
 
     @Uninterruptible(reason = "Must not contain safepoint checks")
@@ -544,7 +529,7 @@ public static void transitionVMToNative() {
      */
     @SubstrateForeignCallTarget(stubCallingConvention = false)
     @Uninterruptible(reason = "Must not contain safepoint checks")
-    private static void enterSlowPathTransitionFromNativeToNewStatus(int newStatus, boolean popFrameAnchor) {
+    private static void enterSlowPathTransitionFromNativeToNewStatus(int newStatus, boolean popFrameAnchor) throws Throwable {
         VMError.guarantee(StatusSupport.isStatusNativeOrSafepoint(), "Must either be at a safepoint or in native mode");
         VMError.guarantee(!SafepointBehavior.ignoresSafepoints(),
                         "The safepoint handling doesn't change the status of threads that ignore safepoints. So, the fast path transition must succeed and this slow path must not be called");

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/ThreadingSupportImpl.java

@@ -25,8 +25,10 @@
 package com.oracle.svm.core.thread;
 
 import static com.oracle.svm.core.SubstrateOptions.MultiThreaded;
+import static com.oracle.svm.core.heap.RestrictHeapAccess.Access.NO_ALLOCATION;
 import static com.oracle.svm.core.thread.ThreadingSupportImpl.Options.SupportRecurringCallback;
 
+import java.io.Serial;
 import java.util.concurrent.TimeUnit;
 
 import org.graalvm.compiler.api.replacements.Fold;
@@ -42,7 +44,6 @@
 import com.oracle.svm.core.heap.RestrictHeapAccess;
 import com.oracle.svm.core.option.HostedOptionKey;
 import com.oracle.svm.core.option.SubstrateOptionsParser;
-import com.oracle.svm.core.thread.Safepoint.SafepointException;
 import com.oracle.svm.core.thread.VMThreads.ActionOnTransitionToJavaSupport;
 import com.oracle.svm.core.thread.VMThreads.StatusSupport;
 import com.oracle.svm.core.threadlocal.FastThreadLocalFactory;
@@ -69,12 +70,8 @@ public static class Options {
      * adapt to a changing frequency of safepoint checks in the code that the thread executes.
      */
     public static class RecurringCallbackTimer {
-        private static final RecurringCallbackAccess CALLBACK_ACCESS = new RecurringCallbackAccess() {
-            @Override
-            public void throwException(Throwable t) {
-                throw new SafepointException(t);
-            }
-        };
+        private static final FastThreadLocalObject<Throwable> EXCEPTION_TL = FastThreadLocalFactory.createObject(Throwable.class, "RecurringCallbackTimer.exception");
+        private static final RecurringCallbackAccess CALLBACK_ACCESS = new RecurringCallbackAccessImpl();
 
         /**
          * Weight of the newest sample in {@link #ewmaChecksPerNano}. Older samples have a total
@@ -107,6 +104,14 @@ public void throwException(Throwable t) {
             this.requestedChecks = INITIAL_CHECKS;
         }
 
+        @Uninterruptible(reason = "Prevent recurring callback execution.", callerMustBe = true)
+        public static Throwable getAndClearPendingException() {
+            Throwable t = EXCEPTION_TL.get();
+            VMError.guarantee(t != null, "There must be a recurring callback exception pending.");
+            EXCEPTION_TL.set(null);
+            return t;
+        }
+
         @Uninterruptible(reason = "Must not contain safepoint checks.")
         void evaluate() {
             updateStatistics();
@@ -208,24 +213,42 @@ private boolean isCallbackDisabled() {
         }
 
         /**
-         * Separate method to invoke {@link #callback} so that {@link #evaluate()} can be strictly
-         * {@link Uninterruptible} and allocation-free.
+         * Recurring callbacks may be executed in any method that contains a safepoint check. This
+         * includes methods that need to be allocation free. Therefore, recurring callbacks must not
+         * allocate any Java heap memory.
          */
         @Uninterruptible(reason = "Required by caller, but does not apply to callee.", calleeMustBe = false)
-        @RestrictHeapAccess(reason = "Callee may allocate", access = RestrictHeapAccess.Access.UNRESTRICTED)
+        @RestrictHeapAccess(reason = "Recurring callbacks must not allocate.", access = NO_ALLOCATION)
         private void invokeCallback() {
             try {
                 callback.run(CALLBACK_ACCESS);
-            } catch (SafepointException se) {
-                throw se;
+            } catch (SafepointException e) {
+                throw e;
             } catch (Throwable t) {
                 /*
-                 * Recurring callbacks are specified to ignore all exceptions. We cannot even log
-                 * the exception because that could lead to a StackOverflowError (especially when
-                 * the recurring callback failed with a StackOverflowError).
+                 * Recurring callbacks are specified to ignore exceptions (except if the exception
+                 * is thrown via RecurringCallbackAccess.throwException(), which is handled above).
+                 * We cannot even log the exception because that could lead to a StackOverflowError
+                 * (especially when the recurring callback failed with a StackOverflowError).
                  */
             }
         }
+
+        /**
+         * We need to distinguish between arbitrary exceptions (must be swallowed) and exceptions
+         * that are thrown via {@link RecurringCallbackAccess#throwException} (must be forwarded to
+         * the application). When a recurring callback uses
+         * {@link RecurringCallbackAccess#throwException}, we store the exception in a thread local
+         * (to avoid allocations) and throw a pre-allocated marker exception instead. We catch the
+         * marker exception internally, accesses the thread local, and rethrow that exception.
+         */
+        private static final class RecurringCallbackAccessImpl implements RecurringCallbackAccess {
+            @Override
+            public void throwException(Throwable t) {
+                EXCEPTION_TL.set(t);
+                throw SafepointException.SINGLETON;
+            }
+        }
     }
 
     private static final FastThreadLocalObject<RecurringCallbackTimer> activeTimer = FastThreadLocalFactory.createObject(RecurringCallbackTimer.class, "ThreadingSupportImpl.activeTimer");
@@ -332,9 +355,9 @@ static boolean needsNativeToJavaSlowpath() {
     }
 
     /**
-     * Recurring callbacks execute arbitrary code and can throw {@link SafepointException}s. In some
-     * code parts (e.g., when executing VM operations), we can't deal with arbitrary code execution
-     * and therefore need to pause the execution of recurring callbacks.
+     * Recurring callbacks execute arbitrary code and may throw exceptions. In some code parts
+     * (e.g., when executing VM operations), we can't deal with arbitrary code execution and
+     * therefore need to pause the execution of recurring callbacks.
      */
     @Uninterruptible(reason = "Must not contain safepoint checks.")
     public static void pauseRecurringCallback(@SuppressWarnings("unused") String reason) {
@@ -370,11 +393,17 @@ public static void resumeRecurringCallbackAtNextSafepoint() {
      */
     public static void resumeRecurringCallback() {
         if (resumeCallbackExecution()) {
-            try {
-                onSafepointCheckSlowpath();
-            } catch (SafepointException e) {
-                throwUnchecked(e.inner); // needed: callers cannot declare `throws Throwable`
-            }
+            maybeExecuteRecurringCallback();
+        }
+    }
+
+    @Uninterruptible(reason = "Prevent safepoints.")
+    private static void maybeExecuteRecurringCallback() {
+        try {
+            onSafepointCheckSlowpath();
+        } catch (SafepointException e) {
+            /* Callers cannot declare `throws Throwable`. */
+            throwUnchecked(RecurringCallbackTimer.getAndClearPendingException());
         }
     }
 
@@ -407,7 +436,18 @@ public static boolean isRecurringCallbackSupported() {
     }
 
     @SuppressWarnings("unchecked")
+    @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     private static <T extends Throwable> void throwUnchecked(Throwable exception) throws T {
         throw (T) exception; // T is inferred as RuntimeException, but doesn't have to be
     }
+
+    static final class SafepointException extends RuntimeException {
+        public static final SafepointException SINGLETON = new SafepointException();
+
+        @Serial //
+        private static final long serialVersionUID = 1L;
+
+        private SafepointException() {
+        }
+    }
 }

substratevm/src/org.graalvm.polyglot.nativeapi/src/org/graalvm/polyglot/nativeapi/PolyglotNativeAPI.java

@@ -2413,20 +2413,17 @@ private static PolyglotStatus doRegisterRecurringCallback0(long intervalNanos, P
         PolyglotCallbackInfoInternal info = new PolyglotCallbackInfoInternal(new ObjectHandle[0], data);
         var infoHandle = (PolyglotCallbackInfo) objectHandles.create(info);
         var handles = getHandles();
-        RecurringCallback recurringCallback = new RecurringCallback() {
-            @Override
-            public void run(Threading.RecurringCallbackAccess access) {
-                handles.freezeHandleCreation();
-                try {
-                    callback.invoke((PolyglotIsolateThread) CurrentIsolate.getCurrentThread(), infoHandle);
-                    CallbackException ce = threadLocals.get().callbackException;
-                    if (ce != null) {
-                        access.throwException(ce);
-                    }
-                } finally {
-                    threadLocals.get().callbackException = null;
-                    handles.unfreezeHandleCreation();
+        RecurringCallback recurringCallback = access -> {
+            handles.freezeHandleCreation();
+            try {
+                callback.invoke((PolyglotIsolateThread) CurrentIsolate.getCurrentThread(), infoHandle);
+                CallbackException ce = threadLocals.get().callbackException;
+                if (ce != null) {
+                    access.throwException(ce);
                 }
+            } finally {
+                threadLocals.get().callbackException = null;
+                handles.unfreezeHandleCreation();
             }
         };
         try {