8271356: Modify jdb to treat an empty command as a repeat of the previous command

[jakobcornell]

This has been under discussion on and off for the past month or so on serviceability-dev, and I think a CSR request is required, so this may be a work in progress.

Notes on the patch:

• The list command previously marked a line in each listing with =>. In a bare list this is the next line up for execution. Previously when requesting a specific location (e.g. list 5) the requested line would be marked. With the patch applied, list will only ever mark the next line up for execution. This is consistent with the behavior of GDB and PDB (at least).
• EOF is printed when the repeat setting is on and a bare list command follows a listing containing the last source line. This feature is from PDB; it's a somewhat softer message than the one for an explicit list request that's out of range.
• I don't speak Chinese or Japanese, so I've omitted localizations for the new messages in those locales. However, I updated the help text in both to include the new commands, with the descriptions left empty for now.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8271356: Modify jdb to treat an empty command as a repeat of the previous command

Reviewers

• Chris Plummer (@plummercj - Reviewer)
• Ioi Lam (@iklam - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5290/head:pull/5290
$ git checkout pull/5290

Update a local copy of the PR:
$ git checkout pull/5290
$ git pull https://git.openjdk.java.net/jdk pull/5290/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5290

View PR using the GUI difftool:
$ git pr show -t 5290

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5290.diff

[bridgekeeper]

👋 Welcome back jakobcornell! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@jakobcornell The following label will be automatically applied to this pull request:

• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 07: Full - Incremental (f5539997)
• 06: Full - Incremental (da4d3a18)
• 05: Full - Incremental (5f896476)
• 04: Full - Incremental (7c7764a5)
• 03: Full - Incremental (77a255d1)
• 02: Full - Incremental (a44b8b4b)
• 01: Full - Incremental (6328744f)
• 00: Full (eb45155e)

[plummercj]

I'm not sure how to advise about the Chinese or Japanese localization. I don't know what the process is for getting the text updated and properly tested. Hopefully someone else has that knowledge and will comment.

What testing have you done? There are jdb tests in test/hotspot/jtreg/vmTestbase/nsk/jdb. You'll also need to write some tests for the new functionality.

[jakobcornell]

I think the main reason the diff was so unhelpful is that part of my original change was converting the <n> <command> handling from recursive to iterative. I think the code as a whole is more intuitive when this is done iteratively, and along with the iterative approach came a fix (for synchronous commands) for a minor bug in which the prompt is printed along with each iteration of an <n> <command> command.

However, reverting to the recursive approach and restoring some original variable names has made the diff much easier to understand. I do have the iterative version saved on a local branch in case it becomes useful.

I've run the tests you indicated (make run-test-vmTestbase_nsk_jdb) and those are all passing. I'll take a look into writing some new tests for the changes.

[plummercj]

I think overall it looks good. Still need tests and still need to resolve the localization issues.

[jakobcornell]

I've added fairly thorough tests for the new stuff in this branch, including command repetition, list auto-advance, and the tentative new behavior of the => indicator in listings.

Chris, do you think I should give the Chinese and Japanese localizations a shot? I do actually know a little Chinese, but the Japanese would basically just be whatever Google Translate says. Or maybe during the CSR review somebody will be able to help? Not really sure how to proceed.

[plummercj]

Chris, do you think I should give the Chinese and Japanese localizations a shot? I do actually know a little Chinese, but the Japanese would basically just be whatever Google Translate says. Or maybe during the CSR review somebody will be able to help? Not really sure how to proceed.

I would hold off on this. I'm trying to find out the proper process for getting this done. It shouldn't be left to individuals trying to push new changes.

[plummercj]

Chris, do you think I should give the Chinese and Japanese localizations a shot? I do actually know a little Chinese, but the Japanese would basically just be whatever Google Translate says. Or maybe during the CSR review somebody will be able to help? Not really sure how to proceed.

I would hold off on this. I'm trying to find out the proper process for getting this done. It shouldn't be left to individuals trying to push new changes.

Looks like nothing needs to be done for localization. Theses files are in an internal list to automatically get translated at some point in the future.

[jakobcornell]

Thanks for following up on that Chris. Are there any outstanding changes that need to be made to the PR then, or are we waiting for the CSR process?

[plummercj]

Thanks for following up on that Chris. Are there any outstanding changes that need to be made to the PR then, or are we waiting for the CSR process?

I think we can do the CSR now. I got it started with https://bugs.openjdk.java.net/browse/JDK-8274167. If you could fill out the Description section and send it to me (posting here is fine), then I'll paste it in the CR and then ask for someone on our team to review it (I probably shouldn't be the reviewer).

[jakobcornell]

Great, thanks Chris. Before I write that up I think we need to clarify something with regard to compatibility. In the CSR ticket you mention that behavior is only changed with repeat turned on, but I made a change to how the list marker (=>) is used which also affects runs with repeat off [1]. There's some more info in the first bullet point in my initial comment for the PR.

So we're going to have to decide whether to include this change. I think it would be confusing for this behavior to be switchable by the repeat command, so I propose we either document it in the CSR as an incompatibility or revert the change in the PR. The change is in line with making JDB more consistent with other debuggers, but its appeal is somewhat subjective. Perhaps you can advise as to which option would be better?

[1] https://github.com/openjdk/jdk/pull/5290/files#diff-26c2ab70eb013703a8dbf9806cf5677966d91a1fde4b5751c9824d00ed393656R1570

[plummercj]

Ok. I think the list marker change is fine. I don't expect compatibility issues due to it, but yes we do need to all that out in the CSR.

[jakobcornell]

Okay great, that's what I was thinking as well. Here's a draft for the description section; while writing this up I realized I forgot to implement one aspect of the list auto-advance, so I pushed a second commit as well.

Summary

Add automatic command repetition and list auto-advance to JDB and make list command output more intuitive.

Problem

The overall problem is that JDB is not as ergonomic as other command line debuggers like GDB and PDB (the Python debugger). In particular:

• In order to quickly step through a chunk of code in JDB, the user has to either step one line at a time, typing out the next command each time, or use a more intricate approach such as a temporary breakpoint or an <n> next command after counting lines. In other debuggers this is accomplished by issuing a next command and then simply repeating it by pressing Enter as necessary.
• In order to examine code more than 10 lines at a time, a JDB user has to invoke list repeatedly with a manually computed line number each time, and if the user doesn't bother to determine the precise line number at the center of the next chunk the listing duplicates or skips lines. In other debuggers, a large chunk of the source can be easily listed by repeatedly invoking list (or just pressing Enter) as needed.

Solution

A new command is added to make the new functionality available on an opt-in basis. The user may enter repeat on to enable or repeat off to disable the features described below. repeat may be used to query the current setting (off by default).

• After issuing one of a certain set of "repeatable" commands [1], entering a blank line at the prompt will now cause the command to be re-executed, just as !! does but without first displaying the command. This only occurs if repeat is enabled. Previously, a blank line was always a no-op.
• In some situations, list command output now starts where the previous list output left off. The situations are (1) an arbitrary list command is followed by a list command with no target (the commands need not be consecutive; only certain commands [2] reset the listing state), and (2) an arbitrary list command is repeated by entering a blank line at the prompt. This behavior only occurs if repeat is enabled, but the listing state survives changes to the repeat setting and is updated even when repeat is disabled. After reaching the end of the file, only "EOF" is printed until the state is reset.
• Previously, the marker => was used in the output of list to indicate the listing "target"; the one specified in the command (usually a line number), or otherwise the current line of execution. Only the current line of execution is now so marked, regardless of whether a target is specified. This occurs irrespective of the repeat setting.

An alternative way to address the usability concerns is to integrate Jline into JDB, for an experience similar to that of Jshell. While this is conceptually simpler, there was concern that the extent of the code changes necessary for this integration would be too large compared to the benefit [3].

[1] The repeatable commands are: up, down, step, stepi, next, cont, list, pop, and reenter. A repeatable command preceded by a repeat number (e.g. 5 next) is also repeatable.
[2] The listing reset commands are: run, suspend, resume, up, down, kill, interrupt, threadgroup, step, stepi, next, cont, pop, and reenter. A listing reset command preceded by a repeat number is also a listing reset command.
[3] https://mail.openjdk.java.net/pipermail/serviceability-dev/2021-August/038889.html

Specification

To my knowledge the behavior of JDB is only specified to the extent of its help and usage messages, which are updated as appropriate in the accompanying pull request.

[plummercj]

I updated the CSR. One section I'd like to see improved is the readability of the Problem section. The middle sentence is long winded and hard to read. Perhaps a bullet list would be better. Also I think the repeatable commands and listing reset commands should be explicitly listed instead of referencing a diff. I made this change already. Also, no need to reference the PR from the CSR.

[mlbridge]

Mailing list message from Jakob Cornell on serviceability-dev:

One section I'd like to see improved is the readability of the Problem section. The middle sentence is long winded and hard to read. Perhaps a bullet list would be better.

Right you are. I've revised it to use bullet points, and while I've added a bit more detail to each they should be easier to follow. If not, let me know and I can do some condensing.

Jakob

[jakobcornell]

Hey Chris, is there any update on this? I've revised the Problem section so I think we're waiting on the CSR ticket to be updated accordingly and for someone to be assigned for the CSR review.

[plummercj]

/csr needed

[plummercj]

Hey Chris, is there any update on this? I've revised the Problem section so I think we're waiting on the CSR ticket to be updated accordingly and for someone to be assigned for the CSR review.

Ok. I updated the CSR with your changes. I'll try to find someone to review it.

[openjdk]

@plummercj this pull request will not be integrated until the CSR request JDK-8274167 for issue JDK-8271356 has been approved.

[jakobcornell]

@plummercj Looks like Serguei Spitsyn approved this about a week ago, so I wanted to make sure this is moving along. Is the CSR proposal now approved or is there a need for another reviewer?

[plummercj]

@plummercj Looks like Serguei Spitsyn approved this about a week ago, so I wanted to make sure this is moving along. Is the CSR proposal now approved or is there a need for another reviewer?

It looks like it needed for me to click on "finalize" so I've done that. Now it is in the "Finalized" state and is I believe waiting final approval by the CSR committee, which I think will result in closing the CSR as "Approved", and you can then complete the review of the PR and push the changes.

[iklam]

Hi Jacob, this is not your fault, but the "zz help text" in the Chinese and Japanese versions are in a single huge line. This makes it impossible to see what you have changed in the GitHub diffs, and impossible to tell whether you made any typos in the process.

I would recommend making a prerequisite PR first:

• Break the huge lines in those two files in the same way as TTYResources.java. Verify that TTYResources_ja.class, etc, are identical to the previous version. Integrate into openjdk.
• Revert the changes of those two files in the current PR
• Merge with the prerequisite PR
• Add the new lines into those two files

Thanks

• Ioi

[openjdk]

@jakobcornell This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8271356: Modify jdb to treat an empty command as a repeat of the previous command

Reviewed-by: cjplummer, iklam

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1051 new commits pushed to the master branch:

• 7addcd7: 8276034: ProblemList gtest dll_address_to_function_and_library_name on macosx-x64
• 2448b3f: 8275874: [JVMCI] only support aligned reads in c2v_readFieldValue
• f1f5e26: 8275872: Sync J2DBench run and analyze Makefile targets with build.xml
• 19f76c2: 8275079: Remove unnecessary conversion to String in java.net.http
• e5cd269: 8274944: AppCDS dump causes SEGV in VM thread while adjusting lambda proxy class info
• 82f4aac: 8259609: C2: optimize long range checks in long counted loops
• 574f890: 8275720: CommonComponentAccessibility.createWithParent isWrapped causes mem leak
• 7c88a59: 8275809: crash in [CommonComponentAccessibility getCAccessible:withEnv:]
• c9dec2f: 8273299: Unnecessary Vector usage in java.security.jgss
• b98ed55: 8275819: [TableRowAccessibility accessibilityChildren] method is ineffective
• ... and 1041 more: https://git.openjdk.java.net/jdk/compare/90cd2fa16458dcc3e36171fa4bf21f26bc92b168...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@plummercj, @iklam) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[jakobcornell]

Thanks Chris. @iklam, would you be able to review these changes again?

[iklam]

LGTM

[jakobcornell]

/summary

[openjdk]

@jakobcornell To set a summary, use the syntax /summary <summary text>

[jakobcornell]

/integrate

[openjdk]

@jakobcornell
Your change (at version f553999) is now ready to be sponsored by a Committer.

[jakobcornell]

@plummercj Looks like this is all ready to go then. Would you be willing to sponsor it for integration?

[plummercj]

@plummercj Looks like this is all ready to go then. Would you be willing to sponsor it for integration?

Yes. I'm doing some testing with our CI now. I'll integrate once that is done.

[plummercj]

/sponsor

[openjdk]

Going to push as commit fe6a202.
Since your change was applied there have been 1100 commits pushed to the master branch:

• cef9db9: 8276039: Remove unnecessary qualifications of java_lang_Class::
• 13265f9: 8274750: java/io/File/GetXSpace.java failed: '/dev': 191488 != 190976
• 5facaa2: 8276128: (bf) Remove unused constant ARRAY_BASE_OFFSET from Direct-X-Buffer
• d6d82f5: 8275608: runtime/Metaspace/elastic/TestMetaspaceAllocationMT2 too slow
• a1ec4f9: 6854300: [TEST_BUG] java/awt/event/MouseEvent/SpuriousExitEnter/SpuriousExitEnter_3.java fails in jdk6u14 & jdk7
• 8cc5950: 8251468: X509Certificate.get{Subject,Issuer}AlternativeNames and getExtendedKeyUsage do not throw CertificateParsingException if extension is unparseable
• 4c3491b: 8017175: [TESTBUG] javax/swing/JPopupMenu/4634626/bug4634626.java sometimes failed on mac
• c0cda1d: 8273026: Slow LoginContext.login() on multi threading application
• 15fd8a3: 8276102: JDK-8245095 integration reverted JDK-8247980
• e89b2c0: 8276086: Increase size of metaspace mappings
• ... and 1090 more: https://git.openjdk.java.net/jdk/compare/90cd2fa16458dcc3e36171fa4bf21f26bc92b168...master

Your commit was automatically rebased without conflicts.

[openjdk]

@plummercj @jakobcornell Pushed as commit fe6a202.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[jakobcornell]

Thanks again, Chris and Ioi!

[plummercj]

@jakobcornell It looks like we have a couple of intermittent test failures that need investigating. Can you please have a look at JDK-8276208? Thanks.

[jakobcornell]

Just verified that this test still passes for me locally on LInux. I see from Daniel's test logs where the behavior is differing on the Windows build, and I'll see if I can reproduce it on a Windows system. I'll also look into fixing the issue of the AssertionErrors being ignored. But I wonder if it would be better to throw RuntimeException instead of calling failure. If an assertion fails, the state of the debugger may be incompatible with the state of the test code, and it seems most natural to me that it be aborted at that point.

Should I open a new pull request linked to 8276208 for these changes?

[plummercj]

I would suggest either Failure or TestFailure instead of RuntimeException (and they both inherit from RuntimeException). This is what other tests do instead of throwing RuntimeException. From an implementation standpoint the two are identical. However, what I've noticed is that a lot of tests catch Failure, but not TestFailure, and it almost always looks something like:

        } catch (Failure e) {
            log.complain("TEST FAILED: " + e.getMessage());
            success = false;
...
        if (!success) {
            log.complain("TEST FAILED");
            return FAILED;
        }

But those are all in jdi and jdwp tests. Jdb tests seem to almost alway use the failure() method, which is the same as the first part of the above code, and then later JdbTest.runtTest() does the 2nd part, so I think using failure() is more consistent. Yes, this does mean the test will continue to run. This seems to be the way most of these tests work. A failure does not indicate that the test should be aborted, and it will continue doing more test cases. Sometimes this is appropriate. I'm sure plenty of times it's not, but I think erring on the side of continuing to run the test is generally more useful.

[plummercj]

The PR for 8276208 should contain the fix for the failures. If you want to also clean up the AssertionError issue at the same time, that's ok, or I can file a separate CR for that part if you wish.

[jakobcornell]

Okay, I'll change those tests to use failure, and I'm okay doing that in the same PR. Unfortunately I'm no longer able to build the JDK at master; I seem to be getting the same error that this person did in August. Since the test classes depend on the JDK itself I'm not able to verify that the changes to the tests are correct (e.g. compile). I suppose if others aren't seeing the same build error I could create that PR anyway and let someone else test the change.

Also, Daniel's failing Windows build seems to be caused by the command 2 2 up only executing up once rather than 4 times, and I wasn't able to reproduce that JDB behavior on master on the Windows system I have access to.

On both of these items, I'm not sure how to proceed.

[jakobcornell]

The JDK build issue is resolved (thanks David Holmes). The switch to failure is now implemented in #6182, and I think we can move the discussion there.

[plummercj]

The JDK build issue is resolved (thanks David Holmes). The switch to failure is now implemented in #6182, and I think we can move the discussion there.

#6182 is still a draft. Was that intentional? No emails will go out while it is a draft.

[jakobcornell]

Yes, I made it a draft because it's not ready for review yet. I'll undo that though to allow the email system to work. Thanks for the heads up.

[mlbridge]

Mailing list message from David Holmes on serviceability-dev:

On 1/11/2021 1:19 pm, Jakob Cornell wrote:

On Mon, 25 Oct 2021 22:21:54 GMT, Jakob Cornell <duke at openjdk.java.net> wrote:

This has been under discussion on and off for the past month or so on serviceability-dev, and I think a CSR request is required, so this may be a work in progress.

Notes on the patch:

- The `list` command previously marked a line in each listing with `=>`. In a bare `list` this is the next line up for execution. Previously when requesting a specific location (e.g. `list 5`) the requested line would be marked. With the patch applied, `list` will only ever mark the next line up for execution. This is consistent with the behavior of GDB and PDB (at least).
- `EOF` is printed when the repeat setting is on and a bare `list` command follows a listing containing the last source line. This feature is from PDB; it's a somewhat softer message than the one for an explicit `list` request that's out of range.
- I don't speak Chinese or Japanese, so I've omitted localizations for the new messages in those locales. However, I updated the help text in both to include the new commands, with the descriptions left empty for now.

Jakob Cornell has updated the pull request incrementally with one additional commit since the last revision:

Restore update of copyright messages in resource files

Okay, I'll change those tests to use `failure`, and I'm okay doing that in the same PR. Unfortunately I'm no longer able to build the JDK at `master`; I seem to be getting the same error that [this person did](https://mail.openjdk.java.net/pipermail/build-dev/2021-August/031907.html) in August. Since the test classes depend on the JDK itself I'm not able to verify that the changes to the tests are correct (e.g. compile). I suppose if others aren't seeing the same build error I could create that PR anyway and let someone else test the change.

That issue was a problem with the boot JDK being too old. make sure you
are using the GA version of JDK 17.

Cheers,
David
-----