Allow stepping through disassembly#831
Conversation
|
Thanks for the feature @danthe1st, looks great on feature wise Would it be possible to change highlight color for dark theme ? its bit hard to read
|
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
a53115b to
191da85
Compare
|
@SougandhS I updated this to use the current instruction pointer background color (see the updated screenshots). Do you know whether there is a way to detect when the stack current stack frame is no longer there/when the highlighting should be removed (e.g. when resuming or stepping into a different file)? |
Thanks, much better now
You can achieve this by registering an |
86a12ff to
86d25a3
Compare
86d25a3 to
5935fe2
Compare
|
@SougandhS Hi. Now that I was able to write a test for this as well, I think eclipse-jdt/eclipse.jdt.ui#2708 and this are ready for review. Note that this won't be able to build successfully without eclipse-jdt/eclipse.jdt.ui#2708. |
5935fe2 to
188bb59
Compare
There was a problem hiding this comment.
Pull request overview
Adds prototype support for stepping through and highlighting bytecode instructions in the Class File Editor when debugging into libraries without source attachments (disassembly view).
Changes:
- Add
JDIStackFrame.getCodeIndex()to expose the current bytecode index for the active stack frame. - Introduce a new
JavaStackFrameSourceDisplayAdapterto highlight the corresponding disassembly instruction inClassFileEditor(and absorb prior lambda-selection behavior). - Add a new UI test to verify disassembly highlighting updates when stepping, and bump bundle/plugin versions.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/model/JDIStackFrame.java | Exposes bytecode codeIndex() via new accessor for UI highlighting. |
| org.eclipse.jdt.debug/META-INF/MANIFEST.MF | Bumps core debug bundle version. |
| org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/sourcelookup/LambdaStackFrameSourceDisplayAdapter.java | Removes the old lambda-only source display adapter (logic moved/merged). |
| org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/sourcelookup/JavaStackFrameSourceDisplayAdapter.java | New unified source display adapter: highlights disassembly instructions and handles lambda selection. |
| org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/sourcelookup/JavaDebugShowInAdapterFactory.java | Routes stack frames to the new adapter (and reuses a single instance). |
| org.eclipse.jdt.debug.ui/pom.xml | Bumps UI plugin Maven version. |
| org.eclipse.jdt.debug.ui/META-INF/MANIFEST.MF | Bumps UI bundle version. |
| org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/sourcelookup/ClassFileEditorHighlightingTest.java | Adds regression test covering disassembly instruction highlighting while stepping. |
| org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/AutomatedSuite.java | Registers the new test in the automated suite. |
Comments suppressed due to low confidence (5)
org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/model/JDIStackFrame.java:1762
getCodeIndex()readsfLocationwithout the samesynchronized (fThread)guard used by otherfLocationaccessors (e.g.,getLineNumber). To avoid races withbind(...)updates, please synchronize similarly and consider returning a sentinel (e.g., -1) when the frame is invalid/not suspended rather than always dereferencingfLocation.
public long getCodeIndex() {
return fLocation.codeIndex();
}
org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/sourcelookup/ClassFileEditorHighlightingTest.java:40
- The test method name
test123is not descriptive and doesn’t follow the naming style used in neighboring tests (e.g.,testFindDuplicatesBug565462). Please rename it to something that reflects the behavior under test (e.g., highlighting the current bytecode instruction in the class file editor).
public void test123() throws Exception {
IJavaProject javaProject = getProjectContext();
createLineBreakpoint(21, CLASS_NAME);
org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/sourcelookup/JavaStackFrameSourceDisplayAdapter.java:113
handleLambdaconnects the document provider toeditorInputbut never disconnects it. This can leak provider connections and keep resources pinned; follow the pattern used elsewhere (e.g., connect in a try/finally and always disconnect).
IDocumentProvider provider = JavaUI.getDocumentProvider();
IEditorInput editorInput = sourceRes.getEditorInput();
provider.connect(editorInput);
IDocument document = provider.getDocument(editorInput);
IRegion region = document.getLineInformation(jdiFrame.getLineNumber() - 1);
org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/sourcelookup/JavaStackFrameSourceDisplayAdapter.java:94
ensureListenerRegistered(page)is invoked both indisplaySource(afterhandleClassFilereturns true) and again insidehandleClassFile. This duplication makes the control flow harder to follow; please keep the call in only one place (preferably the caller) so listener registration is centralized.
if (sourceRes.getEditorInput() instanceof IClassFileEditorInput input) {
if (handleClassFile(page, jdiFrame, input)) {
ensureListenerRegistered(page);
return;
}
org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/sourcelookup/JavaDebugShowInAdapterFactory.java:31
- The adapter factory now reuses a single
JavaStackFrameSourceDisplayAdapterinstance across all stack frames/pages. Since the adapter stores mutable state (currentClassFileEditor/currentClassFileFrameand a singleclassFileUnhighlighterRegisteredflag), stepping/highlighting in one workbench window/page can interfere with another, andensureListenerRegisteredwill only ever register a listener for the first window/page encountered. Consider scoping state/listeners per workbench window (or not reusing a single adapter instance).
private JavaStackFrameSourceDisplayAdapter javaStackFrameSourceDisplayAdapter = new JavaStackFrameSourceDisplayAdapter();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ui/ui/org/eclipse/jdt/internal/debug/ui/sourcelookup/JavaStackFrameSourceDisplayAdapter.java
Outdated
Show resolved
Hide resolved
cc8ae8a to
bfb5d76
Compare
|
I implemented some of the suppressed Copilot changes I consider to make sense but didn't implement the change in the review comment as this isn't really code I added. Regarding the information about this being a prototype: I forgot to remove that from the PR description (done now). |
bfb5d76 to
46323d7
Compare
46323d7 to
5d2b6a8
Compare
|
Hi @danthe1st, Can you test the basic stepping with these 2 options enabled
and see anything is breaking or not ?
|
|
Because those instructions are in the same line in the source file, they are skipped with and without statement stepping. Screencast_20260402_102531.webmIf I use a custom source file, it can make a difference. Screencast_20260402_103207.webmBut I noticed some other issues with a custom source file: Screencast_20260402_104025.webmI'll have to take a look at that. |
|
Nevermind, the issue was because I still had the class file open when I replaced the JAR so the content of the editor didn't match the code executed. Screencast_20260402_104623.webmCode: package dep;
public class Dep {
public void a() {
b();
}
void b(){
print(
c(),
"",
d(),
"---"
);
}
String c() {
return "c";
}
String d() {
return "d";
}
void print(Object... args) {
for(Object o : args) {
System.out.println(o);
}
}
} |
Handled this in a new PR 👍 |
I guess my PR should wait for that and I can update it once that's merged. Hence, I'm marking this as a draft for now. |
|
@danthe1st please hold off rebasing, I'll try to adjust the test and I'll rebase in the process. |
07c57f0 to
952b171
Compare
Sorry, I didn't see that until now/I only saw the notification of the other PR being merged |
952b171 to
01948fc
Compare
|
I've adjusted two things in the PR:
When I run the tests with coverage, I've also adjusted the test to have to run only a small part of the code in the UI thread, so I've removed the UI debug suite. |
When I tested it, it didn't seem to be executed at all. |
01948fc to
8f038cb
Compare
I'll try to find where its called. One more thing to check: What happens if I close the class editor and another editor is opened when stepping? Will the unhighlighting work? Or if debugging multiple programs. Or the same program with multiple class files. For the listener probably with different workbench windows we will have a problem too. Likely we need a list of registered listeners / highlighted editors, or some other mechanism that doesn't rely on fields (e.g. a task queue for the clean-up job). For the editors we likely must check if the editor is still opened before removing highlights. I'm also not 100% sure what the life-cycle of Probably this is another case where we should check what platform debug is doing to remove line highlighting when the debug launch is terminated. And it will be good to have a test for terminating the debug launch and checking that the active class editor has no highlight. |
When I tested with what I tried yesterday (a listener that was registered using the
I can check that as well. |
|
@danthe1st if you have time, can you first check where the Java editor line highlight is removed when terminating a Java debug launch? And what the call stacks are to get to there? |
|
If |
8f038cb to
f00010a
Compare
|
I now used |
...ug.tests/tests/org/eclipse/jdt/debug/tests/sourcelookup/ClassFileEditorHighlightingTest.java
Outdated
Show resolved
Hide resolved
...eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/JavaStackFrameEditorPresenter.java
Show resolved
Hide resolved
|
Just the test issue with checking for no highlighting in |
f00010a to
356c27c
Compare
|
I fixed the aforementioned NPE which was caused by |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/JavaStackFrameEditorPresenter.java
Outdated
Show resolved
Hide resolved
...eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/JavaStackFrameEditorPresenter.java
Outdated
Show resolved
Hide resolved
...ug.tests/tests/org/eclipse/jdt/debug/tests/sourcelookup/ClassFileEditorHighlightingTest.java
Outdated
Show resolved
Hide resolved
...ug.tests/tests/org/eclipse/jdt/debug/tests/sourcelookup/ClassFileEditorHighlightingTest.java
Show resolved
Hide resolved
...ug.tests/tests/org/eclipse/jdt/debug/tests/sourcelookup/ClassFileEditorHighlightingTest.java
Outdated
Show resolved
Hide resolved
|
|
||
| public long getCodeIndex() { | ||
| synchronized (fThread) { | ||
| return fLocation.codeIndex(); |
There was a problem hiding this comment.
getCodeIndex() directly calls fLocation.codeIndex() without the RuntimeException handling used by other fLocation accessors (e.g., getLineNumber() catches JDI runtime failures and returns -1 / throws a DebugException when suspended). As-is, VM disconnect/internal JDI errors can bubble up as unchecked exceptions and potentially break UI presentation. Consider mirroring getLineNumber()'s pattern: wrap in try/catch, and either return -1 on failure or throw a DebugException via targetRequestFailed(...) (and update callers accordingly).
| return fLocation.codeIndex(); | |
| try { | |
| return fLocation.codeIndex(); | |
| } catch (RuntimeException e) { | |
| return -1; | |
| } |
There was a problem hiding this comment.
Is there really a point from doing that for codeIndex()? From what I know, this is always just a field and cannot be unavailable (in contrast to e.g. line numbers).
356c27c to
987dba9
Compare
This change adjusts JavaStackFrameEditorPresenter for ClassFileEditor, so that bytecode instructions are highlighted during debug stepping.
987dba9 to
070e8dc
Compare



What it does
This PR allows stepping through the bytecode of disassembled classes in the class file editor.
This is just an unfinished prototype but I wanted to create a (draft) PR early to be able to get feedback and show the basic approach.This depends on another PR in jdt.ui (eclipse-jdt/eclipse.jdt.ui#2708)
@SougandhS Since you have worked on similar things recently, I think you might be interested in that.
How to test
Author checklist