[GR-45324] [GR-45325] Stability fixes of Breakpoint.

PullRequest: graal/14244

Author: entlicher

truffle/src/com.oracle.truffle.api.debug.test/src/com/oracle/truffle/api/debug/test/BreakpointTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2023, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * The Universal Permissive License (UPL), Version 1.0
@@ -51,10 +51,13 @@
 import java.io.File;
 import java.net.URI;
 import java.util.List;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 
 import org.graalvm.polyglot.Context;
 import org.graalvm.polyglot.Source;
@@ -973,6 +976,94 @@ public void testGlobalBreakpointsInMultipleSessions() throws Throwable {
 
     }
 
+    /**
+     * Runs repeated evaluations of the provided Source in a separate thread, until join() is
+     * called.
+     */
+    private static final class EvalLoop {
+
+        private final AtomicBoolean evaluating = new AtomicBoolean(true);
+        private final Thread evalLoop;
+        private final AtomicReference<Throwable> evalThrowable = new AtomicReference<>();
+
+        EvalLoop(DebuggerTester tester, Source source) {
+            evalLoop = new Thread(() -> {
+                while (evaluating.get()) {
+                    tester.startEval(source);
+                    boolean suspended;
+                    do {
+                        suspended = false;
+                        try {
+                            tester.expectDone();
+                        } catch (AssertionError ae) {
+                            if (ae.getCause() != null && ae.getCause().getMessage().startsWith("Expected done but got event Suspended at")) {
+                                // We've hit the breakpoint. We'll resume automatically.
+                                suspended = true;
+                            } else {
+                                throw ae;
+                            }
+                        }
+                    } while (suspended);
+                }
+            });
+            evalLoop.setUncaughtExceptionHandler((thread, thrwbl) -> evalThrowable.set(thrwbl));
+            evalLoop.start();
+        }
+
+        void join() throws InterruptedException, ExecutionException {
+            evaluating.set(false);
+            evalLoop.join();
+            if (evalThrowable.get() != null) {
+                throw new ExecutionException(evalThrowable.get());
+            }
+        }
+    }
+
+    @Test
+    public void testBreakpointInstallDuringDispose() throws InterruptedException, ExecutionException {
+        final Source source = testSource("ROOT(\n" +
+                        "  LOOP(1000,\n" +
+                        "    STATEMENT)\n" + // break here
+                        ")\n");
+        try (DebuggerSession session = startSession()) {
+            EvalLoop evalLoop = new EvalLoop(tester, source);
+            ExecutorService exec = Executors.newSingleThreadExecutor();
+            for (int repeatCount = 0; repeatCount <= 1000; repeatCount++) {
+                Breakpoint breakpoint = Breakpoint.newBuilder(getSourceImpl(source)).lineIs(3).build();
+                breakpoint.setEnabled(false);
+                session.install(breakpoint);
+                Future<?> installation = exec.submit(() -> {
+                    breakpoint.setEnabled(true);
+                });
+                Thread.sleep(0, repeatCount % 100);
+                breakpoint.dispose();
+                installation.get();
+            }
+            exec.shutdown();
+            // We want the test infrastructure to interrupt this to see the full thread dump.
+            exec.awaitTermination(1, TimeUnit.DAYS);
+            evalLoop.join();
+        }
+    }
+
+    @Test
+    public void testBreakpointParallelInstallDispose() throws InterruptedException, ExecutionException {
+        // Install and dispose of breakpoints parallel with the execution.
+        final Source source = testSource("ROOT(\n" +
+                        "  LOOP(1000,\n" +
+                        "    STATEMENT)\n" + // break here
+                        ")\n");
+        try (DebuggerSession session = startSession()) {
+            EvalLoop evalLoop = new EvalLoop(tester, source);
+            for (int repeatCount = 0; repeatCount <= 100; repeatCount++) {
+                Breakpoint breakpoint = Breakpoint.newBuilder(getSourceImpl(source)).lineIs(3).build();
+                session.install(breakpoint);
+                breakpoint.dispose();
+            }
+            evalLoop.join();
+        }
+    }
+
     @Test
     public void testResolveListener() {
         final Source source = testSource("ROOT(\n" +

truffle/src/com.oracle.truffle.api.debug/src/com/oracle/truffle/api/debug/Breakpoint.java

@@ -66,6 +66,7 @@
 import com.oracle.truffle.api.instrumentation.ExecuteSourceListener;
 import com.oracle.truffle.api.instrumentation.ExecutionEventNode;
 import com.oracle.truffle.api.instrumentation.ExecutionEventNodeFactory;
+import com.oracle.truffle.api.instrumentation.Instrumenter;
 import com.oracle.truffle.api.instrumentation.LoadSourceSectionEvent;
 import com.oracle.truffle.api.instrumentation.LoadSourceSectionListener;
 import com.oracle.truffle.api.instrumentation.SourceFilter;
@@ -198,7 +199,7 @@ public enum Kind {
 
     private final AtomicReference<EventBinding<?>> sourceBinding = new AtomicReference<>();
     private final List<EventBinding<? extends ExecutionEventNodeFactory>> execBindings = new ArrayList<>();
-    private LocationsInExecutedSources locationsInExecutedSources;
+    private volatile LocationsInExecutedSources locationsInExecutedSources;
     private volatile boolean breakpointBindingReady;
 
     Breakpoint(BreakpointLocation key, SuspendAnchor suspendAnchor) {
@@ -273,6 +274,7 @@ public boolean isEnabled() {
      */
     public void setEnabled(boolean enabled) {
         boolean doInstall = false;
+        Debugger d;
         synchronized (this) {
             if (disposed) {
                 // cannot enable disposed breakpoints
@@ -284,10 +286,13 @@ public void setEnabled(boolean enabled) {
                 }
                 this.enabled = enabled;
             }
+            d = debugger;
         }
         if (doInstall) {
             if (enabled) {
-                install();
+                if (d != null) {
+                    installInstrumentation(d.getInstrumenter());
+                } // else when debugger == null, the breakpoint was disposed.
             } else {
                 uninstall();
             }
@@ -371,9 +376,9 @@ public void dispose() {
         DebuggerSession[] breakpointSessions = null;
         Debugger breakpointDebugger = null;
         LocationsInExecutedSources locations = null;
+        setEnabled(false);
         synchronized (this) {
             if (!disposed) {
-                setEnabled(false);
                 breakpointSessions = sessions.toArray(new DebuggerSession[sessions.size()]);
                 breakpointDebugger = debugger;
                 debugger = null;
@@ -511,6 +516,7 @@ synchronized void installGlobal(Debugger d) {
     }
 
     private void install(Debugger d) {
+        assert d != null;
         assert Thread.holdsLock(this);
         if (this.debugger != null && this.debugger != d) {
             throw new IllegalStateException("Breakpoint is already installed in a different Debugger instance.");
@@ -542,18 +548,19 @@ boolean install(DebuggerSession d, boolean failOnError) {
             install(d.getDebugger());
         }
         if (enabled) {
-            install();
+            installInstrumentation(d.getDebugger().getInstrumenter());
         }
         return true;
     }
 
-    private void install() {
+    private void installInstrumentation(Instrumenter instrumenter) {
         EventBinding<?> binding = sourceBinding.get();
         if (binding == null || binding.isDisposed()) {
             BreakpointLocation.LocationFilters filters = locationKey.createLocationFilters(suspendAnchor);
             if (locationKey.isLoadBindingNeeded()) {
-                locationsInExecutedSources = new LocationsInExecutedSources();
-                EventBinding<?> loadBinding = debugger.getInstrumenter().createLoadSourceSectionBinding(filters.nearestFilter, filters.sectionFilter, locationsInExecutedSources, true);
+                LocationsInExecutedSources locations = new LocationsInExecutedSources();
+                locationsInExecutedSources = locations;
+                EventBinding<?> loadBinding = instrumenter.createLoadSourceSectionBinding(filters.nearestFilter, filters.sectionFilter, locations, true);
                 if (sourceBinding.compareAndSet(null, loadBinding)) {
                     try {
                         loadBinding.attach();
@@ -569,7 +576,7 @@ private void install() {
                 }
                 if (needExecBinding) {
                     EventBinding<? extends ExecutionEventNodeFactory> execBinding;
-                    execBinding = debugger.getInstrumenter().attachExecutionEventFactory(filters.nearestFilter, filters.sectionFilter, new BreakpointNodeFactory());
+                    execBinding = instrumenter.attachExecutionEventFactory(filters.nearestFilter, filters.sectionFilter, new BreakpointNodeFactory());
                     execBindingAdded(execBinding);
                 }
             }
@@ -591,6 +598,10 @@ public void onLoad(LoadSourceSectionEvent event) {
             boolean doAssign;
             EventBinding<?> execBinding = null;
             synchronized (this) {
+                if (debugger == null) {
+                    // disposed
+                    return;
+                }
                 if (!(doAssign = executedSources.contains(source))) {
                     SourceSection oldSection = loadedSections.put(source, section);
                     if (oldSection == null) {