Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8263342: Add --connect option to jhsdb hsdb/clhsdb #2908

Closed
wants to merge 2 commits into from

Conversation

@YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented Mar 10, 2021

Most of subcommands in jhsdb supports to connect to debug server via --connect command line option, however hsdb and clhsdb do not accept it.

Both HSDB and CLHSDB support to connect to debug server, so they should have capability to do it via command line option.

I also filed CSR for this issue. Please review it as well.


Progress

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

Issue

  • JDK-8263342: Add --connect option to jhsdb hsdb/clhsdb

Reviewers

Download

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

To update a local copy of the PR:
$ git checkout pull/2908
$ git pull https://git.openjdk.java.net/jdk pull/2908/head

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 10, 2021

👋 Welcome back ysuenaga! 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.

Loading

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 10, 2021

/csr needed

Loading

@openjdk openjdk bot added the csr label Mar 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 10, 2021

@YaSuenag this pull request will not be integrated until the CSR request JDK-8263345 for issue JDK-8263342 has been approved.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 10, 2021

@YaSuenag 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.

Loading

@YaSuenag YaSuenag marked this pull request as ready for review Mar 10, 2021
@openjdk openjdk bot added the rfr label Mar 10, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 10, 2021

Webrevs

Loading

// Attempt to connect to remote debug server
debugServerName = args[0];
Copy link
Contributor

@plummercj plummercj Mar 10, 2021

Choose a reason for hiding this comment

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

I assume this change to no longer support just specifying an execPath is not exposed to the user, unless they are invoking the CLHSDB class directly from the command line.

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 11, 2021

Choose a reason for hiding this comment

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

Yes, but this code would be run from SALauncher::runCLHSDB, so we need to change.
https://github.com/YaSuenag/jdk/blob/JDK-8263342/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/SALauncher.java#L282

Loading

Copy link
Contributor

@plummercj plummercj Mar 11, 2021

Choose a reason for hiding this comment

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

Yes, I understood that part. I just wanted to make sure, for example, that we weren't previously supporting something like attach <exe_path> in clhsdb, and that would be impacted by this, but I'm pretty sure clhsdb already prevents doing that. However, what if you used jshdb <cmd> --exe and don't specify a core file. Did that ever work, and if so will it still work?

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 11, 2021

Choose a reason for hiding this comment

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

Yes, I understood that part. I just wanted to make sure, for example, that we weren't previously supporting something like attach <exe_path> in clhsdb, and that would be impacted by this, but I'm pretty sure clhsdb already prevents doing that.

It would check at SALauncher::buildAttachArgs

} else if (exe != null) {
if (connect != NO_REMOTE) {
throw new SAGetoptException("Unnecessary argument: --connect");
} else if (exe.length() == 0) {
throw new SAGetoptException("You have to set --exe.");
}
newArgs.add(exe);
if ((core == null) || (core.length() == 0)) {
throw new SAGetoptException("You have to set --core.");
}

However, what if you used jshdb --exe and don't specify a core file. Did that ever work, and if so will it still work?

Due to above code snippets, we can see the usage on the console in JDK 15 as following, and it is same behavior after this change.

$ jhsdb clhsdb --exe /java
You have to set --core.
    --pid <pid>             To attach to and operate on the given live process.
    --core <corefile>       To operate on the given core file.
    --exe <executable for corefile>

    The --core and --exe options must be set together to give the core
    file, and associated executable, to operate on. They can use
    absolute or relative paths.
    The --pid option can be set to operate on a live process.
    --core and --pid are mutually exclusive.

    Examples: jhsdb clhsdb --pid 1234
          or  jhsdb clhsdb --core ./core.1234 --exe ./myexe

Loading

* @run main/othervm ClhsdbAttachToDebugServerWithCommandLine
*/

public class ClhsdbAttachToDebugServerWithCommandLine {
Copy link
Contributor

@plummercj plummercj Mar 10, 2021

Choose a reason for hiding this comment

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

The test name is rather long. Since this test is already in the sadebugd directory, you could shorten it to ClhsdbAttachWithCommandLine, although I'm not so sure I like it either. How about ClhsdbAttachWithConnect or ClhsdbAttachWithConnectArgument, or ClhsdbTestConnectArgument (I think I like the last one best)?

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 11, 2021

Choose a reason for hiding this comment

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

I renamed to ClhsdbTestConnectArgument in new commit.

Loading

@openjdk openjdk bot removed the csr label Mar 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 12, 2021

@YaSuenag 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:

8263342: Add --connect option to jhsdb hsdb/clhsdb

Reviewed-by: cjplummer, sspitsyn

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 190 new commits pushed to the master branch:

  • b23228d: 8263914: CDS fails to find the default shared archive on x86_32
  • a5e7a89: 8263904: compiler/intrinsics/bmi/verifycode/BzhiTestI2L.java fails on x86_32
  • f62b100: 8263895: Test nsk/jvmti/GetThreadGroupChildren/getthrdgrpchld001/getthrdgrpchld001.cpp uses incorrect indices
  • f84b52b: 8263897: compiler/c2/aarch64/TestVolatilesSerial.java failed with "java.lang.RuntimeException: Wrong method"
  • f08bf4b: 8263891: Changes for 8076985 missed the fix.
  • b2df513: 8261785: Calling "main" method in anonymous nested class crashes the JVM
  • 840ab7b: 8263894: Convert defaultPrinter and printers fields to local variables
  • ba504fc: 8187450: JNI local refs exceeds capacity warning in NetworkInterface::getAll
  • 0abbfb2: 8263729: [test] divert spurious output away from stream under test in ProcessBuilder Basic test
  • 6c2220e: 8263861: Shenandoah: Remove unused member in ShenandoahGCStateResetter
  • ... and 180 more: https://git.openjdk.java.net/jdk/compare/d0c1aec2023cbaba48548cc22094c417c051595f...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Mar 12, 2021
@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 19, 2021

PiNG: could you review this? I need one more reviewer. CSR has been approved.

Loading

Copy link
Contributor

@sspitsyn sspitsyn left a comment

Hi Yasumasa,
This looks good.
Thanks,
Serguei

Loading

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 23, 2021

/integrate

Loading

@openjdk openjdk bot closed this Mar 23, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 23, 2021
@YaSuenag YaSuenag deleted the JDK-8263342 branch Mar 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 23, 2021

@YaSuenag Since your change was applied there have been 191 commits pushed to the master branch:

  • 6b4c654: 8263776: [JVMCI] add helper to perform Java upcalls
  • b23228d: 8263914: CDS fails to find the default shared archive on x86_32
  • a5e7a89: 8263904: compiler/intrinsics/bmi/verifycode/BzhiTestI2L.java fails on x86_32
  • f62b100: 8263895: Test nsk/jvmti/GetThreadGroupChildren/getthrdgrpchld001/getthrdgrpchld001.cpp uses incorrect indices
  • f84b52b: 8263897: compiler/c2/aarch64/TestVolatilesSerial.java failed with "java.lang.RuntimeException: Wrong method"
  • f08bf4b: 8263891: Changes for 8076985 missed the fix.
  • b2df513: 8261785: Calling "main" method in anonymous nested class crashes the JVM
  • 840ab7b: 8263894: Convert defaultPrinter and printers fields to local variables
  • ba504fc: 8187450: JNI local refs exceeds capacity warning in NetworkInterface::getAll
  • 0abbfb2: 8263729: [test] divert spurious output away from stream under test in ProcessBuilder Basic test
  • ... and 181 more: https://git.openjdk.java.net/jdk/compare/d0c1aec2023cbaba48548cc22094c417c051595f...master

Your commit was automatically rebased without conflicts.

Pushed as commit b2a52ea.

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

Loading

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