diff options
author | brendandouglas <brendandouglas@google.com> | 2018-06-25 08:26:01 -0700 |
---|---|---|
committer | Copybara-Service <copybara-piper@google.com> | 2018-06-25 08:27:59 -0700 |
commit | cac65060368ad57e038b59ec185ef816bef9c7e6 (patch) | |
tree | e714ba04843380afaa25001dd8f4db22aa01a7d7 | |
parent | e5a90da8656863da48865536ab9c75f7ecf8e23e (diff) |
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
5 files changed, 246 insertions, 48 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(); } } diff --git a/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java b/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java index 66303ab30c..064f666d25 100644 --- a/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java @@ -49,10 +49,12 @@ import java.io.IOException; import java.net.InetAddress; import java.net.ServerSocket; import java.time.Duration; +import java.util.Collection; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -170,6 +172,104 @@ public class SkylarkDebugServerTest { } @Test + public void testDoNotPauseAtUnsatisfiedConditionalBreakpoint() throws Exception { + sendStartDebuggingRequest(); + BuildFileAST buildFile = + parseBuildFile("/a/build/file/BUILD", "x = [1,2,3]", "y = [2,3,4]", "z = 1"); + Environment env = newEnvironment(); + + ImmutableList<Breakpoint> breakpoints = + ImmutableList.of( + Breakpoint.newBuilder() + .setLocation(Location.newBuilder().setLineNumber(2).setPath("/a/build/file/BUILD")) + .setExpression("x[0] == 2") + .build(), + Breakpoint.newBuilder() + .setLocation(Location.newBuilder().setLineNumber(3).setPath("/a/build/file/BUILD")) + .setExpression("x[0] == 1") + .build()); + setBreakpoints(breakpoints); + + Thread evaluationThread = execInWorkerThread(buildFile, env); + String threadName = evaluationThread.getName(); + long threadId = evaluationThread.getId(); + Breakpoint expectedBreakpoint = breakpoints.get(1); + + DebugEvent event = client.waitForEvent(DebugEvent::hasThreadPaused, Duration.ofSeconds(5)); + assertThat(event) + .isEqualTo( + DebugEventHelper.threadPausedEvent( + SkylarkDebuggingProtos.PausedThread.newBuilder() + .setName(threadName) + .setId(threadId) + .setLocation(expectedBreakpoint.getLocation().toBuilder().setColumnNumber(1)) + .setPauseReason(PauseReason.HIT_BREAKPOINT) + .build())); + } + + @Test + public void testPauseAtSatisfiedConditionalBreakpoint() throws Exception { + sendStartDebuggingRequest(); + BuildFileAST buildFile = parseBuildFile("/a/build/file/BUILD", "x = [1,2,3]", "y = [2,3,4]"); + Environment env = newEnvironment(); + + Location location = + Location.newBuilder().setLineNumber(2).setPath("/a/build/file/BUILD").build(); + Breakpoint breakpoint = + Breakpoint.newBuilder().setLocation(location).setExpression("x[0] == 1").build(); + setBreakpoints(ImmutableList.of(breakpoint)); + + Thread evaluationThread = execInWorkerThread(buildFile, env); + String threadName = evaluationThread.getName(); + long threadId = evaluationThread.getId(); + + // wait for breakpoint to be hit + DebugEvent event = client.waitForEvent(DebugEvent::hasThreadPaused, Duration.ofSeconds(5)); + + SkylarkDebuggingProtos.PausedThread expectedThreadState = + SkylarkDebuggingProtos.PausedThread.newBuilder() + .setName(threadName) + .setId(threadId) + .setPauseReason(PauseReason.HIT_BREAKPOINT) + .setLocation(location.toBuilder().setColumnNumber(1)) + .build(); + + assertThat(event).isEqualTo(DebugEventHelper.threadPausedEvent(expectedThreadState)); + } + + @Test + public void testPauseAtInvalidConditionBreakpointWithError() throws Exception { + sendStartDebuggingRequest(); + BuildFileAST buildFile = parseBuildFile("/a/build/file/BUILD", "x = [1,2,3]", "y = [2,3,4]"); + Environment env = newEnvironment(); + + Location location = + Location.newBuilder().setLineNumber(2).setPath("/a/build/file/BUILD").build(); + Breakpoint breakpoint = + Breakpoint.newBuilder().setLocation(location).setExpression("z[0] == 1").build(); + setBreakpoints(ImmutableList.of(breakpoint)); + + Thread evaluationThread = execInWorkerThread(buildFile, env); + String threadName = evaluationThread.getName(); + long threadId = evaluationThread.getId(); + + // wait for breakpoint to be hit + DebugEvent event = client.waitForEvent(DebugEvent::hasThreadPaused, Duration.ofSeconds(5)); + + SkylarkDebuggingProtos.PausedThread expectedThreadState = + SkylarkDebuggingProtos.PausedThread.newBuilder() + .setName(threadName) + .setId(threadId) + .setPauseReason(PauseReason.CONDITIONAL_BREAKPOINT_ERROR) + .setLocation(location.toBuilder().setColumnNumber(1)) + .setConditionalBreakpointError( + SkylarkDebuggingProtos.Error.newBuilder().setMessage("name \'z\' is not defined")) + .build(); + + assertThat(event).isEqualTo(DebugEventHelper.threadPausedEvent(expectedThreadState)); + } + + @Test public void testListFramesForInvalidThread() throws Exception { sendStartDebuggingRequest(); DebugEvent event = @@ -473,12 +573,21 @@ public class SkylarkDebugServerTest { assertThat(pausedThread.getLocation()).isEqualTo(expectedLocation); } - private void setBreakpoints(Iterable<Location> locations) throws Exception { - SetBreakpointsRequest.Builder request = SetBreakpointsRequest.newBuilder(); - locations.forEach(l -> request.addBreakpoint(Breakpoint.newBuilder().setLocation(l))); + private void setBreakpoints(Collection<Location> locations) throws Exception { + setBreakpoints( + locations + .stream() + .map(l -> Breakpoint.newBuilder().setLocation(l).build()) + .collect(Collectors.toList())); + } + + private void setBreakpoints(Iterable<Breakpoint> breakpoints) throws Exception { DebugEvent response = client.sendRequestAndWaitForResponse( - DebugRequest.newBuilder().setSequenceNumber(10).setSetBreakpoints(request).build()); + DebugRequest.newBuilder() + .setSequenceNumber(10) + .setSetBreakpoints(SetBreakpointsRequest.newBuilder().addAllBreakpoint(breakpoints)) + .build()); assertThat(response.hasSetBreakpoints()).isTrue(); assertThat(response.getSequenceNumber()).isEqualTo(10); } |