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

8262520: Add SA Command Line Debugger support to connect to debug server #2773

Closed
wants to merge 7 commits into from

Conversation

@YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented Mar 1, 2021

attach command on CLHSDB supports to attach live process (PID) and coredump, but it cannot connect to debug server. CLHSDB does not have a command to connect to debug server.

Other jhsdb commands (jstack, jmap, etc...) can connect debug server via --connect option, so CLHSDB should connect to it.

After this change, you can connect to debug server with 'connect '.


Progress

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

Issue

  • JDK-8262520: Add SA Command Line Debugger support to connect to debug server

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2773/head:pull/2773
$ git checkout pull/2773

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 1, 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

@openjdk
Copy link

@openjdk openjdk bot commented Mar 1, 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 1, 2021
@openjdk openjdk bot added the rfr label Mar 1, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 1, 2021

Loading

@plummercj
Copy link
Contributor

@plummercj plummercj commented Mar 1, 2021

You'll need a CSR for this.

Will jhsdb clhsdb --connect debugserver work? If not, it should to be consistent with the other commands.

Does detach undo the connect? It's not very symetric if we do since we already had attach + detach and now you added connect + detach. Since you can attach to a pid or a core file, maybe instead of adding connect you should just put the debugd support in attach.

Loading

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 1, 2021

You'll need a CSR for this.

Ok, I will add it after this discussion ( coonect or attach )

Will jhsdb clhsdb --connect debugserver work? If not, it should to be consistent with the other commands.

It does not work. CLHSDB and HSDB do not accept '--connect'. I plan to add it after this PR.

Does detach undo the connect?

Yes.

It's not very symetric if we do since we already had attach + detach and now you added connect + detach. Since you can attach to a pid or a core file, maybe instead of adding connect you should just put the debugd support in attach.

I thought connect command is easy to understand because it is similar with --connect option in jhsdb, and also the change is smaller than adding to attach because attach already has 1 string argument (PID).
I'm not familiar of network, but I concern hostname which is configured by numeric chars only - I'm not sure it is allowed, but RFC 1123 allows digit character at the first.

      The syntax of a legal Internet host name was specified in RFC-952
      [DNS:4].  One aspect of host name syntax is hereby changed: the
      restriction on the first character is relaxed to allow either a
      letter or a digit.  Host software MUST support this more liberal
      syntax.

If digit hostname (e.g. 1234) is allowed, we cannot add debugd support to attach because we cannot distinguish it is PID or hostname. If so, it is reasonable to add connect command for this purpose.

Loading

@plummercj
Copy link
Contributor

@plummercj plummercj commented Mar 2, 2021

jhsdb has --pid, --core, and --connect as the 3 ways to "attach" for lack of a better word. It would have been nice if the clhsdb command did a better job of copying these three command line arguments. You are suggesting that adding a connect command would have similarity to the --connect option, but unfortunately the attach command has no such similarity with the command line arguments --pid and --core, so I'm not so sure doing so with connect is actually helping in that regard, especially when "connect" and "attach" pretty much mean the same thing.

As for numeric host names, yes, that is possible, but then you could also have a numeric core file name. If we really want to avoid the numeric host name problem, perhaps something like attachd would be better than connect, but it seems any choice we make will have it's drawbacks due the the baggage of existing commands and options.

Loading

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 2, 2021

@plummercj Ok, I try to add debug server support to attach clhsdb command. Then should we still need CSR?

Loading

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 2, 2021

@plummercj Ok, I try to add debug server support to attach clhsdb command. Then should we still need CSR?

Pushed new commit to be implemented as attach. It works fine with serviceability/sa jtreg tests on my Linux x64.
I will add CSR if you are ok.

Loading

@kevinjwalls
Copy link

@kevinjwalls kevinjwalls commented Mar 2, 2021

Hi,
I ran the first version and agree it works, the connect command worked OK when I ran a local, manual test. But I do like adding to the attach Command rather than connect, avoiding introducing a new Command name.

Assuming an int is a PID like in Tool.java is reasonable.
You can have a leading numeral since a certain RFC, but although I can't find clear language to say it's definitely illegal, actualy using a fully numeric hostname seems unwise.

CLHSDB.java line 185
Now that "private void attachDebugger(int pid)" takes an int, we don't want to catch NumberFormatException, that try/catch block is not wanted.

In the testcase, I think it will fail if a command throws an Exception, but that is limiting what problems it can detect:
could we make the test check at least some of the key output to show that we are attached and getting information, not errors or something unexpected? If at least some key commands we issue are checked that they contain some output indicating success that would be great.

Loading

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 2, 2021

Thanks @kevinjwalls !

CLHSDB.java line 185
Now that "private void attachDebugger(int pid)" takes an int, we don't want to catch NumberFormatException, that try/catch block is not wanted.

I fixed it in new commit, and also I fixed HSDB.java.

In the testcase, I think it will fail if a command throws an Exception, but that is limiting what problems it can detect:
could we make the test check at least some of the key output to show that we are attached and getting information, not errors or something unexpected? If at least some key commands we issue are checked that they contain some output indicating success that would be great.

I added calling class java.lang.Object and class java.lang.String in testcase. We can check whether the connection works fine through them.

Loading

Copy link
Contributor

@plummercj plummercj left a comment

This pre-existing code in CLHSDB.java is a little concerning:

127     private void doUsage() {
128         System.out.println("Usage:  java CLHSDB [[pid] | [path-to-java-executable [path-to-corefile]] | help ]");
129         System.out.println("           pid:                     attach to the process whose id is 'pid'");
130         System.out.println("           path-to-java-executable: Debug a core file produced by this program");
131         System.out.println("           path-to-corefile:        Debug this corefile.  The default is 'core'");
132         System.out.println("        If no arguments are specified, you can select what to do from the GUI.\n");
133         HotSpotAgent.showUsage();
134     }

First concern is that you have not added "debug server" support to it. 2nd concern is the "If no arguments are specified, you can select what to do from the GUI" part. It looks like using the GUI you can launch the CLHSDB console, although I can't find in the source where/how that is actually done. In any case, i don't see any way to pass command arguments when you do that.

I'm just wondering how much of this CLHSDB support we want to keep beyond what the clhsdb command needs. Do we still want to support invoking CLHSDB.main() from the java command line?

Loading

@@ -36,7 +36,7 @@ <h3>Annotated output of CLHSDB help command</h3>
<code>
Available commands:
assert true | false <font color="red">turn on/off asserts in SA code</font>
attach pid | exec core <font color="red">attach SA to a process or core</font>
attach pid | exec core | remote_server <font color="red">attach SA to a process, core, or remote debug server</font>
Copy link
Contributor

@plummercj plummercj Mar 2, 2021

Choose a reason for hiding this comment

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

The jhsdb help says jhsdb jstack --connect debugserver, so maybe debug_server would be better than remote_server. I think "remote debug server" in the help text is fine.

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 3, 2021

Choose a reason for hiding this comment

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

Fixed it in new commit.

Loading

// and coreFilename is the pathname of a core file we are
// supposed to attach to.
// Finally, if remoteMachineName != null, we are supposed to connect to remote debug server.
Copy link
Contributor

@plummercj plummercj Mar 2, 2021

Choose a reason for hiding this comment

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

Split this line so its length is more consistent with the previous lines.

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 3, 2021

Choose a reason for hiding this comment

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

Fixed it in new commit.

Loading

private int pid;
private String execPath;
private String coreFilename;
private String remoteMachineName;
Copy link
Contributor

@plummercj plummercj Mar 2, 2021

Choose a reason for hiding this comment

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

I think debugServerName or remoteDebugServerName would be better.

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 3, 2021

Choose a reason for hiding this comment

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

Rename to debugServerName in new commit.

Loading

System.out.println("Connecting to debug server, please wait...");
agent.attach(remoteMachineName);
this.remoteMachineName = remoteMachineName;
Copy link
Contributor

@plummercj plummercj Mar 2, 2021

Choose a reason for hiding this comment

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

I'm a little surprised to see that there is already some support for connecting to a debug server in CLHSDB.java. Was it previously used from anywhere?

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 3, 2021

Choose a reason for hiding this comment

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

I cannot find out caller of CLHSDB::connect. I guess CLHSDB.java is copied from HSDB.java because they are similar - HSDB uses connect() to connect to debug server.

Loading

// and coreFilename is the pathname of a core file we are
// supposed to attach to.
// Finally, if remoteMachineName != null, we are supposed to connect to remote debug server.
Copy link
Contributor

@plummercj plummercj Mar 2, 2021

Choose a reason for hiding this comment

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

Split into 2 lines.

Loading

private int pid;
private String execPath;
private String coreFilename;
private String remoteMachineName;
Copy link
Contributor

@plummercj plummercj Mar 2, 2021

Choose a reason for hiding this comment

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

debugServerName or remoteDebugServerName

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 3, 2021

Choose a reason for hiding this comment

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

Rename to debugServerName in new commit.

Loading

pid = -1;
execPath = null;
coreFilename = null;
remoteMachineName = null;
Copy link
Contributor

@plummercj plummercj Mar 2, 2021

Choose a reason for hiding this comment

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

In general it's not clear to me why HSDB.java needed to be modified in a way similar to CLHSDB.java. If the goal is really just to add debug server support to the clshdb attach command, why is HSDB involved in that? Or is this adding debug server support elsewhere also (in which case it's not clear to me where this exposed to the user)?

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 3, 2021

Choose a reason for hiding this comment

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

As I said in above, CLHSDB.java and HSDB.java are similar, and also I added attach(String debugServerName) to CommandProcessor$DebuggerInterface which is used in HSDB.java, so I changed HSDB.java even though it does not affect to HSDB.

We can implement attach(String debugServerName) as empty method, and it might be reasonable. What do you think?

Loading

Copy link
Contributor

@plummercj plummercj Mar 3, 2021

Choose a reason for hiding this comment

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

I see now. Both HSDB and CLHSDB implement CommandProcessor$DebuggerInterface, and they pass an instance of this class to the CommandProcessor constructor, and that allows CommandProcessor to do callbacks for the attach (or connect). Given this, it looks like you do want to add support for all 3 attach modes to the HSDB CommandProcessor$DebuggerInterface implementation. Otherwise you won't be able to attach to a debug server when you bring up the command line support using the HSDB gui.

What's been confusing, and I'm finally starting to understand, is that CommandProcessor is the SA command line support, and clhsdb (CLHSDB) and hsdb (HSDB) both use CommandProcessor to provide the command line support to the user. Maybe you should re-purpose this CR/PR as "Add SA CommandProcessor support to connect to debug server", and make sure the CR talks about how this impacts both clhsdb and hsdb.

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 4, 2021

Choose a reason for hiding this comment

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

I've updated subject both CR and PR, and also I updated description in JBS.

Loading

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 3, 2021

This pre-existing code in CLHSDB.java is a little concerning:

127     private void doUsage() {
128         System.out.println("Usage:  java CLHSDB [[pid] | [path-to-java-executable [path-to-corefile]] | help ]");
129         System.out.println("           pid:                     attach to the process whose id is 'pid'");
130         System.out.println("           path-to-java-executable: Debug a core file produced by this program");
131         System.out.println("           path-to-corefile:        Debug this corefile.  The default is 'core'");
132         System.out.println("        If no arguments are specified, you can select what to do from the GUI.\n");
133         HotSpotAgent.showUsage();
134     }

First concern is that you have not added "debug server" support to it. 2nd concern is the "If no arguments are specified, you can select what to do from the GUI" part. It looks like using the GUI you can launch the CLHSDB console, although I can't find in the source where/how that is actually done. In any case, i don't see any way to pass command arguments when you do that.

I just added the feature to connect to debug server, so I did not change CLHSDB(String[] args) and help message.
I think it should be changed when we add --connect option to jhsdb clhsdb. (I will work for it after this PR)

I'm just wondering how much of this CLHSDB support we want to keep beyond what the clhsdb command needs. Do we still want to support invoking CLHSDB.main() from the java command line?

I think we should remove main() from CLHSDB/HSDB to be honest because they are called from SALauncher, and we cannot call them directly because they are not exported in module-info.

Loading

@plummercj
Copy link
Contributor

@plummercj plummercj commented Mar 3, 2021

I think we should remove main() from CLHSDB/HSDB to be honest because they are called from SALauncher, and we cannot call them directly because they are not exported in module-info.

Yes, I'm starting to come to the same conclusion. The only code that constructs a CLHSDB object, and therefore triggers the potential call to doUsage(), is CLHSDB.main(), and it is only called from SALauncher when using jhsdb clhsdb. We could get rid of main() and have some other entrypoint that is passed the values of all the possible options, rather than having to parse them in CLHSDB.java.

There is one test invokes java with the CLHSDB class. See SABase.java. It's part of ciReplay testing. I think it would be easily modified to just run jhsdb clhsdb

Loading

attached = true;
}
catch (DebuggerException e) {
final String errMsg = formatMessage(e.getMessage(), 80);
System.err.println("Unable to connect to machine \"" + remoteMachineName + "\":\n\n" + errMsg);
System.err.println("Unable to connect to machine \"" + debugServerName + "\":\n\n" + errMsg);
Copy link
Contributor

@plummercj plummercj Mar 3, 2021

Choose a reason for hiding this comment

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

"machine" -> "debug server"

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 4, 2021

Choose a reason for hiding this comment

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

I fixed it in new commit.

Loading

@YaSuenag YaSuenag changed the title 8262520: Add CLHSDB command to connect to debug server 8262520: Add SA CommandProcessor support to connect to debug server Mar 4, 2021
@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 4, 2021

Yes, I'm starting to come to the same conclusion. The only code that constructs a CLHSDB object, and therefore triggers the potential call to doUsage(), is CLHSDB.main(), and it is only called from SALauncher when using jhsdb clhsdb. We could get rid of main() and have some other entrypoint that is passed the values of all the possible options, rather than having to parse them in CLHSDB.java.

There is one test invokes java with the CLHSDB class. See SABase.java. It's part of ciReplay testing. I think it would be easily modified to just run jhsdb clhsdb

I think debugd and jsnap are also remove main() if they have because they do not provide commands like jstack.
I will file it to JBS, and will fix them later.

Loading

@plummercj
Copy link
Contributor

@plummercj plummercj commented Mar 4, 2021

BTW, referring back to the change to have attach handle pids and debug servernames, and the concern about a host having a numeric name, we already have code that assumes we don't have to deal with this in Tools.java. I just stumbled across this:

           try {
              pid = Integer.parseInt(args[0]);
              debugeeType = DEBUGEE_PID;
           } catch (NumberFormatException e) {
              // try remote server
              remoteServer = args[0];
              debugeeType  = DEBUGEE_REMOTE;
           }

Loading

@plummercj
Copy link
Contributor

@plummercj plummercj commented Mar 4, 2021

/csr needed

Loading

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

@openjdk openjdk bot commented Mar 4, 2021

@plummercj has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@YaSuenag please create a CSR request and add link to it in JDK-8262520. This pull request cannot be integrated until the CSR request is approved.

Loading

@plummercj
Copy link
Contributor

@plummercj plummercj commented Mar 4, 2021

I think the changes look good now. Please create a CSR and then we can finish the review of this PR.

Loading

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 4, 2021

I filed CSR and finalized it: https://bugs.openjdk.java.net/browse/JDK-8263005

BTW, referring back to the change to have attach handle pids and debug servernames, and the concern about a host having a numeric name, we already have code that assumes we don't have to deal with this in Tools.java. I just stumbled across this:

Thanks! I felt confident for current PR :)

Loading

@YaSuenag YaSuenag changed the title 8262520: Add SA CommandProcessor support to connect to debug server 8262520: Add SA Command Line Debugger support to connect to debug server Mar 5, 2021
out.stderrShouldBeEmptyIgnoreDeprecatedWarnings();
out.shouldMatch("^java/lang/Object @0x[0-9a-f]+$"); // for "class java.lang.Object"
out.shouldMatch("^java/lang/String @0x[0-9a-f]+$"); // for "class java.lang.String"
out.shouldHaveExitValue(0);
Copy link
Contributor

@plummercj plummercj Mar 5, 2021

Choose a reason for hiding this comment

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

I think you should do more error checking here. If you look at ClhsdbLauncher you will see:

        // This will detect most SA failures, including during the attach.
        oa.shouldNotMatch("^sun.jvm.hotspot.debugger.DebuggerException:.*$");
        // This will detect unexpected exceptions, like NPEs and asserts, that are caught
        // by sun.jvm.hotspot.CommandProcessor.
        oa.shouldNotMatch("^Error: .*$");

I think you could use something similar here.

Loading

Copy link
Member Author

@YaSuenag YaSuenag Mar 5, 2021

Choose a reason for hiding this comment

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

I added them in new commit.
BTW I found out to check warnings which relate to -Xcheck:jni at ClhsdbLauncher. I did not add them to ClhsdbAttachToDebugServer because they are not the issue which relates to debug server. Is it ok?

Loading

Copy link
Contributor

@plummercj plummercj Mar 5, 2021

Choose a reason for hiding this comment

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

No need for the -Xcheck:jni checks. I added that recently because we did have a bug, but I think we get more than enough testing with it already when running the ClhsdbLauncher tests.

Loading

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

@openjdk openjdk bot commented Mar 9, 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:

8262520: Add SA Command Line Debugger support to connect to debug server

Reviewed-by: cjplummer, kevinw

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

  • b7f0b3f: 8252173: Use handles instead of jobjects in modules.cpp
  • a6e34b3: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()
  • fbe40e8: 8252399: Update mapMulti documentation to use type test pattern instead of instanceof once JEP 375 exits preview
  • 0f2402d: 8263190: Update java.io, java.math, and java.text to use instanceof pattern variable
  • 4f0a12e: 8262323: do not special case JVMCI in tiered compilation policy
  • 3022baa: 8263167: IGV: build fails with "taskdef AutoUpdate cannot be found"
  • 0bc4562: 8263068: Rename safefetch.hpp to safefetch.inline.hpp
  • 5bfc5fd: 8263051: Modernize the code in the java.awt.color package
  • 5b9b170: 8262955: Unify os::fork_and_exec() across Posix platforms
  • 39b1113: 8262161: Refactor manual I/O stream copying in java.desktop to use new convenience APIs
  • ... and 112 more: https://git.openjdk.java.net/jdk/compare/03d888f463c0a6e3fee70ed8ad606fc0a3082636...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 9, 2021
@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented Mar 9, 2021

/integrate

Loading

@openjdk openjdk bot closed this Mar 9, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 9, 2021

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

  • e5ce97b: 8263206: assert(*error_msg != '\0') failed: Must have error_message while parsing -XX:CompileCommand=unknown
  • 3212f80: 8261937: LambdaForClassInBaseArchive: SimpleApp$$Lambda$1 missing
  • 2218e72: 8262486: Merge trivial JDWP agent changes from the loom repo to the jdk repo
  • 86fac95: 8263142: Delete unused entry points in libawt/libawt_xawt/libawt_headless
  • b7f0b3f: 8252173: Use handles instead of jobjects in modules.cpp
  • a6e34b3: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()
  • fbe40e8: 8252399: Update mapMulti documentation to use type test pattern instead of instanceof once JEP 375 exits preview
  • 0f2402d: 8263190: Update java.io, java.math, and java.text to use instanceof pattern variable
  • 4f0a12e: 8262323: do not special case JVMCI in tiered compilation policy
  • 3022baa: 8263167: IGV: build fails with "taskdef AutoUpdate cannot be found"
  • ... and 116 more: https://git.openjdk.java.net/jdk/compare/03d888f463c0a6e3fee70ed8ad606fc0a3082636...master

Your commit was automatically rebased without conflicts.

Pushed as commit 70342e8.

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

Loading

@YaSuenag YaSuenag deleted the JDK-8262520 branch Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants