RFR: 8262520: Add CLHSDB command to connect to debug server [v3]

[Chris Plummer]

On Tue, 2 Mar 2021 12:55:04 GMT, Yasumasa Suenaga <ysuenaga at openjdk.org> wrote:

>> `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 <hostname>'.
>
> Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix comments

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?

src/jdk.hotspot.agent/doc/clhsdb.html line 39:

> 37: Available commands:
> 38:   assert true | false <font color="red">turn on/off asserts in SA code</font>
> 39:   attach pid | exec core | remote_server  <font color="red">attach SA to a process, core, or remote debug server</font>

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.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CLHSDB.java line 53:

> 51:         // and coreFilename is the pathname of a core file we are
> 52:         // supposed to attach to.
> 53:         // Finally, if remoteMachineName != null, we are supposed to connect to remote debug server.

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

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CLHSDB.java line 125:

> 123:     private String execPath;
> 124:     private String coreFilename;
> 125:     private String remoteMachineName;

I think `debugServerName` or `remoteDebugServerName` would be better.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CLHSDB.java line 230:

> 228:             System.out.println("Connecting to debug server, please wait...");
> 229:             agent.attach(remoteMachineName);
> 230:             this.remoteMachineName = remoteMachineName;

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?

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/HSDB.java line 436:

> 434:     // and coreFilename is the pathname of a core file we are
> 435:     // supposed to attach to.
> 436:     // Finally, if remoteMachineName != null, we are supposed to connect to remote debug server.

Split into 2 lines.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/HSDB.java line 84:

> 82:   private String execPath;
> 83:   private String coreFilename;
> 84:   private String remoteMachineName;

`debugServerName` or `remoteDebugServerName`

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/HSDB.java line 100:

> 98:     execPath = null;
> 99:     coreFilename = null;
> 100:     remoteMachineName = null;

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

-------------

PR: https://git.openjdk.java.net/jdk/pull/2773