From a9faeffebb11780fedb26cf64f4c8e74bd80ee39 Mon Sep 17 00:00:00 2001
From: brendandouglas <brendandouglas@google.com>
Date: Thu, 14 Jun 2018 06:30:58 -0700
Subject: Skylark debugging protocol: remove conditional breakpoints, don't
 include frames in thread paused events.

I started implementing conditional breakpoints server-side in
unknown commit, but:
- client-side implementations seem more common in debugging protocols
- IntelliJ in particular has great support for client-side conditional
breakpoints, but poor support for server-side
- it's unnecessary extra complexity for the server -- simple line
breakpoints + evaluation requests already give us conditional bps.

Also removing frames from thread paused events. It results in poor
performance for stepping and pausing (in which we get frame info for
potentially dozens of threads), and again isn't usual for debuggers.

Finally, ignore breakpoints when performing a client-requested
evaluation.

PiperOrigin-RevId: 200547972
---
 .../lib/skylarkdebug/proto/skylark_debugging.proto   | 11 +----------
 .../build/lib/skylarkdebug/server/ThreadHandler.java | 20 ++++++++++++++++----
 2 files changed, 17 insertions(+), 14 deletions(-)

(limited to 'src/main/java/com/google/devtools/build/lib')

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 3d74f951af..d4cb453e99 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
@@ -192,17 +192,13 @@ message ThreadContinuedEvent {
   Thread thread = 1;
 }
 
-// A location or condition where the debug server will pause execution.
+// A location where the debug server will pause execution.
 message Breakpoint {
   oneof condition {
     // A breakpoint that is triggered when a particular line is reached.
     // Column index will be ignored for breakpoints.
     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.
@@ -280,11 +276,6 @@ message ThreadPausedState {
   // 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
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 d1ccad25c6..4e9f69e1e8 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
@@ -95,6 +95,12 @@ final class ThreadHandler {
   /** All location-based breakpoints (the only type of breakpoint currently supported). */
   private volatile ImmutableSet<SkylarkDebuggingProtos.Location> breakpoints = ImmutableSet.of();
 
+  /**
+   * True if the thread is currently performing a debugger-requested evaluation. If so, we don't
+   * check for breakpoints during the evaluation.
+   */
+  private final ThreadLocal<Boolean> servicingEvalRequest = ThreadLocal.withInitial(() -> false);
+
   /**
    * Threads which are set to be paused in the next checked execution step.
    *
@@ -198,6 +204,9 @@ final class ThreadHandler {
   }
 
   void pauseIfNecessary(Environment env, Location location, DebugServerTransport transport) {
+    if (servicingEvalRequest.get()) {
+      return;
+    }
     PauseReason pauseReason = shouldPauseCurrentThread(env, location);
     if (pauseReason != null) {
       pauseCurrentThread(env, pauseReason, location, transport);
@@ -252,13 +261,17 @@ final class ThreadHandler {
       }
       debuggable = thread.debuggable;
     }
-    // no need to evaluate within the synchronize block: threads can only be resumed in response
-    // to a client request, and requests are handled serially
+    // no need to evaluate within the synchronize block: for paused threads, debuggable is only
+    // 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);
       return DebuggerSerialization.getValueProto("Evaluation result", result);
     } catch (EvalException | InterruptedException e) {
       throw new DebugRequestException(e.getMessage());
+    } finally {
+      servicingEvalRequest.set(false);
     }
   }
 
@@ -344,8 +357,7 @@ final class ThreadHandler {
       builder.setThreadPausedState(
           ThreadPausedState.newBuilder()
               .setPauseReason(pausedState.pauseReason)
-              .setLocation(DebugEventHelper.getLocationProto(pausedState.location))
-              .addAllFrame(listFrames(thread.debuggable, pausedState)));
+              .setLocation(DebugEventHelper.getLocationProto(pausedState.location)));
     }
     return builder.build();
   }
-- 
cgit v1.2.3