8338894: Deprecate jhsdb debugd for removal #20830

kevinjwalls · 2024-09-03T08:06:14Z

Deprecation annotations and warnings on starting the tool(s).
Handle man page in a separate issue.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8339458 to be approved

Issues

JDK-8338894: Deprecate jhsdb debugd for removal (Enhancement - P4)
JDK-8339458: Deprecate jhsdb debugd for removal (CSR)

Reviewers

Alan Bateman (@AlanBateman - Reviewer) Review applies to 73c0f49f
Chris Plummer (@plummercj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20830/head:pull/20830
$ git checkout pull/20830

Update a local copy of the PR:
$ git checkout pull/20830
$ git pull https://git.openjdk.org/jdk.git pull/20830/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20830

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20830.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-03T08:06:45Z

👋 Welcome back kevinw! 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 · 2024-09-03T08:08:10Z

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

8338894: Deprecate jhsdb debugd for removal

Reviewed-by: cjplummer, alanb

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

fbe2629: 8339635: StringConcatFactory optimization for CompactStrings off
8e580ec: 8338123: Linker crash when building a downcall handle with many arguments in x64
5b72bbf: 8339519: Remove size field from instructions
0df10bb: 8339466: Enumerate shared stubs and define static fields and names via declarations
9ebc2ec: 8339317: Optimize ClassFile writeBuffer
d2b36f0: 8339642: Reduce overheads in InvokerBytecodeGenerator
cb00333: 8339640: Reduce construction overheads in StringConcatFactory$InlineHiddenClassStrategy
260908e: 8339592: Simplify and remove unused code in ObjectMethods.
febbd99: 8339168: Optimize ClassFile Util slotSize
a1eebbd: 8339576: Speed up raw bytecode processing in ClassFile API
... and 63 more: https://git.openjdk.org/jdk/compare/0e6bb514c8ec7c4a7100fe06eaa9e954a74fda30...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.

openjdk · 2024-09-03T08:08:42Z

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

kevinjwalls · 2024-09-03T08:24:15Z

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/HotSpotAgent.java:
Add annotation Deprecated(since="24", forRemoval=true) on the various startServer methods, and shutdownServer

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/SALauncher.java:
Add deprecation warnings in help messages.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/DebugServer.java:
A standalone launcher, providing another main entry point (separate to jhsdb), which creates a HotSpotAgent() and calls startServer(). This class should have the same deprecation annotation, and print a warning in its main method.

Handle man page in a separate issue.

bash-4.2$ build/linux-x64/images/jdk/bin/jhsdb
    clhsdb              command line debugger
    hsdb                ui debugger                                                                                                
    debugd --help       to get more information (deprecated)
    jstack --help       to get more information
    jmap   --help       to get more information
    jinfo  --help       to get more information
    jsnap  --help       to get more information
bash-4.2$      

bash-4.2$ build/linux-x64/images/jdk/bin/jhsdb debugd --help                                                                   
WARNING: debugd is deprecated and will be removed in a future release.                                                           
  --serverid <id>     ...etc...
  
  
bash-4.2$ build/linux-x64/images/jdk/bin/jhsdb jstack --help
    --locks                 To print java.util.concurrent locks.                                                                 
    --mixed                 To print both Java and native frames (mixed mode).
    --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>
    --connect [<serverid>@]<host>[:registryport][/servername] To connect to a remote debug server (debugd).

WARNING: debugd (and therefore --connect) are deprecated and will be removed in a future release.

    The --core and --exe options must be set together to give the core..etc...


bash-4.2$ build/linux-x64/images/jdk/bin/jhsdb debugd --pid 123                                                                
WARNING: debugd is deprecated and will be removed in a future release.                                                         
Attaching to process ID 123 and starting RMI services, please wait...  
...etc...

Additionally, update the html docs with the same warning:
src/jdk.hotspot.agent/doc/index.html
src/jdk.hotspot.agent/doc/transported_core.html

mlbridge · 2024-09-03T08:44:32Z

Webrevs

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/HotSpotAgent.java

AlanBateman

I can look at the CSR when it is ready.

kevinjwalls · 2024-09-03T14:48:05Z

OK one other addition, as jhsdb debugd and the --connect option are two sides of the same feature:

We generally expect attaches between the same JDK version (see the SA version mismatch warning),
so a later JDK with the ability to connect to a previous JDK which still has debugd makes no sense.

We can warn at the tool running with --connect, e.g.

bash-4.2$ build/linux-x64/images/jdk/bin/jhsdb jstack --connect serverid@debugserver:1234/servername
WARNING: debugd (and therefore --connect) are deprecated and will be removed in a future release.
Attaching to remote server serverid@debugserver:1234/servername, please wait...

In our own tests, the debugd warning message causes no problems, but the warning on using --connect causes some test disruption, will update.

AlanBateman · 2024-09-03T15:02:34Z

OK one other addition, as jhsdb debugd and the --connect option are two sides of the same feature:

We generally expect attaches between the same JDK version (see the SA version mismatch warning), so a later JDK with the ability to connect to a previous JDK which still has debugd makes no sense.

We can warn at the tool running with --connect, e.g.
bash-4.2$ build/linux-x64/images/jdk/bin/jhsdb jstack --connect serverid@debugserver:1234/servername
WARNING: debugd (and therefore --connect) are deprecated and will be removed in a future release.
Attaching to remote server serverid@debugserver:1234/servername, please wait...

I agree it doesn't make sense. For the warning message it might be clearer to put the tool name into the message, as in something like "WARNING: jhsdb jstack --connect is deprecated ..."). The reason is that someone using "jhsdb jstack --connect" is not running the "debugd" command so would be confusing to get that in the message.

src/jdk.hotspot.agent/doc/transported_core.html

src/jdk.hotspot.agent/doc/index.html

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/DebugServer.java

Co-authored-by: Chris Plummer <chris.plummer@oracle.com>

AlanBateman · 2024-09-04T09:35:39Z

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/SALauncher.java

@@ -69,6 +69,8 @@ private static boolean commonHelp(String mode, boolean canConnectToRemote) {
        System.out.println("    --exe <executable for corefile>");
        if (canConnectToRemote) {
            System.out.println("    --connect [<serverid>@]<host>[:registryport][/servername] To connect to a remote debug server (debugd).");
+            System.out.println();
+            System.out.println("WARNING: debugd (and therefore --connect) are deprecated and will be removed in a future release.");


This is -help output so I assume should be "The --connect option is deprecated and will be removed in a future release".

AlanBateman

Can you paste in up to date examples of the output from jhsdb debugd .. and `jhsdb jstack --connect .." so we can see the warnings. The output is in the first comment has warnings in the --help output, and there isn't an example using --connect that I think we need to review properly.

kevinjwalls · 2024-09-04T11:10:54Z

Updated, including tests as the new --connect warning needs to be ignored.
We were using OutputAnalyzer's stderrShouldBeEmptyIgnoreDeprecatedWarnings() but it's specifically ignoring ".* VM warning:.* deprecated.*". These changes are easy to find and revert when removal happens.

Alan you mention "warnings in the --help output", I still have a warning in the middle of the help text here, I tried it elsewhere but liked having the warning about --connect next to where --connect is mentioned in the help.

It seems like unnecessary additional changes would be needed to enable the common argument helper method to know what command is being invoked to make the --connect message specific to jhsdb jstack --connect etc. Also one message to check for in tests.

Updated messages:

bash-4.2$ build/linux-x64/images/jdk/bin/jhsdb
    clhsdb              command line debugger
    hsdb                ui debugger
    debugd --help       to get more information (deprecated)
    jstack --help       to get more information
    jmap   --help       to get more information
    jinfo  --help       to get more information
    jsnap  --help       to get more information

bash-4.2$ build/linux-x64/images/jdk/bin/jhsdb debugd --help
WARNING: debugd is deprecated and will be removed in a future release.
    --serverid <id>         A unique identifier....

bash-4.2$ build/linux-x64/images/jdk/bin/jhsdb debugd --pid 123
WARNING: debugd is deprecated and will be removed in a future release.
Attaching to process ID 123 and starting RMI services, please wait...

bash-4.2$ build/linux-x64/images/jdk/bin/jhsdb jstack --help
    --locks                 To print java.util.concurrent locks.
    --mixed                 To print both Java and native frames (mixed mode).
    --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>
    --connect [<serverid>@]<host>[:registryport][/servername] To connect to a remote debug server (debugd).

WARNING: --connect is deprecated and will be removed in a future release.

    The --core and --exe options...

bash-4.2$ build/linux-x64/images/jdk/bin/jhsdb jstack --connect serverid@debugserver:1234/servername
WARNING: --connect is deprecated and will be removed in a future release.
Attaching to remote server serverid@debugserver:1234/servername, please wait...

AlanBateman · 2024-09-04T11:13:28Z

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/SALauncher.java

@@ -69,6 +69,8 @@ private static boolean commonHelp(String mode, boolean canConnectToRemote) {
        System.out.println("    --exe <executable for corefile>");
        if (canConnectToRemote) {
            System.out.println("    --connect [<serverid>@]<host>[:registryport][/servername] To connect to a remote debug server (debugd).");
+            System.out.println();
+            System.err.println("WARNING: --connect is deprecated and will be removed in a future release.");


This is jhsdb $TOOL --help output. Shouldn't this just have "(deprecated)" before "To connect ..." rather than a blank line and warning? I assume the changes should only print "WARNING" if the user attempts to run jhsdb $TOOL -connect ...

OK yes understood, we can keep things less ALARMING unless you try and use the options.

--connect is already a long line, so adding "(deprecated)" I think gets lost. We can do:

bash-4.2$ build/linux-x64/images/jdk/bin/jhsdb jstack --help --locks To print java.util.concurrent locks. --mixed To print both Java and native frames (mixed mode). --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> --connect [<serverid>@]<host>[:registryport][/servername] To connect to a remote debug server (debugd). The --connect option is deprecated and will be removed in a future release. 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. The --connect option can be set to connect to a debug server (debugd). --core, --pid, and --connect are mutually exclusive. Examples: jhsdb jstack --pid 1234 or jhsdb jstack --core ./core.1234 --exe ./myexe or jhsdb jstack --connect serverid@debugserver:1234/servername bash-4.2$

Actually the text "The --connect option can be set to connect to a debug server (debugd)." is in there twice. Will remove the second one.

AlanBateman · 2024-09-04T11:16:29Z

you mention "warnings in the --help output", I still have a warning in the middle of the help text here, I tried it elsewhere but liked having the warning about --connect next to where --connect is mentioned in the help.

I had added a comment before seeing your latest update. My view is that it would be better to only print "WARNING" when the user tried to use a deprecated command/option. In the help output it can show that the command/option is deprecated but blank line + WARNING in the help looks a bit unusual (to me anyway).

AlanBateman

The WARNING and help output look okay now.

kevinjwalls · 2024-09-04T13:42:57Z

Thanks, have updated CSR also.

src/jdk.hotspot.agent/doc/index.html

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/SALauncher.java

test/hotspot/jtreg/serviceability/sa/sadebugd/ClhsdbTestConnectArgument.java

plummercj

Looks good. Make sure you get good test coverage of you OutputAnalyzer change.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/SALauncher.java

kevinjwalls · 2024-09-06T08:22:20Z

Make sure you get good test coverage of you OutputAnalyzer change.

That was the right thing to say, as I didn't have it quite right. stderrShouldBeEmptyIgnoreDeprecatedWarnings() needs to work if both kinds of deprecation message occurred. 8-)
It needs to remove both patterns, then check, like in the other relevant method, stderrShouldMatchIgnoreDeprecatedWarnings(). This can be in a common method. Updated.

kevinjwalls · 2024-09-10T16:26:39Z

/integrate

openjdk · 2024-09-10T16:28:04Z

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

be0dca0: 8339698: x86 unused andw/orw/xorw/addw encoding could be removed
64a79d8: 8335625: Update Javadoc for GetCpuLoad
c246ede: 8339799: Reduce work done in j.l.invoke bytecode generators
38441b3: 8339677: [vectorapi] YYYXXXVector::withLaneHelper and laneHelper should use Double::doubleToRawLongBits/Float::floatToRawIntBits
fb51c1e: 8339837: Remove unused BootstrapMethodsInvokers.isLambdaMetafactoryCondyBSM
4d597de: 8338930: StringConcatFactory hardCoded string concatenation strategy
ad10493: 8338526: Don't store abstract and interface Klasses in class metaspace
0d8e52b: 8339800: Prefer invokeBasic in BootstrapMethodInvokers
64de781: 8339587: runtime/reflect/ReflectOutOfMemoryError.java fails with "bootstrap method initialization exception"
125f743: 8305489: runtime/ErrorHandling/TestDwarf.java fails in some Linux configurations after JDK-8303805
... and 94 more: https://git.openjdk.org/jdk/compare/0e6bb514c8ec7c4a7100fe06eaa9e954a74fda30...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-09-10T16:28:11Z

@kevinjwalls Pushed as commit 3352522.

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

8338894: Deprecate jhsdb debugd for removal

0939558

openjdk bot added the csr Pull request needs approved CSR before integration label Sep 3, 2024

openjdk bot added the serviceability serviceability-dev@openjdk.org label Sep 3, 2024

Update html docs

e224d54

kevinjwalls marked this pull request as ready for review September 3, 2024 08:40

openjdk bot added the rfr Pull request is ready for review label Sep 3, 2024

AlanBateman reviewed Sep 3, 2024

View reviewed changes

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/HotSpotAgent.java Show resolved Hide resolved

AlanBateman approved these changes Sep 3, 2024

View reviewed changes

plummercj reviewed Sep 3, 2024

View reviewed changes

src/jdk.hotspot.agent/doc/transported_core.html Outdated Show resolved Hide resolved

src/jdk.hotspot.agent/doc/index.html Outdated Show resolved Hide resolved

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/DebugServer.java Show resolved Hide resolved

kevinjwalls and others added 2 commits September 4, 2024 10:16

Update src/jdk.hotspot.agent/doc/index.html

3e20294

Co-authored-by: Chris Plummer <chris.plummer@oracle.com>

Update src/jdk.hotspot.agent/doc/transported_core.html

eb0ad9e

Co-authored-by: Chris Plummer <chris.plummer@oracle.com>

AlanBateman reviewed Sep 4, 2024

View reviewed changes

AlanBateman suggested changes Sep 4, 2024

View reviewed changes

kevinjwalls added 2 commits September 4, 2024 11:58

Test updates for --connect

cf860df

--connect message

856f0e9

AlanBateman reviewed Sep 4, 2024

View reviewed changes

--connect option text, not a WARNING

73c0f49

AlanBateman approved these changes Sep 4, 2024

View reviewed changes

plummercj reviewed Sep 4, 2024

View reviewed changes

remove p

7aa89ae

OutputAnalyzer to ignore additional deprecation messages.

4a5357d

openjdk bot removed the rfr Pull request is ready for review label Sep 5, 2024

kevinjwalls added 2 commits September 5, 2024 14:03

oops

bfc2020

whitespace

1f7f76f

openjdk bot added the rfr Pull request is ready for review label Sep 5, 2024

plummercj approved these changes Sep 5, 2024

View reviewed changes

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/SALauncher.java Show resolved Hide resolved

Correction to test library change

a373cc6

plummercj approved these changes Sep 6, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Sep 6, 2024

openjdk bot added the integrated Pull request has been integrated label Sep 10, 2024

openjdk bot closed this Sep 10, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8338894: Deprecate jhsdb debugd for removal #20830

8338894: Deprecate jhsdb debugd for removal #20830

kevinjwalls commented Sep 3, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 3, 2024

openjdk bot commented Sep 3, 2024 •

edited

Loading

openjdk bot commented Sep 3, 2024

kevinjwalls commented Sep 3, 2024 •

edited

Loading

mlbridge bot commented Sep 3, 2024 •

edited

Loading

AlanBateman left a comment

kevinjwalls commented Sep 3, 2024 •

edited

Loading

AlanBateman commented Sep 3, 2024 •

edited

Loading

AlanBateman Sep 4, 2024

AlanBateman left a comment •

edited

Loading

kevinjwalls commented Sep 4, 2024

AlanBateman Sep 4, 2024

kevinjwalls Sep 4, 2024 •

edited

Loading

AlanBateman commented Sep 4, 2024

AlanBateman left a comment

kevinjwalls commented Sep 4, 2024

plummercj left a comment

kevinjwalls commented Sep 6, 2024

kevinjwalls commented Sep 10, 2024

openjdk bot commented Sep 10, 2024

openjdk bot commented Sep 10, 2024

8338894: Deprecate jhsdb debugd for removal #20830

8338894: Deprecate jhsdb debugd for removal #20830

Conversation

kevinjwalls commented Sep 3, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 3, 2024

openjdk bot commented Sep 3, 2024 • edited Loading

openjdk bot commented Sep 3, 2024

kevinjwalls commented Sep 3, 2024 • edited Loading

mlbridge bot commented Sep 3, 2024 • edited Loading

Webrevs

AlanBateman left a comment

Choose a reason for hiding this comment

kevinjwalls commented Sep 3, 2024 • edited Loading

AlanBateman commented Sep 3, 2024 • edited Loading

AlanBateman Sep 4, 2024

Choose a reason for hiding this comment

AlanBateman left a comment • edited Loading

Choose a reason for hiding this comment

kevinjwalls commented Sep 4, 2024

AlanBateman Sep 4, 2024

Choose a reason for hiding this comment

kevinjwalls Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

AlanBateman commented Sep 4, 2024

AlanBateman left a comment

Choose a reason for hiding this comment

kevinjwalls commented Sep 4, 2024

plummercj left a comment

Choose a reason for hiding this comment

kevinjwalls commented Sep 6, 2024

kevinjwalls commented Sep 10, 2024

openjdk bot commented Sep 10, 2024

openjdk bot commented Sep 10, 2024

kevinjwalls commented Sep 3, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 3, 2024 •

edited

Loading

kevinjwalls commented Sep 3, 2024 •

edited

Loading

mlbridge bot commented Sep 3, 2024 •

edited

Loading

kevinjwalls commented Sep 3, 2024 •

edited

Loading

AlanBateman commented Sep 3, 2024 •

edited

Loading

AlanBateman left a comment •

edited

Loading

kevinjwalls Sep 4, 2024 •

edited

Loading