Skip to content

Allow stepping through disassembly#831

Open
danthe1st wants to merge 1 commit intoeclipse-jdt:masterfrom
danthe1st:debug-disassembly
Open

Allow stepping through disassembly#831
danthe1st wants to merge 1 commit intoeclipse-jdt:masterfrom
danthe1st:debug-disassembly

Conversation

@danthe1st
Copy link
Copy Markdown

@danthe1st danthe1st commented Dec 22, 2025

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)

image image

@SougandhS Since you have worked on similar things recently, I think you might be interested in that.

How to test

  • Start Eclipse with this change as well as this patch
  • Step through some code that steps into libraries without a source attachment
  • It automatically jumps to the relevant bytecode instruction(s) corresponding to the stack frame
  • Stepping through normal (non-library) classes and library classes with source attachments should still work

Author checklist

@SougandhS
Copy link
Copy Markdown
Member

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

image

@SougandhS SougandhS added the enhancement New feature or request label Jan 1, 2026
@eclipse-jdt-bot
Copy link
Copy Markdown
Contributor

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

org.eclipse.jdt.debug/META-INF/MANIFEST.MF

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 patch
From 0fc0c7499e1927da534bae46cee3001370717b67 Mon Sep 17 00:00:00 2001
From: Eclipse JDT Bot <jdt-bot@eclipse.org>
Date: Thu, 1 Jan 2026 05:20:08 +0000
Subject: [PATCH] Version bump(s) for 4.39 stream


diff --git a/org.eclipse.jdt.debug/META-INF/MANIFEST.MF b/org.eclipse.jdt.debug/META-INF/MANIFEST.MF
index 9afbed70f..bef1c1bbb 100644
--- a/org.eclipse.jdt.debug/META-INF/MANIFEST.MF
+++ b/org.eclipse.jdt.debug/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.jdt.debug; singleton:=true
-Bundle-Version: 3.25.0.qualifier
+Bundle-Version: 3.25.100.qualifier
 Bundle-ClassPath: jdimodel.jar
 Bundle-Activator: org.eclipse.jdt.internal.debug.core.JDIDebugPlugin
 Bundle-Vendor: %providerName
-- 
2.52.0

Further information are available in Common Build Issues - Missing version increments.

@danthe1st danthe1st force-pushed the debug-disassembly branch 4 times, most recently from a53115b to 191da85 Compare January 5, 2026 10:57
@danthe1st
Copy link
Copy Markdown
Author

@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)?

@SougandhS
Copy link
Copy Markdown
Member

@SougandhS I updated this to use the current instruction pointer background color (see the updated screenshots).

Thanks, much better now

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)?

You can achieve this by registering an IDebugContextListener and check that stackframe's status accordingly

@danthe1st danthe1st force-pushed the debug-disassembly branch 4 times, most recently from 86a12ff to 86d25a3 Compare January 10, 2026 15:22
@danthe1st danthe1st marked this pull request as ready for review January 10, 2026 15:31
@danthe1st
Copy link
Copy Markdown
Author

@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.
Also, as you probably see, I am not that experienced with the tests here ;).

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 JavaStackFrameSourceDisplayAdapter to highlight the corresponding disassembly instruction in ClassFileEditor (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() reads fLocation without the same synchronized (fThread) guard used by other fLocation accessors (e.g., getLineNumber). To avoid races with bind(...) updates, please synchronize similarly and consider returning a sentinel (e.g., -1) when the frame is invalid/not suspended rather than always dereferencing fLocation.
	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 test123 is 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

  • handleLambda connects the document provider to editorInput but 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 in displaySource (after handleClassFile returns true) and again inside handleClassFile. 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 JavaStackFrameSourceDisplayAdapter instance across all stack frames/pages. Since the adapter stores mutable state (currentClassFileEditor/currentClassFileFrame and a single classFileUnhighlighterRegistered flag), stepping/highlighting in one workbench window/page can interfere with another, and ensureListenerRegistered will 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.

@danthe1st
Copy link
Copy Markdown
Author

danthe1st commented Feb 28, 2026

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).

@SougandhS
Copy link
Copy Markdown
Member

SougandhS commented Apr 2, 2026

Hi @danthe1st, Can you test the basic stepping with these 2 options enabled

image and see anything is breaking or not ?

Copy link
Copy Markdown
Member

@SougandhS SougandhS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image

These load instructions will be skipped (wont be suspended) when IJDIPreferencesConstants.PREF_STATEMENT_LEVEL_STEPPING is enabled. - Can you confirm it ?

@danthe1st
Copy link
Copy Markdown
Author

danthe1st commented Apr 2, 2026

Because those instructions are in the same line in the source file, they are skipped with and without statement stepping.

Screencast_20260402_102531.webm

If I use a custom source file, it can make a difference.

Screencast_20260402_103207.webm

But I noticed some other issues with a custom source file:

Screencast_20260402_104025.webm

I'll have to take a look at that.

@danthe1st
Copy link
Copy Markdown
Author

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.webm

Code:

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);
		}
	}
}

@danthe1st
Copy link
Copy Markdown
Author

danthe1st commented Apr 2, 2026

When I compile the file with debugging symbols (javac -g:none), "Step Into" skips until the first method call to somewhere else which isn't ideal but this isn't new with my implementation (if that should be fixed, I guess this would make sense as a distinct change to keep the PRs small):

Screencast_20260402_105200.webm

Other than that, it can step over individual bytecode instructions if no debugging information (like line numbers) is present:

Screencast_20260402_105542.webm

(It cannot properly highlight it in printStream() because the attached source (Temurin) doesn't perfectly match the class files (GraalVM) there.)

If wanted, I can also try to add some tests for things like stepping without debugging information.

@SougandhS
Copy link
Copy Markdown
Member

if that should be fixed, I guess this would make sense as a distinct change to keep the PRs small)

OK for me 👍

Other than that, it can step over individual bytecode instructions if no debugging information (like line numbers) is present:

Sounds nice

If wanted, I can also try to add some tests for things like stepping without debugging information.

That would be good. please add few more tests if possible

@danthe1st danthe1st force-pushed the debug-disassembly branch from 5d2b6a8 to d473972 Compare April 2, 2026 12:28
@danthe1st
Copy link
Copy Markdown
Author

I added another test for the situation with no debugging information in the class files (I compiled it using the JDK APIs there). If more/different tests are desired, please tell me what else you think should have a regression test.

@danthe1st danthe1st force-pushed the debug-disassembly branch 3 times, most recently from 43213a7 to 41bb3c5 Compare April 3, 2026 11:36
@danthe1st
Copy link
Copy Markdown
Author

The above pushes update to a change in my JDT.ui PR and make sure that it also works correctly on constructors.

@danthe1st
Copy link
Copy Markdown
Author

Since the JDU-UI changes are in, I am now getting actual test failures (it seems like a problem in how I wrote these tests/how the tests/workbench are initialized). I plan to check that tomorrow.

@danthe1st danthe1st force-pushed the debug-disassembly branch from b1717b8 to 65ad692 Compare April 8, 2026 16:14
@danthe1st
Copy link
Copy Markdown
Author

The tests failed with Jenkins because AutomatedSuite didn't run them in the UI thread. For now, I created a new TestSuite for this. What is the recommended approach for tests that should run fully in the UI thread?
Should I also move the test class to a different package?

@trancexpress
Copy link
Copy Markdown
Contributor

Odd, this seems to want the UI thread:

➜  org.eclipse.jdt.debug.tests git:(master) git grep useUIThread
pom.xml:          <useUIThread>true</useUIThread>

The shared launch also specifies running in the UI thread.

What is the error message from the test?

@danthe1st
Copy link
Copy Markdown
Author

danthe1st commented Apr 8, 2026

What is the error message from the test?

org.eclipse.ui.PartInitException: No active workbench page
	at org.eclipse.jdt.internal.ui.javaeditor.EditorUtility.openInEditor(EditorUtility.java:388)
	at org.eclipse.jdt.internal.ui.javaeditor.EditorUtility.openInSpecificEditor(EditorUtility.java:191)
	at org.eclipse.jdt.internal.ui.javaeditor.EditorUtility.openInEditor(EditorUtility.java:169)
	at org.eclipse.jdt.internal.ui.javaeditor.EditorUtility.openInEditor(EditorUtility.java:154)
	at org.eclipse.jdt.debug.tests.sourcelookup.ClassFileEditorHighlightingTest.testConstructorInPackage(ClassFileEditorHighlightingTest.java:99)
	at java.base/java.lang.reflect.Method.invoke(Method.java:565)
	at junit.framework.TestCase.runTest(TestCase.java:177)
	at junit.framework.TestCase.runBare(TestCase.java:142)
	at org.eclipse.jdt.debug.tests.AbstractDebugTest.runBare(AbstractDebugTest.java:2993)
	at junit.framework.TestResult$1.protect(TestResult.java:122)
	at junit.framework.TestResult.runProtected(TestResult.java:142)
	at junit.framework.TestResult.run(TestResult.java:125)
	at junit.framework.TestCase.run(TestCase.java:130)
	at junit.framework.TestSuite.runTest(TestSuite.java:241)
	at junit.framework.TestSuite.run(TestSuite.java:236)
	at junit.framework.TestSuite.runTest(TestSuite.java:241)
	at org.eclipse.jdt.debug.tests.DebugSuite$1.run(DebugSuite.java:61)
	at java.base/java.lang.Thread.run(Thread.java:1516)

See https://github.com/eclipse-jdt/eclipse.jdt.debug/runs/70316773248

AutomatedSuite extends DebugSuite which runs all tests in a non-UI thread.

/**
 * Debug test suite framework that runs test in a non UI thread.
 */
public abstract class DebugSuite extends TestSuite {

I could also move the test to a different test suite but I don't know which one would be better.

@trancexpress
Copy link
Copy Markdown
Contributor

trancexpress commented Apr 8, 2026

I'm not sure, maybe the non-UI thread choice was made due t o EventWaiter not processing UI events while waiting... DebugEventWaiter on the other hand does process UI events...

From my POV no need to overcomplicate things for one test case, what is in the PR right now seems OK. If we find that the UI thread test suite is growing, we can have two top level test suites, where the UI one doesn't run tests in a separate thread.

@danthe1st
Copy link
Copy Markdown
Author

I think there's also EvalTestSuite which apparently does some UI tests but I don't know about that one and running those locally didn't seem to work properly for me (and I'm not sure whether that runs the tests fully in the UI thread or just does some additional processing for handling UI events).

} catch (DebugException e) {
DebugUIPlugin.log(e);
if (adaptableObject instanceof JDIStackFrame) {
return (T) javaStackFrameSourceDisplayAdapter;// ensure the same instance is reused
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add to the comment why it should be the same instance.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the case because T is ISourceDisplay (checked by the if) which is the type of javaStackFrameSourceDisplayAdapter (I now changed that field to use the interface as well). I hope this is clear now.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but I still don't understand the comment with the re-use. Is it for efficiency, not creating the object many times? Or there is some state that is important?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because it stores state, specifically information about/for the listener ensuring that the highlighting is cleaned up when the instruction pointer is no longer at the class file.

}
}

private boolean classFileUnhighlighterRegistered = false;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is checked in the UI thread, but set from a non-UI thread? I don't think debug events are propagated in the UI thread at least and display source sounds like UI thread code.

Can you debug and check? Best to use AtomicBoolean if that is the case. Or maybe volatile is enough.

I'm also not sure what happens if a debug event unregisters the listener concurrently to the code checking it it should be registered.

I'm also not sure about displaySource just running the UI code, e.g. org.eclipse.debug.internal.ui.elements.adapters.StackFrameSourceDisplayAdapter.displaySource(Object, IWorkbenchPage, boolean) schedules a job to do that...

Copy link
Copy Markdown
Author

@danthe1st danthe1st Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is called in a UI thread, at least in my testing.
image

Copy link
Copy Markdown
Contributor

@trancexpress trancexpress Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through StackFrameSourceDisplayAdapter again, it looks like the job is started to not run the source look up in the UI thread.

So likely this is a problem already with: #812

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched to using synchronized in b07279b.

The reason for using synchronized over volatile/atomic variables is that there are multiple variables involved and if something tries to call displaySource() concurrently for whatever reason (I don't think this should happen under normal circumstances but I don't know for sure), it should still ensure that these variables are updated together and there's no hard to debug/reproduce in-between state.

ClassFileEditor editor = (ClassFileEditor) EditorUtility.openInEditor(javaProject.getProject().getFile("bin/" + CLASS_NAME + ".class"));
IClassFileEditorInput editorInput = (IClassFileEditorInput) editor.getEditorInput();
IClassFile classFile = editorInput.getClassFile();
thread.getTopStackFrame().getLaunch().setSourceLocator(stackFrame -> classFile);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary?

Copy link
Copy Markdown
Author

@danthe1st danthe1st Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sure it actually uses the ClassFileEditor for displaying the source.
This is necessary because the source file is present in the project and otherwise not show the bytecode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants