From 3e71ee7a59531125d36e6f94891c68a36b0e739a Mon Sep 17 00:00:00 2001 From: Shuhei Takahashi Date: Thu, 30 Jan 2025 23:13:26 +0900 Subject: [PATCH 1/2] Improve variables view We used to have two scopes of variables, "locals" and "arguments". However, VM somehow returns incorrect results for LocalVariable#isArgument(), so we presented confusing views to users. Since it's not very important to distinguish arguments and local variables, we now include them in the same "locals" scope. Instead, we introduce the new "fields" scope containing fields of the current object (aka "this"). And finally, we now support inspecting the hierarchy of objects. --- .../org/javacs/debug/JavaDebugServer.java | 151 +++++++++++++++--- src/test/examples/debug/DeepVariables.class | Bin 0 -> 844 bytes src/test/examples/debug/DeepVariables.java | 14 ++ .../java/org/javacs/JavaDebugServerTest.java | 66 +++++++- 4 files changed, 212 insertions(+), 19 deletions(-) create mode 100644 src/test/examples/debug/DeepVariables.class create mode 100644 src/test/examples/debug/DeepVariables.java diff --git a/src/main/java/org/javacs/debug/JavaDebugServer.java b/src/main/java/org/javacs/debug/JavaDebugServer.java index 53d36ce68..61dd3d04d 100644 --- a/src/main/java/org/javacs/debug/JavaDebugServer.java +++ b/src/main/java/org/javacs/debug/JavaDebugServer.java @@ -13,6 +13,7 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.util.ArrayList; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Objects; @@ -440,6 +441,7 @@ public void terminate(TerminateArguments req) { @Override public void continue_(ContinueArguments req) { + valueIdTracker.clear(); vm.resume(); } @@ -454,6 +456,7 @@ public void next(NextArguments req) { var step = vm.eventRequestManager().createStepRequest(thread, StepRequest.STEP_LINE, StepRequest.STEP_OVER); step.addCountFilter(1); step.enable(); + valueIdTracker.clear(); vm.resume(); } @@ -468,6 +471,7 @@ public void stepIn(StepInArguments req) { var step = vm.eventRequestManager().createStepRequest(thread, StepRequest.STEP_LINE, StepRequest.STEP_INTO); step.addCountFilter(1); step.enable(); + valueIdTracker.clear(); vm.resume(); } @@ -482,6 +486,7 @@ public void stepOut(StepOutArguments req) { var step = vm.eventRequestManager().createStepRequest(thread, StepRequest.STEP_LINE, StepRequest.STEP_OUT); step.addCountFilter(1); step.enable(); + valueIdTracker.clear(); vm.resume(); } @@ -632,47 +637,157 @@ private com.sun.jdi.StackFrame findFrame(long id) { @Override public ScopesResponseBody scopes(ScopesArguments req) { var resp = new ScopesResponseBody(); + var fields = new Scope(); + fields.name = "Fields"; + fields.presentationHint = "locals"; + fields.expensive = true; // do not expand by default + fields.variablesReference = req.frameId * 2; var locals = new Scope(); locals.name = "Locals"; locals.presentationHint = "locals"; - locals.variablesReference = req.frameId * 2; - var arguments = new Scope(); - arguments.name = "Arguments"; - arguments.presentationHint = "arguments"; - arguments.variablesReference = req.frameId * 2 + 1; - resp.scopes = new Scope[] {locals, arguments}; + locals.variablesReference = req.frameId * 2 + 1; + resp.scopes = new Scope[] {fields, locals}; return resp; } + private static final long VALUE_ID_START = 1000000000; + + private static class ValueIdTracker { + private final HashMap values = new HashMap<>(); + private long nextId = VALUE_ID_START; + + public void clear() { + values.clear(); + // Keep nextId to avoid accidentally accessing wrong Values. + } + + public Value get(long id) { + return values.get(id); + } + + public long put(Value value) { + long id = nextId++; + values.put(id, value); + return id; + } + } + + private final ValueIdTracker valueIdTracker = new ValueIdTracker(); + + private static boolean hasInterestingChildren(Value value) { + return value instanceof ObjectReference && !(value instanceof StringReference); + } + @Override public VariablesResponseBody variables(VariablesArguments req) { - var frameId = req.variablesReference / 2; - var scopeId = (int) (req.variablesReference % 2); - var argumentScope = scopeId == 1; + if (req.variablesReference < VALUE_ID_START) { + var frameId = req.variablesReference / 2; + var scopeId = (int)(req.variablesReference % 2); + return frameVariables(frameId, scopeId); + } + Value value = valueIdTracker.get(req.variablesReference); + return valueChildren(value); + } + + private VariablesResponseBody frameVariables(long frameId, int scopeId) { var frame = findFrame(frameId); + var thread = frame.thread(); + var variables = new ArrayList(); + + if (scopeId == 0) { + var thisValue = frame.thisObject(); + if (thisValue != null) { + variables.addAll(objectFieldsAsVariables(thisValue, thread)); + } + } else { + variables.addAll(frameLocalsAsVariables(frame, thread)); + } + + var resp = new VariablesResponseBody(); + resp.variables = variables.toArray(Variable[]::new); + return resp; + } + + private VariablesResponseBody valueChildren(Value parentValue) { + // TODO: Use an actual owner thread. + ThreadReference mainThread = vm.allThreads().get(0); + var variables = new ArrayList(); + + if (parentValue instanceof ArrayReference array) { + variables.addAll(arrayElementsAsVariables(array, mainThread)); + } else if (parentValue instanceof ObjectReference object) { + variables.addAll(objectFieldsAsVariables(object, mainThread)); + } + + var resp = new VariablesResponseBody(); + resp.variables = variables.toArray(Variable[]::new); + return resp; + } + + private List frameLocalsAsVariables(com.sun.jdi.StackFrame frame, ThreadReference thread) { List visible; try { visible = frame.visibleVariables(); } catch (AbsentInformationException __) { LOG.warning(String.format("No visible variable information in %s", frame.location())); - return new VariablesResponseBody(); + return List.of(); } - var values = frame.getValues(visible); - var thread = frame.thread(); + var variables = new ArrayList(); + var values = frame.getValues(visible); for (var v : visible) { - if (v.isArgument() != argumentScope) continue; + var value = values.get(v); var w = new Variable(); w.name = v.name(); - w.value = print(values.get(v), thread); + w.value = print(value, thread); w.type = v.typeName(); - // TODO set variablesReference and allow inspecting structure of collections and POJOs + if (hasInterestingChildren(value)) { + w.variablesReference = valueIdTracker.put(value); + } + if (value instanceof ArrayReference array) { + w.indexedVariables = array.length(); + } // TODO set variablePresentationHint variables.add(w); } - var resp = new VariablesResponseBody(); - resp.variables = variables.toArray(Variable[]::new); - return resp; + return variables; + } + + private List arrayElementsAsVariables(ArrayReference array, ThreadReference thread) { + var variables = new ArrayList(); + var arrayType = (ArrayType) array.type(); + var values = array.getValues(); + var length = values.size(); + for (int i = 0; i < length; i++) { + var value = values.get(i); + var w = new Variable(); + w.name = Integer.toString(i, 10); + w.value = print(value, thread); + w.type = arrayType.componentTypeName(); + if (hasInterestingChildren(value)) { + w.variablesReference = valueIdTracker.put(value); + } + variables.add(w); + } + return variables; + } + + private List objectFieldsAsVariables(ObjectReference object, ThreadReference thread) { + var variables = new ArrayList(); + var classType = (ClassType) object.type(); + var values = object.getValues(classType.allFields()); + for (var field : values.keySet()) { + var value = values.get(field); + var w = new Variable(); + w.name = field.name(); + w.value = print(value, thread); + w.type = field.typeName(); + if (hasInterestingChildren(value)) { + w.variablesReference = valueIdTracker.put(value); + } + variables.add(w); + } + return variables; } private String print(Value value, ThreadReference t) { diff --git a/src/test/examples/debug/DeepVariables.class b/src/test/examples/debug/DeepVariables.class new file mode 100644 index 0000000000000000000000000000000000000000..acb420536a0e27652924040bdf9a0655269e7310 GIT binary patch literal 844 zcmZuv+iuf95Iq|wapE|aHYD6Hg;Jap8sG(?AYO`)DusXuN>%aXgjLwev5PNAd=@;k zh)R3_AB8yU1R9IV^6bv+*)wNm_Sf$pKLI?!V+%PLCh|54C<-(V-Op~%bNyNG_2f`a zV}ar`<*WF)fZ=q;CQ1U;7g8RL-B7s`Pev9@7}`{9*qXx#6W>HlpqasK?)kn9EmUEY zv4(XMO&b@`$|N0qiDEe?N02aeYmmxOLGMkd{CE_H(w+A$T*M_4mu+0ZRe`k>lf=qf zN{$3-q5PDnP!0osn0TJRmb0K*sjT1mKV;!LZkV`f;}$kpz&Ys!%p)zv^9AzGUT4f@ zH~1h0>I3D=VKSe{@SX0A!1^GVy58STEra>^qoTz6Ak&>bY38mXY1?_fB8)XFf7Vaa zxZ#W!o97&JO2Kkmtt{`N>twe^K@v{oOO*=C Date: Thu, 20 Mar 2025 01:07:13 +0900 Subject: [PATCH 2/2] Fix breakpoints in classes with inner/anonymous classes Existing breakpoint installation logic assumes that there is one-to-one correspondence between a class and a Java source file. But it does not hold for classes with inner classes and/or anonymous classes. This patch fixes the logic to handle the case. Also adds a regression test. --- .../org/javacs/debug/JavaDebugServer.java | 151 ++++++++++-------- .../examples/debug/InnerClasses$Inner$1.class | Bin 0 -> 777 bytes .../examples/debug/InnerClasses$Inner.class | Bin 0 -> 657 bytes src/test/examples/debug/InnerClasses.class | Bin 0 -> 678 bytes src/test/examples/debug/InnerClasses.java | 20 +++ .../java/org/javacs/JavaDebugServerTest.java | 53 ++++-- 6 files changed, 145 insertions(+), 79 deletions(-) create mode 100644 src/test/examples/debug/InnerClasses$Inner$1.class create mode 100644 src/test/examples/debug/InnerClasses$Inner.class create mode 100644 src/test/examples/debug/InnerClasses.class create mode 100644 src/test/examples/debug/InnerClasses.java diff --git a/src/main/java/org/javacs/debug/JavaDebugServer.java b/src/main/java/org/javacs/debug/JavaDebugServer.java index 61dd3d04d..ecf2ea4f6 100644 --- a/src/main/java/org/javacs/debug/JavaDebugServer.java +++ b/src/main/java/org/javacs/debug/JavaDebugServer.java @@ -7,6 +7,10 @@ import com.sun.jdi.request.BreakpointRequest; import com.sun.jdi.request.EventRequest; import com.sun.jdi.request.StepRequest; + +import org.javacs.LogFormat; +import org.javacs.debug.proto.*; + import java.io.IOException; import java.net.ConnectException; import java.nio.file.Files; @@ -19,8 +23,6 @@ import java.util.Objects; import java.util.Set; import java.util.logging.*; -import org.javacs.LogFormat; -import org.javacs.debug.proto.*; public class JavaDebugServer implements DebugServer { public static void main(String[] args) { // TODO don't show references for main method @@ -34,7 +36,9 @@ private static void createLogFile() { try { // TODO make location configurable var logFile = - new FileHandler("/Users/georgefraser/Documents/java-language-server/java-debug-server.log", false); + new FileHandler( + "/Users/georgefraser/Documents/java-language-server/java-debug-server.log", + false); logFile.setFormatter(new LogFormat()); Logger.getLogger("").addHandler(logFile); } catch (IOException e) { @@ -73,7 +77,11 @@ private void process(com.sun.jdi.event.Event event) { if (event instanceof ClassPrepareEvent) { var prepare = (ClassPrepareEvent) event; var type = prepare.referenceType(); - LOG.info("ClassPrepareRequest for class " + type.name() + " in source " + relativePath(type)); + LOG.info( + "ClassPrepareRequest for class " + + type.name() + + " in source " + + relativePath(type)); enablePendingBreakpointsIn(type); vm.resume(); } else if (event instanceof com.sun.jdi.event.BreakpointEvent) { @@ -145,7 +153,10 @@ public SetBreakpointsResponseBody setBreakpoints(SetBreakpointsArguments req) { private void disableBreakpoints(Source source) { for (var b : vm.eventRequestManager().breakpointRequests()) { if (matchesFile(b, source)) { - LOG.info(String.format("Disable breakpoint %s:%d", source.path, b.location().lineNumber())); + LOG.info( + String.format( + "Disable breakpoint %s:%d", + source.path, b.location().lineNumber())); b.disable(); } } @@ -159,8 +170,9 @@ private Breakpoint enableBreakpoint(Source source, SourceBreakpoint b) { } } // Check for breakpoint in loaded classes - for (var type : loadedTypesMatching(source.path)) { - return enableBreakpointImmediately(source, b, type); + var enabled = enableBreakpointImmediately(source, b); + if (enabled != null) { + return enabled; } // If class hasn't been loaded, add breakpoint to pending list return enableBreakpointLater(source, b); @@ -192,7 +204,10 @@ private List loadedTypesMatching(String absolutePath) { } private Breakpoint enableDisabledBreakpoint(Source source, BreakpointRequest b) { - LOG.info(String.format("Enable disabled breakpoint %s:%d", source.path, b.location().lineNumber())); + LOG.info( + String.format( + "Enable disabled breakpoint %s:%d", + source.path, b.location().lineNumber())); b.enable(); var ok = new Breakpoint(); ok.verified = true; @@ -201,27 +216,20 @@ private Breakpoint enableDisabledBreakpoint(Source source, BreakpointRequest b) return ok; } - private Breakpoint enableBreakpointImmediately(Source source, SourceBreakpoint b, ReferenceType type) { - if (!tryEnableBreakpointImmediately(source, b, type)) { - var failed = new Breakpoint(); - failed.verified = false; - failed.message = source.name + ":" + b.line + " could not be found or had no code on it"; - return failed; + private Breakpoint enableBreakpointImmediately(Source source, SourceBreakpoint b) { + ArrayList locations = new ArrayList<>(); + for (var type : loadedTypesMatching(source.path)) { + try { + locations.addAll(type.locationsOfLine(b.line)); + } catch (AbsentInformationException __) { + LOG.info( + String.format( + "No locations in %s for breakpoint %s:%d", + type.name(), source.path, b.line)); + } } - var ok = new Breakpoint(); - ok.verified = true; - ok.source = source; - ok.line = b.line; - return ok; - } - - private boolean tryEnableBreakpointImmediately(Source source, SourceBreakpoint b, ReferenceType type) { - List locations; - try { - locations = type.locationsOfLine(b.line); - } catch (AbsentInformationException __) { - LOG.info(String.format("No locations in %s for breakpoint %s:%d", type.name(), source.path, b.line)); - return false; + if (locations.isEmpty()) { + return null; } for (var l : locations) { LOG.info(String.format("Create breakpoint %s:%d", source.path, l.lineNumber())); @@ -229,7 +237,11 @@ private boolean tryEnableBreakpointImmediately(Source source, SourceBreakpoint b req.setSuspendPolicy(EventRequest.SUSPEND_ALL); req.enable(); } - return true; + var ok = new Breakpoint(); + ok.verified = true; + ok.source = source; + ok.line = b.line; + return ok; } private Breakpoint enableBreakpointLater(Source source, SourceBreakpoint b) { @@ -247,7 +259,8 @@ private Breakpoint enableBreakpointLater(Source source, SourceBreakpoint b) { } @Override - public SetFunctionBreakpointsResponseBody setFunctionBreakpoints(SetFunctionBreakpointsArguments req) { + public SetFunctionBreakpointsResponseBody setFunctionBreakpoints( + SetFunctionBreakpointsArguments req) { LOG.warning("Not yet implemented"); return new SetFunctionBreakpointsResponseBody(); } @@ -297,7 +310,8 @@ private static AttachingConnector connector(String transport) { } found.add(conn.transport().name()); } - throw new RuntimeException("Couldn't find connector for transport " + transport + " in " + found); + throw new RuntimeException( + "Couldn't find connector for transport " + transport + " in " + found); } @Override @@ -369,48 +383,36 @@ private void enablePendingBreakpointsIn(ReferenceType type) { // Look for pending breakpoints that can be enabled var enabled = new ArrayList(); for (var b : pendingBreakpoints) { - if (b.source.path.endsWith(path)) { - enablePendingBreakpoint(b, type); + if (b.source.path.endsWith(path) && enablePendingBreakpoint(b, type)) { enabled.add(b); } } pendingBreakpoints.removeAll(enabled); } - private void enablePendingBreakpoint(Breakpoint b, ReferenceType type) { + private boolean enablePendingBreakpoint(Breakpoint b, ReferenceType type) { + List locations; try { - var locations = type.locationsOfLine(b.line); - for (var line : locations) { - var req = vm.eventRequestManager().createBreakpointRequest(line); - req.setSuspendPolicy(EventRequest.SUSPEND_ALL); - req.enable(); - } - if (locations.isEmpty()) { - LOG.info("No locations at " + b.source.path + ":" + b.line); - var failed = new BreakpointEventBody(); - failed.reason = "changed"; - failed.breakpoint = b; - b.verified = false; - b.message = b.source.name + ":" + b.line + " could not be found or had no code on it"; - client.breakpoint(failed); - return; - } - LOG.info("Enable breakpoint at " + b.source.path + ":" + b.line); - var ok = new BreakpointEventBody(); - ok.reason = "changed"; - ok.breakpoint = b; - b.verified = true; - b.message = null; - client.breakpoint(ok); + locations = type.locationsOfLine(b.line); } catch (AbsentInformationException __) { - LOG.info("Absent information at " + b.source.path + ":" + b.line); - var failed = new BreakpointEventBody(); - failed.reason = "changed"; - failed.breakpoint = b; - b.verified = false; - b.message = b.source.name + ":" + b.line + " could not be found or had no code on it"; - client.breakpoint(failed); + return false; + } + if (locations.isEmpty()) { + return false; } + for (var line : locations) { + var req = vm.eventRequestManager().createBreakpointRequest(line); + req.setSuspendPolicy(EventRequest.SUSPEND_ALL); + req.enable(); + } + LOG.info("Enable breakpoint at " + b.source.path + ":" + b.line); + var ok = new BreakpointEventBody(); + ok.reason = "changed"; + ok.breakpoint = b; + b.verified = true; + b.message = null; + client.breakpoint(ok); + return true; } private String relativePath(ReferenceType type) { @@ -453,7 +455,9 @@ public void next(NextArguments req) { return; } LOG.info("Send StepRequest(STEP_LINE, STEP_OVER) to VM and resume"); - var step = vm.eventRequestManager().createStepRequest(thread, StepRequest.STEP_LINE, StepRequest.STEP_OVER); + var step = + vm.eventRequestManager() + .createStepRequest(thread, StepRequest.STEP_LINE, StepRequest.STEP_OVER); step.addCountFilter(1); step.enable(); valueIdTracker.clear(); @@ -468,7 +472,9 @@ public void stepIn(StepInArguments req) { return; } LOG.info("Send StepRequest(STEP_LINE, STEP_INTO) to VM and resume"); - var step = vm.eventRequestManager().createStepRequest(thread, StepRequest.STEP_LINE, StepRequest.STEP_INTO); + var step = + vm.eventRequestManager() + .createStepRequest(thread, StepRequest.STEP_LINE, StepRequest.STEP_INTO); step.addCountFilter(1); step.enable(); valueIdTracker.clear(); @@ -483,7 +489,9 @@ public void stepOut(StepOutArguments req) { return; } LOG.info("Send StepRequest(STEP_LINE, STEP_OUT) to VM and resume"); - var step = vm.eventRequestManager().createStepRequest(thread, StepRequest.STEP_LINE, StepRequest.STEP_OUT); + var step = + vm.eventRequestManager() + .createStepRequest(thread, StepRequest.STEP_LINE, StepRequest.STEP_OUT); step.addCountFilter(1); step.enable(); valueIdTracker.clear(); @@ -682,7 +690,7 @@ private static boolean hasInterestingChildren(Value value) { public VariablesResponseBody variables(VariablesArguments req) { if (req.variablesReference < VALUE_ID_START) { var frameId = req.variablesReference / 2; - var scopeId = (int)(req.variablesReference % 2); + var scopeId = (int) (req.variablesReference % 2); return frameVariables(frameId, scopeId); } Value value = valueIdTracker.get(req.variablesReference); @@ -724,7 +732,8 @@ private VariablesResponseBody valueChildren(Value parentValue) { return resp; } - private List frameLocalsAsVariables(com.sun.jdi.StackFrame frame, ThreadReference thread) { + private List frameLocalsAsVariables( + com.sun.jdi.StackFrame frame, ThreadReference thread) { List visible; try { visible = frame.visibleVariables(); @@ -808,7 +817,9 @@ private String printObject(ObjectReference object, ThreadReference t) { return string.value(); } catch (InvocationException e) { return String.format("toString() threw %s", e.exception().type().name()); - } catch (InvalidTypeException | ClassNotLoadedException | IncompatibleThreadStateException e) { + } catch (InvalidTypeException + | ClassNotLoadedException + | IncompatibleThreadStateException e) { throw new RuntimeException(e); } } diff --git a/src/test/examples/debug/InnerClasses$Inner$1.class b/src/test/examples/debug/InnerClasses$Inner$1.class new file mode 100644 index 0000000000000000000000000000000000000000..e9c9b6a1e20912a71566ba980c1e4207684a7340 GIT binary patch literal 777 zcmZva&2G~`6otGOVj${^VE$CXmH1I-C;w0liW(XH;dYBi^n(3ouTU0w%nR%6D#>Jk1Kd%$06e%`&GingH z3lRkNcJRWK_%7i-;{(nP8SRSK+4&05`40DUp?S!$%1Xc|1?)|s!XrFpMtwmO*y6lG z*{6VF`r>G@2bA05`WM`2u6)DVHPtIf4f+ZSvMr(TitJ#USqV?@l+i(m4t9!_UFFaj0@8Tn&nrU_-k)|?jWI^Mb zhZWBiW8IGLa7!*W3Bg+^A>WCwB%$0@TAoeD1DRZiLByG$8xKX) z7m2d*Ycg+cRLXt5_bS2b#gk+xkCf$CW?$R4e?Tan$VS-H;Emca|$X6wHI@EKn?B0D}?OFo5@)vCYX1|Oy3x6jBu;0r6>)4sm2NE4!OQe^z}^ zsNfIqM~QbPf)O&X_v74i&)mDaPtX4!0UY3)ha4Oic^?H73A5MYk7z_f_Zz3(YuU>P z#qUb1><7WA)jK6jq2$8zF%AD6z5O@MM2r*9xCp0km#B3&I~(gM-EY=AE*1#EPpxHg7>P8MY1JkkDsYVX z37(|in3TF-W#J;d|5Bg`zt87gf{H(F{M+B_12vEWXHW@L^M^+37 zZ{Y?X?lAL!%Edhv@9^