Repository navigation
Fix statement stepping skipping constant assignments - #1058
Merged
Merged
Conversation
With statement-level stepping enabled, a line was skipped as "intermediate" if it started with a load/constant push instruction and the next line location was 1-3 bytes away. That is also true for simple statements like "result = false;" (iconst_0; istore_1), so the debugger never stopped on such assignments. Commit 8904bd1 only handled the case where the instruction after the push is a return. Now a line is only skipped if every instruction up to the next line location is a load/constant push. Any other instruction (store, return, branch, invoke, ...) means the line is a real statement and must not be skipped. Fixes eclipse-jdt#1053 Assisted-by: Copilot with Claude Opus 5
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the constant-assignment stepping fix.
No open issues 🎉
What changed in this PR
Fixes statement-level stepping so the debugger stops on constant assignments instead of treating them as intermediate instructions. This extends the return-statement fix for #1053.
Changes:
- Skip a short bytecode range only when every instruction is a load or constant push.
- Add regression coverage for stepping into and over constant assignments.
- Explicitly enable step filters in the return and assignment regression tests.
| File | Description |
|---|---|
org.eclipse.jdt.debug/model/org/eclipse/jdt/internal/debug/core/model/JDIThread.java |
Checks the entire bytecode range before skipping a location. |
org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/AbstractDebugTest.java |
Registers the new assignment test program. |
org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/test/stepping/StatementSteppingTests.java |
Adds assignment regression coverage and enables step filters explicitly. |
org.eclipse.jdt.debug.tests/testprograms/StatementStepAssignment.java |
Provides constant assignments for the stepping regression test. |
Review effort: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With statement-level stepping enabled, a line was skipped as "intermediate" if it started with a load/constant push instruction and the next line location was 1-3 bytes away. That is also true for simple statements like "result = false;" (iconst_0; istore_1), so the debugger never stopped on such assignments.
Commit 8904bd1 only handled the case where the instruction after the push is a return. Now a line is only skipped if every instruction up to the next line location is a load/constant push. Any other instruction (store, return, branch, invoke, ...) means the line is a real statement and must not be skipped.
Fixes #1053
Assisted-by: Copilot with Claude Opus 5