From b72f886573cd702f03ea71895e32fd729287d9bf Mon Sep 17 00:00:00 2001 From: Nipuna Ranasinghe Date: Thu, 31 Oct 2024 09:48:16 +0530 Subject: [PATCH 1/9] Optimize dynamic breakpoint handling --- .../debugadapter/BreakpointProcessor.java | 46 ++++++++++--------- .../debugadapter/ExecutionContext.java | 23 +++++++--- .../debugadapter/JBallerinaDebugServer.java | 29 +++++++----- .../debugadapter/JDIEventProcessor.java | 6 +-- 4 files changed, 61 insertions(+), 43 deletions(-) diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java index 9d0b8b935ad2..f24b44701f5b 100644 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java @@ -48,6 +48,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; @@ -108,8 +109,9 @@ void processBreakpointEvent(BreakpointEvent bpEvent) { // on the last line. if (requireStepOut(bpEvent)) { activateDynamicBreakPoints((int) bpEvent.thread().uniqueID(), DynamicBreakpointMode.CALLER); + context.setPrevInstruction(DebugInstruction.STEP_OVER); context.getDebuggeeVM().resume(); - } else if (context.getLastInstruction() != null && context.getLastInstruction() != DebugInstruction.CONTINUE) { + } else if (context.getPrevInstruction() != null && context.getPrevInstruction() != DebugInstruction.CONTINUE) { jdiEventProcessor.notifyStopEvent(bpEvent); } else if (fileBreakpoints == null || !fileBreakpoints.containsKey(lineNumber)) { jdiEventProcessor.notifyStopEvent(bpEvent); @@ -168,19 +170,13 @@ private void processAdvanceBreakpoints(BreakpointEvent event, BalBreakpoint brea /** * Responsible for clearing dynamic(temporary) breakpoints used for step over instruction and for restoring the * original user breakpoints before proceeding with the other debug instructions. - * - * @param instruction debug instruction */ - void restoreUserBreakpoints(DebugInstruction instruction) { + void restoreUserBreakpoints() { if (context.getDebuggeeVM() == null) { return; } - context.getEventManager().deleteAllBreakpoints(); - if (instruction == DebugInstruction.CONTINUE || instruction == DebugInstruction.STEP_OVER) { - context.getDebuggeeVM().allClasses().forEach(referenceType -> - activateUserBreakPoints(referenceType, false)); - } + context.getDebuggeeVM().allClasses().forEach(classRef -> activateUserBreakPoints(classRef, false)); } /** @@ -238,15 +234,23 @@ void activateDynamicBreakPoints(int threadId, DynamicBreakpointMode mode) { try { List jStackFrames = threadReference.frames(); List validFrames = jdiEventProcessor.filterValidBallerinaFrames(jStackFrames); - - if (mode == DynamicBreakpointMode.CURRENT && !validFrames.isEmpty()) { - configureBreakpointsForMethod(validFrames.get(0)); + if (!validFrames.isEmpty()) { + Location currentLocation = validFrames.get(0).getJStackFrame().location(); + if (mode == DynamicBreakpointMode.CURRENT && (Objects.isNull(context.getPrevLocation()) || + !context.getPrevLocation().method().equals(currentLocation.method()))) { + context.getEventManager().deleteAllBreakpoints(); + configureBreakpointsForMethod(currentLocation); + } + context.setPrevLocation(currentLocation); } + // If the current function is invoked within another ballerina function, we need to explicitly set another // temporary breakpoint on the location of its invocation. This is supposed to handle the situations where // the user wants to step over on an exit point of the current function. if (mode == DynamicBreakpointMode.CALLER && validFrames.size() > 1) { - configureBreakpointsForMethod(validFrames.get(1)); + context.getEventManager().deleteAllBreakpoints(); + configureBreakpointsForMethod(validFrames.get(1).getJStackFrame().location()); + context.setPrevLocation(validFrames.get(1).getJStackFrame().location()); } } catch (JdiProxyException e) { LOGGER.error(e.getMessage()); @@ -261,11 +265,10 @@ void activateDynamicBreakPoints(int threadId, DynamicBreakpointMode mode) { * Configures temporary(dynamic) breakpoints for all the lines within the method, which encloses the given stack * frame location. This strategy is used when processing STEP_OVER requests. * - * @param balStackFrame stack frame which contains the method information + * @param currentLocation current stack frame location */ - private void configureBreakpointsForMethod(BallerinaStackFrame balStackFrame) { + private void configureBreakpointsForMethod(Location currentLocation) { try { - Location currentLocation = balStackFrame.getJStackFrame().location(); ReferenceType referenceType = currentLocation.declaringType(); List allLocations = currentLocation.method().allLineLocations(); Optional firstLocation = allLocations.stream() @@ -291,7 +294,7 @@ private void configureBreakpointsForMethod(BallerinaStackFrame balStackFrame) { } nextStepPoint++; } while (nextStepPoint <= lastLocation.get().lineNumber()); - } catch (AbsentInformationException | JdiProxyException e) { + } catch (AbsentInformationException e) { LOGGER.error(e.getMessage()); } } @@ -379,14 +382,15 @@ private BExpressionValue evaluateExpressionSafely(String expression, ThreadRefer // As we are disabling all the breakpoint requests before evaluating the user's conditional // expression, need to re-enable all the breakpoints before continuing the remote VM execution. - restoreUserBreakpoints(context.getLastInstruction()); + context.getEventManager().classPrepareRequests().forEach(EventRequest::enable); + context.getEventManager().breakpointRequests().forEach(BreakpointRequest::enable); return evaluationResult; } private boolean requireStepOut(BreakpointEvent event) { try { - if (context.getLastInstruction() != DebugInstruction.STEP_OVER - && context.getLastInstruction() != DebugInstruction.STEP_OUT) { + if (context.getPrevInstruction() != DebugInstruction.STEP_OVER + && context.getPrevInstruction() != DebugInstruction.STEP_OUT) { return false; } Location currentLocation = event.location(); @@ -415,7 +419,7 @@ private void notifyBreakPointChangesToClient(BalBreakpoint balBreakpoint) { /** * Dynamic Breakpoint Options. */ - enum DynamicBreakpointMode { + public enum DynamicBreakpointMode { /** * Configures dynamic breakpoints only for the current method (active stack frame). */ diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/ExecutionContext.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/ExecutionContext.java index 9fcdba5595ae..0c7ff2dbfe49 100644 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/ExecutionContext.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/ExecutionContext.java @@ -16,6 +16,7 @@ package org.ballerinalang.debugadapter; +import com.sun.jdi.Location; import com.sun.jdi.request.EventRequestManager; import io.ballerina.projects.Project; import org.ballerinalang.debugadapter.jdi.VirtualMachineProxyImpl; @@ -39,14 +40,16 @@ public class ExecutionContext { private String sourceProjectRoot; private final DebugProjectCache projectCache; private Process launchedProcess; - private DebugInstruction lastInstruction; private boolean terminateRequestReceived; private boolean supportsRunInTerminalRequest; + private DebugInstruction prevInstruction; + private Location prevLocation; ExecutionContext(JBallerinaDebugServer adapter) { this.adapter = adapter; this.projectCache = new DebugProjectCache(); - this.lastInstruction = DebugInstruction.CONTINUE; + this.prevInstruction = DebugInstruction.CONTINUE; + this.prevLocation = null; } public Optional getLaunchedProcess() { @@ -102,12 +105,20 @@ public BufferedReader getErrorStream() { return new BufferedReader(new InputStreamReader(launchedProcess.getErrorStream(), StandardCharsets.UTF_8)); } - public DebugInstruction getLastInstruction() { - return lastInstruction; + public DebugInstruction getPrevInstruction() { + return prevInstruction; } - public void setLastInstruction(DebugInstruction lastInstruction) { - this.lastInstruction = lastInstruction; + public void setPrevInstruction(DebugInstruction prevInstruction) { + this.prevInstruction = prevInstruction; + } + + public Location getPrevLocation() { + return prevLocation; + } + + public void setPrevLocation(Location prevLocation) { + this.prevLocation = prevLocation; } public DebugMode getDebugMode() { diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JBallerinaDebugServer.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JBallerinaDebugServer.java index b6d854e42bf6..9c36e6163d1f 100755 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JBallerinaDebugServer.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JBallerinaDebugServer.java @@ -30,6 +30,7 @@ import io.ballerina.identifier.Utils; import io.ballerina.projects.Project; import io.ballerina.projects.directory.SingleFileProject; +import org.ballerinalang.debugadapter.BreakpointProcessor.DynamicBreakpointMode; import org.ballerinalang.debugadapter.breakpoint.BalBreakpoint; import org.ballerinalang.debugadapter.completion.CompletionGenerator; import org.ballerinalang.debugadapter.completion.context.CompletionContext; @@ -425,7 +426,7 @@ public CompletableFuture source(SourceArguments args) { @Override public CompletableFuture continue_(ContinueArguments args) { - prepareFor(DebugInstruction.CONTINUE); + prepareFor(DebugInstruction.CONTINUE, args.getThreadId()); context.getDebuggeeVM().resume(); ContinueResponse continueResponse = new ContinueResponse(); continueResponse.setAllThreadsContinued(true); @@ -434,29 +435,25 @@ public CompletableFuture continue_(ContinueArguments args) { @Override public CompletableFuture next(NextArguments args) { - prepareFor(DebugInstruction.STEP_OVER); + prepareFor(DebugInstruction.STEP_OVER, args.getThreadId()); eventProcessor.sendStepRequest(args.getThreadId(), StepRequest.STEP_OVER); return CompletableFuture.completedFuture(null); } @Override public CompletableFuture stepIn(StepInArguments args) { - prepareFor(DebugInstruction.STEP_IN); + prepareFor(DebugInstruction.STEP_IN, args.getThreadId()); eventProcessor.sendStepRequest(args.getThreadId(), StepRequest.STEP_INTO); return CompletableFuture.completedFuture(null); } @Override public CompletableFuture stepOut(StepOutArguments args) { - stepOut(args.getThreadId()); + prepareFor(DebugInstruction.STEP_OUT, args.getThreadId()); + eventProcessor.sendStepRequest(args.getThreadId(), StepRequest.STEP_OUT); return CompletableFuture.completedFuture(null); } - void stepOut(int threadId) { - prepareFor(DebugInstruction.STEP_OUT); - eventProcessor.sendStepRequest(threadId, StepRequest.STEP_OUT); - } - @Override public CompletableFuture setExceptionBreakpoints(SetExceptionBreakpointsArguments args) { return CompletableFuture.completedFuture(null); @@ -1119,10 +1116,18 @@ private void attachToRemoteVM(String hostName, int portName) throws IOException, /** * Clears previous state information and prepares for the given debug instruction type execution. */ - private void prepareFor(DebugInstruction instruction) { + private void prepareFor(DebugInstruction instruction, int threadId) { clearState(); - eventProcessor.getBreakpointProcessor().restoreUserBreakpoints(instruction); - context.setLastInstruction(instruction); + DebugInstruction prevInstruction = context.getPrevInstruction(); + if (prevInstruction == DebugInstruction.STEP_OVER && instruction == DebugInstruction.CONTINUE) { + // need to remove dynamic breakpoints and restore user breakpoints, only when stepping over is followed + // by a 'continue' instruction. + eventProcessor.getBreakpointProcessor().restoreUserBreakpoints(); + } else if (instruction == DebugInstruction.STEP_OVER) { + // Activates dynamic breakpoints to replicate the step-over behavior. + eventProcessor.getBreakpointProcessor().activateDynamicBreakPoints(threadId, DynamicBreakpointMode.CURRENT); + } + context.setPrevInstruction(instruction); } /** diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JDIEventProcessor.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JDIEventProcessor.java index 37b8a123888a..25a8ae228435 100755 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JDIEventProcessor.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JDIEventProcessor.java @@ -101,7 +101,7 @@ void startListening() { private void processEvent(EventSet eventSet, Event event) { if (event instanceof ClassPrepareEvent evt) { - if (context.getLastInstruction() != DebugInstruction.STEP_OVER) { + if (context.getPrevInstruction() != DebugInstruction.STEP_OVER) { breakpointProcessor.activateUserBreakPoints(evt.referenceType(), true); } eventSet.resume(); @@ -136,9 +136,7 @@ void enableBreakpoints(String qualifiedClassName, LinkedHashMap Date: Mon, 4 Nov 2024 15:44:13 +0530 Subject: [PATCH 2/9] Improve JDI breakpoint events handling --- .../debugadapter/BreakpointProcessor.java | 13 ++--- .../engine/invokable/JvmMethod.java | 22 ++------ .../debugadapter/jdi/JDIUtils.java | 51 +++++++++++++++++++ 3 files changed, 61 insertions(+), 25 deletions(-) create mode 100644 misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/jdi/JDIUtils.java diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java index f24b44701f5b..cbcab73d7af3 100644 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java @@ -22,17 +22,15 @@ import com.sun.jdi.ThreadReference; import com.sun.jdi.event.BreakpointEvent; import com.sun.jdi.request.BreakpointRequest; -import com.sun.jdi.request.EventRequest; import com.sun.jdi.request.StepRequest; import org.ballerinalang.debugadapter.breakpoint.BalBreakpoint; import org.ballerinalang.debugadapter.breakpoint.LogMessage; import org.ballerinalang.debugadapter.breakpoint.TemplateLogMessage; -import org.ballerinalang.debugadapter.config.ClientConfigHolder; -import org.ballerinalang.debugadapter.config.ClientLaunchConfigHolder; import org.ballerinalang.debugadapter.evaluation.BExpressionValue; import org.ballerinalang.debugadapter.evaluation.DebugExpressionEvaluator; import org.ballerinalang.debugadapter.evaluation.EvaluationException; import org.ballerinalang.debugadapter.evaluation.EvaluationExceptionKind; +import org.ballerinalang.debugadapter.jdi.JDIUtils; import org.ballerinalang.debugadapter.jdi.JdiProxyException; import org.ballerinalang.debugadapter.jdi.StackFrameProxyImpl; import org.ballerinalang.debugadapter.jdi.ThreadReferenceProxyImpl; @@ -202,6 +200,7 @@ void activateUserBreakPoints(ReferenceType referenceType, boolean shouldNotify) for (BalBreakpoint breakpoint : breakpoints.values()) { List locations = referenceType.locationsOfLine(breakpoint.getLine()); if (!locations.isEmpty()) { + // TODO: should we consider the last location instead? Location loc = locations.get(0); BreakpointRequest bpReq = context.getEventManager().createBreakpointRequest(loc); bpReq.enable(); @@ -365,8 +364,7 @@ private BExpressionValue evaluateExpressionSafely(String expression, ThreadRefer // the new event. If this new EventSet is in 'SUSPEND_ALL' mode, then a deadlock will occur because no one // will resume the EventSet. Therefore to avoid this, we are disabling possible event requests before doing // the condition evaluation. - context.getEventManager().classPrepareRequests().forEach(EventRequest::disable); - context.getEventManager().breakpointRequests().forEach(BreakpointRequest::disable); + JDIUtils.disableJDIRequests(context); ThreadReferenceProxyImpl thread = context.getAdapter().getAllThreads().get((int) threadReference.uniqueID()); List validFrames = jdiEventProcessor.filterValidBallerinaFrames(thread.frames()); @@ -380,10 +378,9 @@ private BExpressionValue evaluateExpressionSafely(String expression, ThreadRefer evaluator.setExpression(expression); BExpressionValue evaluationResult = evaluator.evaluate(); - // As we are disabling all the breakpoint requests before evaluating the user's conditional + // As we disabled all the breakpoint requests before evaluating the user's conditional // expression, need to re-enable all the breakpoints before continuing the remote VM execution. - context.getEventManager().classPrepareRequests().forEach(EventRequest::enable); - context.getEventManager().breakpointRequests().forEach(BreakpointRequest::enable); + JDIUtils.disableJDIRequests(context); return evaluationResult; } diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/evaluation/engine/invokable/JvmMethod.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/evaluation/engine/invokable/JvmMethod.java index 3324101e7af2..43601b5b4fee 100644 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/evaluation/engine/invokable/JvmMethod.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/evaluation/engine/invokable/JvmMethod.java @@ -18,11 +18,10 @@ import com.sun.jdi.Method; import com.sun.jdi.Value; -import com.sun.jdi.request.EventRequest; -import com.sun.jdi.request.EventRequestManager; import org.ballerinalang.debugadapter.SuspendedContext; import org.ballerinalang.debugadapter.evaluation.EvaluationException; import org.ballerinalang.debugadapter.evaluation.utils.EvaluationUtils; +import org.ballerinalang.debugadapter.jdi.JDIUtils; import java.util.List; import java.util.Optional; @@ -62,21 +61,10 @@ public abstract class JvmMethod { * @return invocation result */ public Value invokeSafely() throws EvaluationException { - disablePendingJDIRequests(); - return this.invoke(); - } - - /** - * When invoking methods in the remote JVM, it can cause deadlocks if 'invokeMethod' is called from the - * client's event handler thread. In that case, the thread will be waiting for the invokeMethod to complete - * and won't read the EventSet that comes in for the new event. If this new EventSet is in 'SUSPEND_ALL' - * mode, then a deadlock will occur because no one will resume the EventSet. Therefore to avoid this, we are - * disabling possible event requests before doing any method invocations. - */ - private void disablePendingJDIRequests() { - EventRequestManager eventManager = context.getExecutionContext().getEventManager(); - eventManager.classPrepareRequests().forEach(EventRequest::disable); - eventManager.breakpointRequests().forEach(EventRequest::disable); + JDIUtils.disableJDIRequests(context.getExecutionContext()); + Value value = this.invoke(); + JDIUtils.enableJDIRequests(context.getExecutionContext()); + return value; } /** diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/jdi/JDIUtils.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/jdi/JDIUtils.java new file mode 100644 index 000000000000..0db0b357b16d --- /dev/null +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/jdi/JDIUtils.java @@ -0,0 +1,51 @@ +/* + * Copyright (c) 2024, WSO2 LLC. (http://wso2.com). + * + * 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 org.ballerinalang.debugadapter.jdi; + +import com.sun.jdi.request.EventRequest; +import com.sun.jdi.request.EventRequestManager; +import org.ballerinalang.debugadapter.ExecutionContext; + +/** + * JDI utility functions. + */ +public class JDIUtils { + + /** + * When invoking methods in the remote JVM, it can cause deadlocks if 'invokeMethod' is called from the + * client's event handler thread. In that case, the thread will be waiting for the invokeMethod to complete + * and won't read the EventSet that comes in for the new event. If this new EventSet is in 'SUSPEND_ALL' + * mode, then a deadlock will occur because no one will resume the EventSet. + *

+ * Therefore, to avoid this, we are disabling possible event requests (only class prepare and breakpoint requests + * for now) before invoking the method. + */ + public static void disableJDIRequests(ExecutionContext context) { + EventRequestManager eventManager = context.getEventManager(); + eventManager.classPrepareRequests().forEach(EventRequest::disable); + eventManager.breakpointRequests().forEach(EventRequest::disable); + } + + /** + * Used to re-enable the event requests after the method invocation is completed. + */ + public static void enableJDIRequests(ExecutionContext context) { + EventRequestManager eventManager = context.getEventManager(); + eventManager.classPrepareRequests().forEach(EventRequest::enable); + eventManager.breakpointRequests().forEach(EventRequest::enable); + } +} From d3ac599e4c568e0ebeba04cd45bb256d3708f1b6 Mon Sep 17 00:00:00 2001 From: Nipuna Ranasinghe Date: Mon, 4 Nov 2024 15:46:13 +0530 Subject: [PATCH 3/9] Optimize breakpoint processing logic in no-debug mode --- .../debugadapter/BreakpointProcessor.java | 7 ------- .../debugadapter/JBallerinaDebugServer.java | 11 ++++++++++- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java index cbcab73d7af3..a5201c22b7fe 100644 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java @@ -185,13 +185,6 @@ void restoreUserBreakpoints() { */ void activateUserBreakPoints(ReferenceType referenceType, boolean shouldNotify) { try { - // avoids setting break points if the server is running in 'no-debug' mode. - ClientConfigHolder configHolder = context.getAdapter().getClientConfigHolder(); - if (configHolder instanceof ClientLaunchConfigHolder - && ((ClientLaunchConfigHolder) configHolder).isNoDebugMode()) { - return; - } - String qualifiedClassName = getQualifiedClassName(referenceType); if (!userBreakpoints.containsKey(qualifiedClassName)) { return; diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JBallerinaDebugServer.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JBallerinaDebugServer.java index 9c36e6163d1f..93ee2aea3cc9 100755 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JBallerinaDebugServer.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JBallerinaDebugServer.java @@ -216,6 +216,11 @@ public CompletableFuture initialize(InitializeRequestArguments arg @Override public CompletableFuture setBreakpoints(SetBreakpointsArguments args) { return CompletableFuture.supplyAsync(() -> { + SetBreakpointsResponse bpResponse = new SetBreakpointsResponse(); + if (isNoDebugMode()) { + return bpResponse; + } + BalBreakpoint[] balBreakpoints = Arrays.stream(args.getBreakpoints()) .map((SourceBreakpoint sourceBreakpoint) -> toBreakpoint(sourceBreakpoint, args.getSource())) .toArray(BalBreakpoint[]::new); @@ -225,7 +230,6 @@ public CompletableFuture setBreakpoints(SetBreakpointsAr breakpointsMap.put(bp.getLine(), bp); } - SetBreakpointsResponse bpResponse = new SetBreakpointsResponse(); String sourcePathUri = args.getSource().getPath(); Optional qualifiedClassName = getQualifiedClassName(context, sourcePathUri); if (qualifiedClassName.isEmpty()) { @@ -1130,6 +1134,11 @@ private void prepareFor(DebugInstruction instruction, int threadId) { context.setPrevInstruction(instruction); } + private boolean isNoDebugMode() { + ClientConfigHolder confHolder = context.getAdapter().getClientConfigHolder(); + return confHolder instanceof ClientLaunchConfigHolder launchConfigHolder && launchConfigHolder.isNoDebugMode(); + } + /** * Clears state information. */ From 676a500ba706266de7f763c740651ee933fbe372 Mon Sep 17 00:00:00 2001 From: Nipuna Ranasinghe Date: Mon, 4 Nov 2024 17:32:14 +0530 Subject: [PATCH 4/9] Fix step-over behaviour for function-typed parameters --- .../debugadapter/BreakpointProcessor.java | 25 ++++++++++++++++--- .../debugadapter/JDIEventProcessor.java | 3 +-- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java index a5201c22b7fe..9564544bb10f 100644 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java @@ -228,8 +228,8 @@ void activateDynamicBreakPoints(int threadId, DynamicBreakpointMode mode) { List validFrames = jdiEventProcessor.filterValidBallerinaFrames(jStackFrames); if (!validFrames.isEmpty()) { Location currentLocation = validFrames.get(0).getJStackFrame().location(); - if (mode == DynamicBreakpointMode.CURRENT && (Objects.isNull(context.getPrevLocation()) || - !context.getPrevLocation().method().equals(currentLocation.method()))) { + if (mode == DynamicBreakpointMode.CURRENT + && !isWithinSameSource(currentLocation, context.getPrevLocation())) { context.getEventManager().deleteAllBreakpoints(); configureBreakpointsForMethod(currentLocation); } @@ -253,6 +253,24 @@ void activateDynamicBreakPoints(int threadId, DynamicBreakpointMode mode) { } } + /** + * Checks whether the given two locations are within the same source file. + * + * @param currentLocation current location + * @param prevLocation previous location + * @return true if the given two locations are within the same source file, false otherwise + */ + private boolean isWithinSameSource(Location currentLocation, Location prevLocation) { + if (prevLocation == null) { + return false; + } + try { + return Objects.equals(currentLocation.sourcePath(), prevLocation.sourcePath()); + } catch (AbsentInformationException e) { + return false; + } + } + /** * Configures temporary(dynamic) breakpoints for all the lines within the method, which encloses the given stack * frame location. This strategy is used when processing STEP_OVER requests. @@ -261,7 +279,6 @@ void activateDynamicBreakPoints(int threadId, DynamicBreakpointMode mode) { */ private void configureBreakpointsForMethod(Location currentLocation) { try { - ReferenceType referenceType = currentLocation.declaringType(); List allLocations = currentLocation.method().allLineLocations(); Optional firstLocation = allLocations.stream() .filter(location -> location.lineNumber() > 0) @@ -273,7 +290,7 @@ private void configureBreakpointsForMethod(Location currentLocation) { int nextStepPoint = firstLocation.get().lineNumber(); do { - List locations = referenceType.locationsOfLine(nextStepPoint); + List locations = currentLocation.method().locationsOfLine(nextStepPoint); if (!locations.isEmpty() && (locations.get(0).lineNumber() > firstLocation.get().lineNumber())) { // Checks whether there are any user breakpoint configured for the same location, before adding the // dynamic breakpoint. diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JDIEventProcessor.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JDIEventProcessor.java index 25a8ae228435..b538daf2e89b 100755 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JDIEventProcessor.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JDIEventProcessor.java @@ -44,7 +44,6 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CopyOnWriteArrayList; -import static org.ballerinalang.debugadapter.BreakpointProcessor.DynamicBreakpointMode; import static org.ballerinalang.debugadapter.JBallerinaDebugServer.isBalStackFrame; import static org.ballerinalang.debugadapter.utils.PackageUtils.BAL_FILE_EXT; @@ -136,7 +135,7 @@ void enableBreakpoints(String qualifiedClassName, LinkedHashMap Date: Mon, 4 Nov 2024 18:06:28 +0530 Subject: [PATCH 5/9] Add minor fix --- .../org/ballerinalang/debugadapter/BreakpointProcessor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java index 9564544bb10f..dba7af208e45 100644 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java @@ -390,7 +390,7 @@ private BExpressionValue evaluateExpressionSafely(String expression, ThreadRefer // As we disabled all the breakpoint requests before evaluating the user's conditional // expression, need to re-enable all the breakpoints before continuing the remote VM execution. - JDIUtils.disableJDIRequests(context); + JDIUtils.enableJDIRequests(context); return evaluationResult; } From a8bc7cb476ea008469aa9a8018982d9c52f110df Mon Sep 17 00:00:00 2001 From: Nipuna Ranasinghe Date: Mon, 4 Nov 2024 23:11:32 +0530 Subject: [PATCH 6/9] Address review suggestions --- .../ballerinalang/debugadapter/BreakpointProcessor.java | 8 +++----- .../org/ballerinalang/debugadapter/ExecutionContext.java | 4 ++-- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java index dba7af208e45..5896da008f3c 100644 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java @@ -228,8 +228,9 @@ void activateDynamicBreakPoints(int threadId, DynamicBreakpointMode mode) { List validFrames = jdiEventProcessor.filterValidBallerinaFrames(jStackFrames); if (!validFrames.isEmpty()) { Location currentLocation = validFrames.get(0).getJStackFrame().location(); - if (mode == DynamicBreakpointMode.CURRENT - && !isWithinSameSource(currentLocation, context.getPrevLocation())) { + Optional prevLocation = context.getPrevLocation(); + if (mode == DynamicBreakpointMode.CURRENT && + (prevLocation.isEmpty() || !isWithinSameSource(currentLocation, prevLocation.get()))) { context.getEventManager().deleteAllBreakpoints(); configureBreakpointsForMethod(currentLocation); } @@ -261,9 +262,6 @@ void activateDynamicBreakPoints(int threadId, DynamicBreakpointMode mode) { * @return true if the given two locations are within the same source file, false otherwise */ private boolean isWithinSameSource(Location currentLocation, Location prevLocation) { - if (prevLocation == null) { - return false; - } try { return Objects.equals(currentLocation.sourcePath(), prevLocation.sourcePath()); } catch (AbsentInformationException e) { diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/ExecutionContext.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/ExecutionContext.java index 0c7ff2dbfe49..04d26d2870f7 100644 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/ExecutionContext.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/ExecutionContext.java @@ -113,8 +113,8 @@ public void setPrevInstruction(DebugInstruction prevInstruction) { this.prevInstruction = prevInstruction; } - public Location getPrevLocation() { - return prevLocation; + public Optional getPrevLocation() { + return Optional.ofNullable(prevLocation); } public void setPrevLocation(Location prevLocation) { From 260a048f22823d182775282d315beb647661840e Mon Sep 17 00:00:00 2001 From: Nipuna Ranasinghe Date: Tue, 5 Nov 2024 20:36:00 +0530 Subject: [PATCH 7/9] Fix dynamic breakpoint processing followed by continue commands --- .../debugadapter/BreakpointProcessor.java | 32 +++++++++++-------- .../debugadapter/JBallerinaDebugServer.java | 10 +++--- 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java index 5896da008f3c..8433049daa2f 100644 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/BreakpointProcessor.java @@ -106,7 +106,7 @@ void processBreakpointEvent(BreakpointEvent bpEvent) { // need to internally step out if we are at the last line of a function, in order to ignore having debug hits // on the last line. if (requireStepOut(bpEvent)) { - activateDynamicBreakPoints((int) bpEvent.thread().uniqueID(), DynamicBreakpointMode.CALLER); + activateDynamicBreakPoints((int) bpEvent.thread().uniqueID(), DynamicBreakpointMode.CALLER, true); context.setPrevInstruction(DebugInstruction.STEP_OVER); context.getDebuggeeVM().resume(); } else if (context.getPrevInstruction() != null && context.getPrevInstruction() != DebugInstruction.CONTINUE) { @@ -218,21 +218,23 @@ void activateUserBreakPoints(ReferenceType referenceType, boolean shouldNotify) * Activates dynamic/temporary breakpoints (which will be used to process the STEP-OVER instruction) via Java Debug * Interface(JDI). * - * @param threadId ID of the active java thread in oder to configure dynamic breakpoint on the active stack trace - * @param mode dynamic breakpoint mode + * @param threadId ID of the active java thread in oder to configure dynamic breakpoint on the active stack + * trace. + * @param mode dynamic breakpoint mode. + * @param validate If true, validates whether the dynamic breakpoints are already applied before activating dynamic + * breakpoints. This is to optimize the performance by avoiding redundant breakpoint activations. */ - void activateDynamicBreakPoints(int threadId, DynamicBreakpointMode mode) { - ThreadReferenceProxyImpl threadReference = context.getAdapter().getAllThreads().get(threadId); + void activateDynamicBreakPoints(int threadId, DynamicBreakpointMode mode, boolean validate) { try { + ThreadReferenceProxyImpl threadReference = context.getAdapter().getAllThreads().get(threadId); List jStackFrames = threadReference.frames(); List validFrames = jdiEventProcessor.filterValidBallerinaFrames(jStackFrames); - if (!validFrames.isEmpty()) { + + if (mode == DynamicBreakpointMode.CURRENT && !validFrames.isEmpty()) { Location currentLocation = validFrames.get(0).getJStackFrame().location(); Optional prevLocation = context.getPrevLocation(); - if (mode == DynamicBreakpointMode.CURRENT && - (prevLocation.isEmpty() || !isWithinSameSource(currentLocation, prevLocation.get()))) { - context.getEventManager().deleteAllBreakpoints(); - configureBreakpointsForMethod(currentLocation); + if (!validate || prevLocation.isEmpty() || !isWithinSameSource(currentLocation, prevLocation.get())) { + doActivateDynamicBreakPoints(currentLocation); } context.setPrevLocation(currentLocation); } @@ -241,9 +243,7 @@ void activateDynamicBreakPoints(int threadId, DynamicBreakpointMode mode) { // temporary breakpoint on the location of its invocation. This is supposed to handle the situations where // the user wants to step over on an exit point of the current function. if (mode == DynamicBreakpointMode.CALLER && validFrames.size() > 1) { - context.getEventManager().deleteAllBreakpoints(); - configureBreakpointsForMethod(validFrames.get(1).getJStackFrame().location()); - context.setPrevLocation(validFrames.get(1).getJStackFrame().location()); + doActivateDynamicBreakPoints(validFrames.get(1).getJStackFrame().location()); } } catch (JdiProxyException e) { LOGGER.error(e.getMessage()); @@ -254,6 +254,12 @@ void activateDynamicBreakPoints(int threadId, DynamicBreakpointMode mode) { } } + private void doActivateDynamicBreakPoints(Location location) { + context.getEventManager().deleteAllBreakpoints(); + configureBreakpointsForMethod(location); + context.setPrevLocation(location); + } + /** * Checks whether the given two locations are within the same source file. * diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JBallerinaDebugServer.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JBallerinaDebugServer.java index 93ee2aea3cc9..0d84b61b4a4c 100755 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JBallerinaDebugServer.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/JBallerinaDebugServer.java @@ -1122,14 +1122,14 @@ private void attachToRemoteVM(String hostName, int portName) throws IOException, */ private void prepareFor(DebugInstruction instruction, int threadId) { clearState(); + BreakpointProcessor bpProcessor = eventProcessor.getBreakpointProcessor(); DebugInstruction prevInstruction = context.getPrevInstruction(); if (prevInstruction == DebugInstruction.STEP_OVER && instruction == DebugInstruction.CONTINUE) { - // need to remove dynamic breakpoints and restore user breakpoints, only when stepping over is followed - // by a 'continue' instruction. - eventProcessor.getBreakpointProcessor().restoreUserBreakpoints(); + bpProcessor.restoreUserBreakpoints(); + } else if (prevInstruction == DebugInstruction.CONTINUE && instruction == DebugInstruction.STEP_OVER) { + bpProcessor.activateDynamicBreakPoints(threadId, DynamicBreakpointMode.CURRENT, false); } else if (instruction == DebugInstruction.STEP_OVER) { - // Activates dynamic breakpoints to replicate the step-over behavior. - eventProcessor.getBreakpointProcessor().activateDynamicBreakPoints(threadId, DynamicBreakpointMode.CURRENT); + bpProcessor.activateDynamicBreakPoints(threadId, DynamicBreakpointMode.CURRENT, true); } context.setPrevInstruction(instruction); } From cb2e789d869acf5246e86b8754e5b4e394495ed9 Mon Sep 17 00:00:00 2001 From: Nipuna Ranasinghe Date: Sun, 10 Nov 2024 23:06:40 +0530 Subject: [PATCH 8/9] Fix intermittent failures during breakpoint condition evaluations --- .../main/java/org/ballerinalang/debugadapter/jdi/JDIUtils.java | 1 - 1 file changed, 1 deletion(-) diff --git a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/jdi/JDIUtils.java b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/jdi/JDIUtils.java index 0db0b357b16d..412279b549e3 100644 --- a/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/jdi/JDIUtils.java +++ b/misc/debug-adapter/modules/debug-adapter-core/src/main/java/org/ballerinalang/debugadapter/jdi/JDIUtils.java @@ -45,7 +45,6 @@ public static void disableJDIRequests(ExecutionContext context) { */ public static void enableJDIRequests(ExecutionContext context) { EventRequestManager eventManager = context.getEventManager(); - eventManager.classPrepareRequests().forEach(EventRequest::enable); eventManager.breakpointRequests().forEach(EventRequest::enable); } } From 06e7fa7e6acda38271c49c0c7476497f09bf392a Mon Sep 17 00:00:00 2001 From: Nipuna Ranasinghe Date: Sun, 10 Nov 2024 23:07:16 +0530 Subject: [PATCH 9/9] Update tests with the fixed step-over behaviour --- .../debugger/test/adapter/DebugInstructionTest.java | 8 ++------ .../debugger/test/adapter/RecursiveDebugTest.java | 8 ++++---- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/tests/jballerina-debugger-integration-test/src/test/java/org/ballerinalang/debugger/test/adapter/DebugInstructionTest.java b/tests/jballerina-debugger-integration-test/src/test/java/org/ballerinalang/debugger/test/adapter/DebugInstructionTest.java index 041df59f7acb..af1b60f54820 100644 --- a/tests/jballerina-debugger-integration-test/src/test/java/org/ballerinalang/debugger/test/adapter/DebugInstructionTest.java +++ b/tests/jballerina-debugger-integration-test/src/test/java/org/ballerinalang/debugger/test/adapter/DebugInstructionTest.java @@ -73,12 +73,8 @@ public void breakpointInBetweenStepOverTest() throws BallerinaTestException { debugTestRunner.initDebugSession(DebugUtils.DebuggeeExecutionKind.RUN); Pair debugHitInfo = debugTestRunner.waitForDebugHit(25000); - // Debugger should hit the breakpoint inside the function invocation when trying to step over. - debugTestRunner.resumeProgram(debugHitInfo.getRight(), DebugTestRunner.DebugResumeKind.STEP_OVER); - debugHitInfo = debugTestRunner.waitForDebugHit(10000); - Assert.assertEquals(debugHitInfo.getLeft(), new BallerinaTestDebugPoint(debugTestRunner.testEntryFilePath, 46)); - - // Should go back to the caller function when stepping over again. + // Debugger should not hit the breakpoint inside function invocations when trying to step over the caller + // function. debugTestRunner.resumeProgram(debugHitInfo.getRight(), DebugTestRunner.DebugResumeKind.STEP_OVER); debugHitInfo = debugTestRunner.waitForDebugHit(10000); Assert.assertEquals(debugHitInfo.getLeft(), new BallerinaTestDebugPoint(debugTestRunner.testEntryFilePath, 31)); diff --git a/tests/jballerina-debugger-integration-test/src/test/java/org/ballerinalang/debugger/test/adapter/RecursiveDebugTest.java b/tests/jballerina-debugger-integration-test/src/test/java/org/ballerinalang/debugger/test/adapter/RecursiveDebugTest.java index c9cb706cc6d2..2e2363d8eec6 100644 --- a/tests/jballerina-debugger-integration-test/src/test/java/org/ballerinalang/debugger/test/adapter/RecursiveDebugTest.java +++ b/tests/jballerina-debugger-integration-test/src/test/java/org/ballerinalang/debugger/test/adapter/RecursiveDebugTest.java @@ -63,12 +63,12 @@ public void testStepRequestsOnRecursiveFunctions() throws BallerinaTestException debugHitInfo = debugTestRunner.waitForDebugHit(10000); Assert.assertEquals(debugHitInfo.getLeft(), new BallerinaTestDebugPoint(debugTestRunner.testEntryFilePath, 26)); - // tries STEP_OVER request on the recursive function call line, which should results in a debug hit on the - // user configured breakpoint (since STEP_OVER request should honor any breakpoints which get reached during - // the program execution, before reaching to the next line of the same method). + // tries STEP_OVER request on the recursive function call line, which should not results in a debug hit on the + // user configured breakpoint (since STEP requests should not depend on any breakpoints which get reached + // during the program execution, before reaching to the next line of the same method). debugTestRunner.resumeProgram(debugHitInfo.getRight(), DebugTestRunner.DebugResumeKind.STEP_OVER); debugHitInfo = debugTestRunner.waitForDebugHit(10000); - Assert.assertEquals(debugHitInfo.getLeft(), debugTestRunner.testBreakpoints.get(0)); + Assert.assertEquals(debugHitInfo.getLeft(), new BallerinaTestDebugPoint(debugTestRunner.testEntryFilePath, 26)); // Todo - enable after fixing https://github.com/ballerina-platform/ballerina-lang/issues/34847 // since now the breakpoint is removed, STEP_OVER request should result in a debug hit on next immediate