Skylark debugging protocol: include a 'stop reason' in ThreadPausedEvent.

Pulls out a ThreadPausedState message, containing the previously
separate 'isPaused' and 'locationIfPaused' information, as well as
the stack frames and the reason for pausing, which is useful for
debugging clients (e.g. IDEs might change focus when a breakpoint is
hit, but not when all threads are paused).

TAG_CHANGE_OK=This proto has never yet been used
TYPE_CHANGE_OK=This proto has never yet been used
PiperOrigin-RevId: 200109927

3 files changed, 71 insertions, 40 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto b/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto
index 649f2936e2..f81185f20a 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto
@@ -184,11 +184,6 @@ message ThreadEndedEvent {
 message ThreadPausedEvent {
   // The thread that was paused.
   Thread thread = 1;
-
-  // The list of stack frames for the paused thread. The first element in the
-  // list represents the topmost frame (that is, the current innermost
-  // function).
-  repeated Frame frame = 2;
 }
 
 // An event indicating that a thread has continued execution after being paused.
@@ -268,16 +263,45 @@ message Thread {
   // The identifier of the thread.
   int64 id = 1;
 
-  // Indicates whether the thread is paused (if true) or running (if false).
-  bool is_paused = 2;
-
-  // If the thread is paused, this field contains the location in Skylark code
-  // of the next statement or expression that will be executed.
-  Location location = 3;
+  // Present if the thread is paused, providing details such as the thread's
+  // location. Not set if the thread is running.
+  ThreadPausedState thread_paused_state = 2;
 
   // A descriptive name for the thread that can be displayed in the debugger's
   // UI.
-  string name = 4;
+  string name = 3;
+}
+
+// Information about a paused thread.
+message ThreadPausedState {
+  PauseReason pause_reason = 1;
+
+  // The location in Skylark code of the next statement or expression that will
+  // be executed.
+  Location location = 2;
+
+  // The list of stack frames for the paused thread. The first element in the
+  // list represents the topmost frame (that is, the current innermost
+  // function).
+  repeated Frame frame = 3;
+}
+
+// The reason why a thread was paused
+enum PauseReason {
+  // The debug server hasn't set a reason for pausing a thread.
+  UNSET = 0;
+
+  // The stepping condition in a ContinueExecutionRequest was hit.
+  STEPPING = 4;
+
+  // All threads are paused (e.g. prior to the initial StartDebuggingRequest).
+  ALL_THREADS_PAUSED = 1;
+
+  // Thread paused in response to a PauseThreadRequest.
+  PAUSE_THREAD_REQUEST = 2;
+
+  // A breakpoint was hit.
+  HIT_BREAKPOINT = 3;
 }
 
 // The debugger representation of a Skylark value.
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java
index aa2279f97d..0254484062 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java
@@ -130,9 +130,9 @@ final class DebugEventHelper {
         .build();
   }
 
-  static DebugEvent threadPausedEvent(Thread thread, Collection<Frame> frames) {
+  static DebugEvent threadPausedEvent(Thread thread) {
     return DebugEvent.newBuilder()
-        .setThreadPaused(ThreadPausedEvent.newBuilder().setThread(thread).addAllFrame(frames))
+        .setThreadPaused(ThreadPausedEvent.newBuilder().setThread(thread))
         .build();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java
index e1f74f9dd6..d1ccad25c6 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java
@@ -21,6 +21,8 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos;
+import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.PauseReason;
+import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.ThreadPausedState;
 import com.google.devtools.build.lib.syntax.Debuggable;
 import com.google.devtools.build.lib.syntax.Debuggable.ReadyToPause;
 import com.google.devtools.build.lib.syntax.Environment;
@@ -62,13 +64,17 @@ final class ThreadHandler {
   /** Information about a paused thread. */
   private static class PausedThreadState {
 
+    /** The reason execution was paused. */
+    final PauseReason pauseReason;
+
     /** The {@link Location} where execution is currently paused. */
     final Location location;
 
     /** Used to block execution of threads */
     final Semaphore semaphore;
 
-    PausedThreadState(Location location) {
+    PausedThreadState(PauseReason pauseReason, Location location) {
+      this.pauseReason = pauseReason;
       this.location = location;
       this.semaphore = new Semaphore(0);
     }
@@ -192,8 +198,9 @@ final class ThreadHandler {
   }
 
   void pauseIfNecessary(Environment env, Location location, DebugServerTransport transport) {
-    if (shouldPauseCurrentThread(env, location)) {
-      pauseCurrentThread(env, location, transport);
+    PauseReason pauseReason = shouldPauseCurrentThread(env, location);
+    if (pauseReason != null) {
+      pauseCurrentThread(env, pauseReason, location, transport);
     }
   }
 
@@ -210,22 +217,17 @@ final class ThreadHandler {
   /** Handles a {@code ListFramesRequest} and returns its response. */
   ImmutableList<SkylarkDebuggingProtos.Frame> listFrames(long threadId)
       throws DebugRequestException {
-    Debuggable debuggable;
-    PausedThreadState pausedState;
     synchronized (threads) {
       ThreadState thread = threads.get(threadId);
       if (thread == null) {
         throw new DebugRequestException(String.format("Thread %s is not running.", threadId));
       }
-      pausedState = thread.pausedState;
+      PausedThreadState pausedState = thread.pausedState;
       if (pausedState == null) {
         throw new DebugRequestException(String.format("Thread %s is not paused.", threadId));
       }
-      debuggable = thread.debuggable;
+      return listFrames(thread.debuggable, pausedState);
     }
-    // no need to list frames within the synchronize block: threads can only be resumed in response
-    // to a client request, and requests are handled serially
-    return listFrames(debuggable, pausedState);
   }
 
   private static ImmutableList<SkylarkDebuggingProtos.Frame> listFrames(
@@ -265,12 +267,11 @@ final class ThreadHandler {
    * ContinueExecutionRequest.
    */
   private void pauseCurrentThread(
-      Environment env, Location location, DebugServerTransport transport) {
+      Environment env, PauseReason pauseReason, Location location, DebugServerTransport transport) {
     long threadId = Thread.currentThread().getId();
 
     SkylarkDebuggingProtos.Thread threadProto;
     PausedThreadState pausedState;
-    Debuggable debuggable;
     synchronized (threads) {
       ThreadState thread = threads.get(threadId);
       if (thread == null) {
@@ -282,28 +283,32 @@ final class ThreadHandler {
         transport.postEvent(DebugEventHelper.threadStartedEvent(threadId, fallbackThreadName));
         thread = doRegisterThread(threadId, fallbackThreadName, env);
       }
-      debuggable = thread.debuggable;
-      pausedState = new PausedThreadState(location);
+      pausedState = new PausedThreadState(pauseReason, location);
       thread.pausedState = pausedState;
       // get proto after setting the paused state, so that it's up to date
       threadProto = getThreadProto(thread);
     }
 
-    transport.postEvent(
-        DebugEventHelper.threadPausedEvent(threadProto, listFrames(debuggable, pausedState)));
+    transport.postEvent(DebugEventHelper.threadPausedEvent(threadProto));
     pausedState.semaphore.acquireUninterruptibly();
     transport.postEvent(
         DebugEventHelper.threadContinuedEvent(
-            threadProto.toBuilder().clearLocation().setIsPaused(false).build()));
+            threadProto.toBuilder().clearThreadPausedState().build()));
   }
 
-  private boolean shouldPauseCurrentThread(Environment env, Location location) {
+  @Nullable
+  private PauseReason shouldPauseCurrentThread(Environment env, Location location) {
     long threadId = Thread.currentThread().getId();
-    if (threadsToPause.remove(threadId) || pausingAllThreads) {
-      return true;
+    boolean pausingAllThreads = this.pausingAllThreads;
+    if (pausingAllThreads) {
+      threadsToPause.remove(threadId);
+      return PauseReason.ALL_THREADS_PAUSED;
+    }
+    if (threadsToPause.remove(threadId)) {
+      return PauseReason.PAUSE_THREAD_REQUEST;
     }
     if (hasBreakpointAtLocation(location)) {
-      return true;
+      return PauseReason.HIT_BREAKPOINT;
     }
 
     // TODO(bazel-team): if contention becomes a problem, consider changing 'threads' to a
@@ -311,10 +316,10 @@ final class ThreadHandler {
     synchronized (threads) {
       ThreadState thread = threads.get(threadId);
       if (thread != null && thread.readyToPause != null && thread.readyToPause.test(env)) {
-        return true;
+        return PauseReason.STEPPING;
       }
     }
-    return false;
+    return null;
   }
 
   private boolean hasBreakpointAtLocation(Location location) {
@@ -336,9 +341,11 @@ final class ThreadHandler {
 
     PausedThreadState pausedState = thread.pausedState;
     if (pausedState != null) {
-      builder
-          .setIsPaused(true)
-          .setLocation(DebugEventHelper.getLocationProto(pausedState.location));
+      builder.setThreadPausedState(
+          ThreadPausedState.newBuilder()
+              .setPauseReason(pausedState.pauseReason)
+              .setLocation(DebugEventHelper.getLocationProto(pausedState.location))
+              .addAllFrame(listFrames(thread.debuggable, pausedState)));
     }
     return builder.build();
   }