[GR-61335] [GR-61393] Fix missing synchronization in statistics listener. Fix transient safepoint poll failure.

PullRequest: graal/19792

Author: chumer

substratevm/src/com.oracle.svm.truffle/src/com/oracle/svm/truffle/api/SubstrateThreadLocalHandshake.java

@@ -52,7 +52,6 @@
 import com.oracle.svm.core.threadlocal.FastThreadLocalInt;
 import com.oracle.svm.core.threadlocal.FastThreadLocalObject;
 import com.oracle.svm.core.util.VMError;
-import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.impl.ThreadLocalHandshake;
 import com.oracle.truffle.api.nodes.Node;
 
@@ -140,12 +139,7 @@ public TruffleSafepointImpl getCurrent() {
         if (SubstrateUtil.HOSTED) {
             return HOSTED_STATE.get();
         } else {
-            TruffleSafepointImpl state = STATE.get();
-            if (state == null) {
-                throw CompilerDirectives.shouldNotReachHere("Thread local handshake is not initialized for this thread. " +
-                                "Did you call getCurrent() outside while a polyglot context not entered?");
-            }
-            return state;
+            return STATE.get();
         }
     }
 

truffle/src/com.oracle.truffle.api.test/src/com/oracle/truffle/api/test/TruffleSafepointTest.java

@@ -1703,6 +1703,56 @@ public boolean call(TestSetup s, TestRootNode node) {
         }
     }
 
+    /*
+     * Test for GR-61393. We enter and schedule a thread local action on the current thread. Then we
+     * spawn a thread that polls safepoints which is not entered. So TruffleSafepoint.getCurrent()
+     * is not initialized. We should still be able to poll safepoints there. In the default
+     * handshake implementation this did fail because DefaultThreadLocalHandshake.PENDING_COUNT is
+     * global.
+     */
+    @Test
+    public void testTruffleSafepointNotEnteredThread() throws Throwable {
+        try (Context c = createTestContext()) {
+            c.enter();
+            c.initialize(ProxyLanguage.ID);
+            Env env = LanguageContext.get(null).getEnv();
+
+            env.submitThreadLocal(null, new ThreadLocalAction(false, false) {
+                @Override
+                protected void perform(Access access) {
+                    // don't ever process this action
+                }
+            });
+
+            AtomicReference<Throwable> e = new AtomicReference<>();
+            Thread t = new Thread(() -> {
+                try {
+                    AbstractPolyglotTest.assertFails(() -> TruffleSafepoint.getCurrent(), Throwable.class, (ex) -> {
+                        /*
+                         * If we build this on an older SVM version (23.1) then we might still get
+                         * an AssertionError here.
+                         */
+                        assertTrue(ex instanceof AssertionError || ex instanceof IllegalStateException);
+                    });
+
+                    // no exception, just ignored
+                    TruffleSafepoint.poll(null);
+                    TruffleSafepoint.pollHere(null);
+                } catch (Throwable error) {
+                    e.set(error);
+                }
+            });
+
+            t.start();
+            t.join();
+
+            Throwable error = e.get();
+            if (error != null) {
+                throw error;
+            }
+        }
+    }
+
     @Ignore("GR-55104: transiently hangs")
     @Test
     public void testDeadlockDueToTooFewCarrierThreads() {

truffle/src/com.oracle.truffle.api/src/com/oracle/truffle/api/TruffleSafepoint.java

@@ -124,7 +124,8 @@ protected TruffleSafepoint(EngineSupport support) {
      * Guest language exceptions may be thrown by this method. If
      * {@link #setAllowSideEffects(boolean) side-effects} are allowed then also guest language
      * exceptions may be thrown. Otherwise only internal or {@link ThreadDeath thread-death}
-     * exceptions may be thrown. This method is safe to be used on compiled code paths.
+     * exceptions may be thrown. This method is safe to be used on compiled code paths. Polling may
+     * be performed on threads without entered polyglot context, polls are ignored there.
      * <p>
      * Example usage with an unbounded loop sum behind a {@link TruffleBoundary}.
      *
@@ -238,7 +239,7 @@ public static void pollHere(Node location) {
      * TruffleSafepoint sp = TruffleSafepoint.getCurrent();
      * sp.setBlocked(location, Interrupter.THREAD_INTERRUPT, ReentrantLock::lockInterruptibly, lock, null, null);
      * </pre>
-     * 
+     *
      * @see TruffleSafepoint
      * @since 22.1
      * @deprecated Use
@@ -318,7 +319,7 @@ public abstract <T, R> R setBlockedFunction(Node location, Interrupter interrupt
      * The same as
      * {@link #setBlockedFunction(Node, Interrupter, InterruptibleFunction, Object, Runnable, Consumer)}.
      * The only difference is that the interruptible functional method does not return anything.
-     * 
+     *
      * @since 23.1
      */
     public abstract <T> void setBlocked(Node location, Interrupter interrupter, Interruptible<T> interruptible, T object, Runnable beforeInterrupt,
@@ -433,10 +434,15 @@ public static <T, R> R setBlockedThreadInterruptibleFunction(Node location, Inte
      * Important: The result of this method must not be stored or used on a different thread than
      * the current thread.
      *
+     * @throws IllegalStateException if the current thread is not entered with a polyglot context.
      * @since 21.1
      */
     public static TruffleSafepoint getCurrent() {
-        return HANDSHAKE.getCurrent();
+        TruffleSafepoint t = HANDSHAKE.getCurrent();
+        if (t == null) {
+            throw new IllegalStateException("The TruffleSafepoint mechanism is not initialized on this thread. Did you call getCurrent() while a polyglot context is not entered?");
+        }
+        return t;
     }
 
     /**

truffle/src/com.oracle.truffle.api/src/com/oracle/truffle/api/impl/DefaultThreadLocalHandshake.java

@@ -42,7 +42,6 @@
 
 import java.util.concurrent.atomic.AtomicInteger;
 
-import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.nodes.Node;
 
 final class DefaultThreadLocalHandshake extends ThreadLocalHandshake {
@@ -74,12 +73,7 @@ public void poll(Node enclosingNode) {
 
     @Override
     public TruffleSafepointImpl getCurrent() {
-        TruffleSafepointImpl state = STATE.get();
-        if (state == null) {
-            throw CompilerDirectives.shouldNotReachHere("Thread local handshake is not initialized for this thread. " +
-                            "Did you call getCurrent() outside while a polyglot context not entered?");
-        }
-        return state;
+        return STATE.get();
     }
 
     @Override

truffle/src/com.oracle.truffle.api/src/com/oracle/truffle/api/impl/ThreadLocalHandshake.java

@@ -170,7 +170,7 @@ public void ensureThreadInitialized() {
     @TruffleBoundary
     protected final void processHandshake(Node location) {
         TruffleSafepointImpl s = getCurrent();
-        if (s.fastPendingSet) {
+        if (s != null && s.fastPendingSet) {
             s.processOrNotifyHandshakes(location, s.takeHandshakes(), null);
         }
     }
@@ -616,9 +616,9 @@ void verifyUnused() throws AssertionError {
         void processOrNotifyHandshakes(Node location, List<HandshakeEntry> toProcessOrNotify, Boolean blockedNotification) {
             /*
              * blockedNotification == null -> claim and process handshakes
-             * 
+             *
              * blockedNotification == TRUE -> just notify handshakes blocked, don't claim them
-             * 
+             *
              * blockedNotification == FALSE -> just notify handshakes unblocked, don't claim them
              */
             if (toProcessOrNotify == null) {

truffle/src/com.oracle.truffle.runtime/src/com/oracle/truffle/runtime/debug/StatisticsListener.java

@@ -190,7 +190,7 @@ public synchronized void onCompilationInvalidated(OptimizedCallTarget target, Ob
     }
 
     @Override
-    public void onCompilationStarted(OptimizedCallTarget target, AbstractCompilationTask task) {
+    public synchronized void onCompilationStarted(OptimizedCallTarget target, AbstractCompilationTask task) {
         compilations++;
         final CurrentCompilationStatistics times = new CurrentCompilationStatistics(task.tier());
         currentCompilationStatistics.set(times);
@@ -291,7 +291,7 @@ public synchronized void onCompilationSuccess(OptimizedCallTarget target, Abstra
     }
 
     @Override
-    public void onCompilationFailed(OptimizedCallTarget target, String reason, boolean bailout, boolean permanentBailout, int tier, Supplier<String> lazyStackTrace) {
+    public synchronized void onCompilationFailed(OptimizedCallTarget target, String reason, boolean bailout, boolean permanentBailout, int tier, Supplier<String> lazyStackTrace) {
         if (bailout) {
             if (permanentBailout) {
                 permanentBailouts++;
@@ -309,7 +309,7 @@ public void onCompilationFailed(OptimizedCallTarget target, String reason, boole
     }
 
     @Override
-    public void onEngineClosed(EngineData runtimeData) {
+    public synchronized void onEngineClosed(EngineData runtimeData) {
         printStatistics(runtimeData);
     }
 

truffle/src/com.oracle.truffle.runtime/src/com/oracle/truffle/runtime/hotspot/HotSpotThreadLocalHandshake.java

@@ -46,6 +46,7 @@
 import com.oracle.truffle.api.impl.ThreadLocalHandshake;
 import com.oracle.truffle.api.nodes.Node;
 import com.oracle.truffle.runtime.ModulesSupport;
+
 import jdk.vm.ci.common.JVMCIError;
 import jdk.vm.ci.hotspot.HotSpotJVMCIRuntime;
 import jdk.vm.ci.hotspot.HotSpotVMConfigAccess;
@@ -98,12 +99,7 @@ static void doHandshake(Object node) {
     @Override
     @TruffleBoundary
     public TruffleSafepointImpl getCurrent() {
-        TruffleSafepointImpl state = STATE.get();
-        if (state == null) {
-            throw CompilerDirectives.shouldNotReachHere("Thread local handshake is not initialized for this thread. " +
-                            "Did you call getCurrent() outside while a polyglot context not entered?");
-        }
-        return state;
+        return STATE.get();
     }
 
     @Override