Go back to handling conditional breakpoints server side.

I should have left it as it was -- doing it client-side is just too
slow.

ENUM_VALUE_OK=This proto has never yet been used
PiperOrigin-RevId: 201957138

4 files changed, 133 insertions, 44 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 918a667db4..a1066c3e7f 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
@@ -178,9 +178,15 @@ message ThreadContinuedEvent {
 message Breakpoint {
   oneof condition {
     // A breakpoint that is triggered when a particular line is reached.
-    // Column index will be ignored for breakpoints.
+    // Column index will be ignored for breakpoints. The debugger only supports
+    // one breakpoint per line. If multiple breakpoints are supplied for a
+    // single line, only the last such breakpoint is accepted.
     Location location = 1;
   }
+  // An optional condition for the breakpoint. When present, the breakpoint will
+  // be triggered iff both the primary condition holds and this expression
+  // evaluates to True.
+  string expression = 2;
 }
 
 // A single frame in a thread's stack trace.
@@ -251,6 +257,10 @@ message PausedThread {
   // The location in Skylark code of the next statement or expression that will
   // be executed.
   Location location = 4;
+
+  // An error that occurred while evaluating a breakpoint condition. Present if
+  // and only if pause_reason is CONDITIONAL_BREAKPOINT_ERROR.
+  Error conditional_breakpoint_error = 5;
 }
 
 // The reason why a thread was paused
@@ -259,16 +269,19 @@ enum PauseReason {
   UNSET = 0;
 
   // The stepping condition in a ContinueExecutionRequest was hit.
-  STEPPING = 4;
+  STEPPING = 1;
 
   // All threads are paused (e.g. prior to the initial StartDebuggingRequest).
-  ALL_THREADS_PAUSED = 1;
+  ALL_THREADS_PAUSED = 2;
 
   // Thread paused in response to a PauseThreadRequest.
-  PAUSE_THREAD_REQUEST = 2;
+  PAUSE_THREAD_REQUEST = 3;
 
   // A breakpoint was hit.
-  HIT_BREAKPOINT = 3;
+  HIT_BREAKPOINT = 4;
+
+  // An error occurred evaluating a breakpoint condition
+  CONDITIONAL_BREAKPOINT_ERROR = 5;
 }
 
 // The debugger representation of a Skylark value.
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ConditionalBreakpointException.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ConditionalBreakpointException.java
new file mode 100644
index 0000000000..df96ad96ef
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ConditionalBreakpointException.java
@@ -0,0 +1,23 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.skylarkdebug.server;
+
+/** An exception that occurs while evaluating a conditional breakpoint expression. */
+final class ConditionalBreakpointException extends Exception {
+
+  ConditionalBreakpointException(String message) {
+    super(message);
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java
index 1e4853ba9c..93e833db03 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java
@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.skylarkdebug.server;
 
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Throwables;
@@ -22,7 +21,6 @@ import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos;
-import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.Breakpoint.ConditionCase;
 import com.google.devtools.build.lib.syntax.DebugServer;
 import com.google.devtools.build.lib.syntax.Environment;
 import com.google.devtools.build.lib.syntax.Eval;
@@ -194,13 +192,7 @@ public final class SkylarkDebugServer implements DebugServer {
   /** Handles a {@code SetBreakpointsRequest} and returns its response. */
   private SkylarkDebuggingProtos.DebugEvent setBreakpoints(
       long sequenceNumber, SkylarkDebuggingProtos.SetBreakpointsRequest request) {
-    threadHandler.setBreakpoints(
-        request
-            .getBreakpointList()
-            .stream()
-            .filter(b -> b.getConditionCase() == ConditionCase.LOCATION)
-            .map(SkylarkDebuggingProtos.Breakpoint::getLocation)
-            .collect(toImmutableSet()));
+    threadHandler.setBreakpoints(request.getBreakpointList());
     return DebugEventHelper.setBreakpointsResponse(sequenceNumber);
   }
 
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 a7a2d6e0f4..585d581ed2 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
@@ -15,17 +15,20 @@
 package com.google.devtools.build.lib.skylarkdebug.server;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
 
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos;
+import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.Breakpoint;
+import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.Error;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.PauseReason;
 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;
 import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.EvalUtils;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
@@ -87,7 +90,8 @@ final class ThreadHandler {
   private final Map<Long, SteppingThreadState> steppingThreads = new HashMap<>();
 
   /** All location-based breakpoints (the only type of breakpoint currently supported). */
-  private volatile ImmutableSet<SkylarkDebuggingProtos.Location> breakpoints = ImmutableSet.of();
+  private volatile ImmutableMap<SkylarkDebuggingProtos.Location, SkylarkDebuggingProtos.Breakpoint>
+      breakpoints = ImmutableMap.of();
 
   /**
    * True if the thread is currently performing a debugger-requested evaluation. If so, we don't
@@ -123,13 +127,19 @@ final class ThreadHandler {
     }
   }
 
-  void setBreakpoints(ImmutableSet<SkylarkDebuggingProtos.Location> breakpoints) {
-    // all breakpoints cover the entire line, so unset the column number.
-    this.breakpoints =
-        breakpoints
-            .stream()
-            .map(location -> location.toBuilder().clearColumnNumber().build())
-            .collect(toImmutableSet());
+  void setBreakpoints(Collection<Breakpoint> breakpoints) {
+    Map<SkylarkDebuggingProtos.Location, SkylarkDebuggingProtos.Breakpoint> map = new HashMap<>();
+    for (SkylarkDebuggingProtos.Breakpoint breakpoint : breakpoints) {
+      if (breakpoint.getConditionCase()
+          != SkylarkDebuggingProtos.Breakpoint.ConditionCase.LOCATION) {
+        continue;
+      }
+      // all breakpoints cover the entire line, so unset the column number
+      SkylarkDebuggingProtos.Location location =
+          breakpoint.getLocation().toBuilder().clearColumnNumber().build();
+      map.put(location, breakpoint);
+    }
+    this.breakpoints = ImmutableMap.copyOf(map);
   }
 
   /**
@@ -188,7 +198,14 @@ final class ThreadHandler {
     if (servicingEvalRequest.get()) {
       return;
     }
-    PauseReason pauseReason = shouldPauseCurrentThread(env, location);
+    PauseReason pauseReason;
+    Error error = null;
+    try {
+      pauseReason = shouldPauseCurrentThread(env, location);
+    } catch (ConditionalBreakpointException e) {
+      pauseReason = PauseReason.CONDITIONAL_BREAKPOINT_ERROR;
+      error = Error.newBuilder().setMessage(e.getMessage()).build();
+    }
     if (pauseReason == null) {
       return;
     }
@@ -197,7 +214,7 @@ final class ThreadHandler {
     synchronized (this) {
       steppingThreads.remove(threadId);
     }
-    pauseCurrentThread(env, pauseReason, location, transport);
+    pauseCurrentThread(env, location, transport, pauseReason, error);
   }
 
   /** Handles a {@code ListFramesRequest} and returns its response. */
@@ -233,11 +250,24 @@ final class ThreadHandler {
     // accessed in response to a client request, and requests are handled serially
     // TODO(bazel-team): support asynchronous replies, and use finer-grained locks
     try {
-      servicingEvalRequest.set(true);
-      Object result = debuggable.evaluate(expression);
+      Object result = doEvaluate(debuggable, expression);
       return DebuggerSerialization.getValueProto("Evaluation result", result);
     } catch (EvalException | InterruptedException e) {
       throw new DebugRequestException(e.getMessage());
+    }
+  }
+
+  /**
+   * Evaluate the given expression in the environment defined by the provided {@link Debuggable}.
+   *
+   * <p>The caller is responsible for ensuring that the associated skylark thread isn't currently
+   * running.
+   */
+  private Object doEvaluate(Debuggable debuggable, String expression)
+      throws EvalException, InterruptedException {
+    try {
+      servicingEvalRequest.set(true);
+      return debuggable.evaluate(expression);
     } finally {
       servicingEvalRequest.set(false);
     }
@@ -248,7 +278,11 @@ final class ThreadHandler {
    * ContinueExecutionRequest.
    */
   private void pauseCurrentThread(
-      Environment env, PauseReason pauseReason, Location location, DebugServerTransport transport) {
+      Environment env,
+      Location location,
+      DebugServerTransport transport,
+      PauseReason pauseReason,
+      @Nullable Error conditionalBreakpointError) {
     long threadId = Thread.currentThread().getId();
 
     PausedThreadState pausedState =
@@ -257,14 +291,15 @@ final class ThreadHandler {
       pausedThreads.put(threadId, pausedState);
     }
     SkylarkDebuggingProtos.PausedThread threadProto =
-        getPausedThreadProto(pausedState, pauseReason);
+        getPausedThreadProto(pausedState, pauseReason, conditionalBreakpointError);
     transport.postEvent(DebugEventHelper.threadPausedEvent(threadProto));
     pausedState.semaphore.acquireUninterruptibly();
     transport.postEvent(DebugEventHelper.threadContinuedEvent(threadId));
   }
 
   @Nullable
-  private PauseReason shouldPauseCurrentThread(Environment env, Location location) {
+  private PauseReason shouldPauseCurrentThread(Environment env, Location location)
+      throws ConditionalBreakpointException {
     long threadId = Thread.currentThread().getId();
     if (pausingAllThreads) {
       return PauseReason.ALL_THREADS_PAUSED;
@@ -272,7 +307,7 @@ final class ThreadHandler {
     if (threadsToPause.contains(threadId)) {
       return PauseReason.PAUSE_THREAD_REQUEST;
     }
-    if (hasBreakpointAtLocation(location)) {
+    if (hasBreakpointMatchedAtLocation(env, location)) {
       return PauseReason.HIT_BREAKPOINT;
     }
 
@@ -287,26 +322,52 @@ final class ThreadHandler {
     return null;
   }
 
-  private boolean hasBreakpointAtLocation(Location location) {
+  /**
+   * Returns true if there's a breakpoint at the current location, with a satisfied condition if
+   * relevant.
+   */
+  private boolean hasBreakpointMatchedAtLocation(Environment env, Location location)
+      throws ConditionalBreakpointException {
     // breakpoints is volatile, so taking a local copy
-    ImmutableSet<SkylarkDebuggingProtos.Location> breakpoints = this.breakpoints;
+    ImmutableMap<SkylarkDebuggingProtos.Location, SkylarkDebuggingProtos.Breakpoint> breakpoints =
+        this.breakpoints;
     if (breakpoints.isEmpty()) {
       return false;
     }
     SkylarkDebuggingProtos.Location locationProto = DebugEventHelper.getLocationProto(location);
-    // column data ignored for breakpoints
-    return locationProto != null
-        && breakpoints.contains(locationProto.toBuilder().clearColumnNumber().build());
+    if (locationProto == null) {
+      return false;
+    }
+    locationProto = locationProto.toBuilder().clearColumnNumber().build();
+    SkylarkDebuggingProtos.Breakpoint breakpoint = breakpoints.get(locationProto);
+    if (breakpoint == null) {
+      return false;
+    }
+    String condition = breakpoint.getExpression();
+    if (condition.isEmpty()) {
+      return true;
+    }
+    try {
+      return EvalUtils.toBoolean(doEvaluate(env, condition));
+    } catch (EvalException | InterruptedException e) {
+      throw new ConditionalBreakpointException(e.getMessage());
+    }
   }
 
   /** Returns a {@code Thread} proto builder with information about the given thread. */
   private static SkylarkDebuggingProtos.PausedThread getPausedThreadProto(
-      PausedThreadState thread, PauseReason pauseReason) {
-    return SkylarkDebuggingProtos.PausedThread.newBuilder()
-        .setId(thread.id)
-        .setName(thread.name)
-        .setPauseReason(pauseReason)
-        .setLocation(DebugEventHelper.getLocationProto(thread.location))
-        .build();
+      PausedThreadState thread,
+      PauseReason pauseReason,
+      @Nullable Error conditionalBreakpointError) {
+    SkylarkDebuggingProtos.PausedThread.Builder builder =
+        SkylarkDebuggingProtos.PausedThread.newBuilder()
+            .setId(thread.id)
+            .setName(thread.name)
+            .setPauseReason(pauseReason)
+            .setLocation(DebugEventHelper.getLocationProto(thread.location));
+    if (conditionalBreakpointError != null) {
+      builder.setConditionalBreakpointError(conditionalBreakpointError);
+    }
+    return builder.build();
   }
 }