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

8262504: Some CLHSDB command cannot know they run on remote debugger #2766

Closed
wants to merge 1 commit into from

Conversation

YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented Feb 28, 2021

pmap and pstack CLHSDB command do not work on remote debugger, we can see following error message:

hsdb> pmap
not yet implemented (debugger does not support CDebugger)!

However, SA has different message for this purpose:

if (getDebugeeType() == DEBUGEE_REMOTE) {
out.println("remote configuration is not yet implemented");
} else {
out.println("not yet implemented (debugger does not support CDebugger)!");
}

SA should show "remote configuration is not yet implemented" when it works on remote debugger.


Progress

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

Issue

  • JDK-8262504: Some CLHSDB command cannot know they run on remote debugger

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 28, 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.

@openjdk
Copy link

openjdk bot commented Feb 28, 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.

@openjdk openjdk bot added the serviceability serviceability-dev@openjdk.org label Feb 28, 2021
@YaSuenag YaSuenag marked this pull request as ready for review March 1, 2021 01:44
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 1, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 1, 2021

Webrevs

Comment on lines +54 to +69
public Tool(HotSpotAgent agent) {
this.agent = agent;
if (agent == null) {
jvmDebugger = null;
debugeeType = -1;
} else {
jvmDebugger = agent.getDebugger();
debugeeType = switch (agent.getStartupMode()) {
case HotSpotAgent.PROCESS_MODE -> DEBUGEE_PID;
case HotSpotAgent.CORE_FILE_MODE -> DEBUGEE_CORE;
case HotSpotAgent.REMOTE_MODE -> DEBUGEE_REMOTE;
default -> throw new IllegalStateException("Invalid attach mode");
};
}
}

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.

This section seems to be the key to your changes, but I have to admit that I don't follow how it all ties in together. Maybe you can explain the overall structure of the Tool subclasses to help explain. They seem to be overloaded with modes that make it hard to follow all the logic. For example, you've added the Tool(HotSpotAgent agent) constructor, and we already have Tool() and Tool(JVMDebugger d). It's unclear to me what the use case is for each. Also, we have about 10 subclasses of Tool. Why do only PMap an PStack need this new constructor.

BTW, looking at Tool and its subclasses has made me realize we have a whole other area of historical tool baggage, similar to what we are discussing with CLHSDB and HSDB. My guess is all these tool subclasses used to be invoked directly from the java command line. Now they are all hidden behind jhsdb subcommands, but still carry the old java command line support. In addition, when adding jhsdb and it's subcommands, it looks like there was an effort to mimic the existing Attach API commands like jmap and jinfo, so the SA versions of these commands have multiple modes which invoke different Tool subclasses. For example, jhdsb jmap alone can invoke the ClassLoaderStats, FinalizerInfo, ObjectHistogram, PMap, and HeapSummary tools, in addition to dumping the heap graph in HProf or GXL format. I think things would have been simpler and cleaner if each of these were made separate jhsdb subcommands, but it's probably too late for that. I suppose we could deprecate the existing sub-commands and add all these new sub-commands, but that will just make things harder for us until the old commands are eventually removed, which might never happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

This section seems to be the key to your changes, but I have to admit that I don't follow how it all ties in together. Maybe you can explain the overall structure of the Tool subclasses to help explain. They seem to be overloaded with modes that make it hard to follow all the logic. For example, you've added the Tool(HotSpotAgent agent) constructor, and we already have Tool() and Tool(JVMDebugger d). It's unclear to me what the use case is for each. Also, we have about 10 subclasses of Tool. Why do only PMap an PStack need this new constructor.

All of traditional serviceability commands (jstack, jmap, etc...) seem to extend Tool because it has a function to parse commandline args and to attach debuggee commonly.
Tool supports several attach mechanism: Tool() supports attach debuggee manually, Tool(JVMDebugger) supports attach debuggee via available JVMDegbugger. HotSpotAgent communicates to debugee, so Tool would create its instance. Thus I added Tool(HotSpotAgent) to use available remote debug server.

AFAICS only PMap and PStack do not support remote debug server, so I changed for them.
(I plan to support pmap and pstack when we attach remote debug server, so it is important to know debuggee type.)

BTW, looking at Tool and its subclasses has made me realize we have a whole other area of historical tool baggage, similar to what we are discussing with CLHSDB and HSDB. My guess is all these tool subclasses used to be invoked directly from the java command line. Now they are all hidden behind jhsdb subcommands, but still carry the old java command line support. In addition, when adding jhsdb and it's subcommands, it looks like there was an effort to mimic the existing Attach API commands like jmap and jinfo, so the SA versions of these commands have multiple modes which invoke different Tool subclasses. For example, jhdsb jmap alone can invoke the ClassLoaderStats, FinalizerInfo, ObjectHistogram, PMap, and HeapSummary tools, in addition to dumping the heap graph in HProf or GXL format. I think things would have been simpler and cleaner if each of these were made separate jhsdb subcommands, but it's probably too late for that. I suppose we could deprecate the existing sub-commands and add all these new sub-commands, but that will just make things harder for us until the old commands are eventually removed, which might never happen.

I agree with you it is too late to happen.
As I commented in #2773 (comment) , we can do simplify for them as possible. For example, we can remove main() and related methods (e.g. showing usage) from them. If we can remove commandline support from all tool classes, It might help a little to maintain.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understanding correctly, we have two users of Tools.java. One is SALauncher and the other is CommandProcessor. When we use SALauncher, we determine the debugeeType based on the command line arguments used. When using the CommandProcessor, we need access to the HotSpotAgent to get the debuggeeType, since it has stored this in its startupMode field . So the only reason for this new constructor is so we can set debugeeType, and then as you've pointed out, that makes the following existing code work correctly:

 if (getDebugeeType() == DEBUGEE_REMOTE) { 
     out.println("remote configuration is not yet implemented"); 
 } else { 
     out.println("not yet implemented (debugger does not support CDebugger)!"); 
 } 

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with you it is too late to happen.
As I commented in #2773 (comment) , we can do simplify for them as possible. For example, we can remove main() and related methods (e.g. showing usage) from them. If we can remove commandline support from all tool classes, It might help a little to maintain.

Yes, I'd like to see a lot of this excess baggage go away. I also wouldn't mind at least considering re-imagining the command line interface for all these Tool subclasses, but would only do so if we could quickly deprecate what we currently have. We already have enough cases of being able to do the same thing more than one way (I think in some cases 5 ways). We don't need one more, but on the other hand, as an example, having jhsdb jmap support dumping the java heap just doesn't make any sense. My guess is the first jmap just dumped the memory map of loaded libraries, and then more and more unrelated subcommands kept being added to it, but I'm not sure of the history. The Attach API jmap command suffers from the same issue, although oddly is does not support the library mapping output like the SA version does. I'm not sure which version of jmap came first and what it initially looked like.

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot agree with restructuring sub commands and options in jhsdb for compatibility.

Since jhsdb is introduced, SA tools (jstack, jmap, and so on) has been removed the feature to use SA, they are for live process only with Attach API. So I guess we can ignore about SA interface for them. In addition, they are marked as unsupported tool in the document.

Thus I guess we can think about jhsdb and jcmd (dcmd) interface only. If so, I think we can remove some excess baggages from them (e.g. command line support), and it is worth to work for maintenance in future.

But this PR does not aim it, it will be the next step.

@openjdk
Copy link

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

8262504: Some CLHSDB command cannot know they run on remote debugger

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

  • 8c1112a: 8261916: gtest/GTestWrapper.java vmErrorTest.unimplemented1_vm_assert failed
  • 1e57087: 8263392: Allow current thread to be specified in ExceptionMark
  • 4d1c08c: 8263616: 'Deprecatd' typo in src/hotspot/share/classfile/classFileParser.cpp
  • 0c718ab: 8262277: URLClassLoader.getResource throws undocumented IllegalArgumentException
  • 4f1cda4: 8263387: G1GarbageCollection JFR event gets gc phase, not gc type
  • 5ab5244: 8263514: Minor issue in JavacFileManager.SortFiles.REVERSE
  • 771b146: 8245025: MoveAndUpdateClosure::do_addr calls function with side-effects in an assert
  • 46d78f0: 6539707: (fc) MappedByteBuffer.force() method throws an IOException in a very simple test
  • 189289d: 8262326: MaxMetaspaceSize does not have to be aligned to metaspace commit alignment
  • d825198: 8263556: remove @modules java.base from tests
  • ... and 196 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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 5, 2021
@YaSuenag
Copy link
Member Author

PING: could you review it? I need one more reviewer, thanks!

Copy link
Contributor

@sspitsyn sspitsyn left a comment

Choose a reason for hiding this comment

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

Hi Yasumasa,
The fix looks good to me.
Thanks,
Serguei

@YaSuenag
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Mar 16, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 16, 2021
@openjdk
Copy link

openjdk bot commented Mar 16, 2021

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

  • d896246: 8263420: Incorrect function name in NSAccessibilityStaticText native peer implementation
  • 8c1112a: 8261916: gtest/GTestWrapper.java vmErrorTest.unimplemented1_vm_assert failed
  • 1e57087: 8263392: Allow current thread to be specified in ExceptionMark
  • 4d1c08c: 8263616: 'Deprecatd' typo in src/hotspot/share/classfile/classFileParser.cpp
  • 0c718ab: 8262277: URLClassLoader.getResource throws undocumented IllegalArgumentException
  • 4f1cda4: 8263387: G1GarbageCollection JFR event gets gc phase, not gc type
  • 5ab5244: 8263514: Minor issue in JavacFileManager.SortFiles.REVERSE
  • 771b146: 8245025: MoveAndUpdateClosure::do_addr calls function with side-effects in an assert
  • 46d78f0: 6539707: (fc) MappedByteBuffer.force() method throws an IOException in a very simple test
  • 189289d: 8262326: MaxMetaspaceSize does not have to be aligned to metaspace commit alignment
  • ... and 197 more: https://git.openjdk.java.net/jdk/compare/03d888f463c0a6e3fee70ed8ad606fc0a3082636...master

Your commit was automatically rebased without conflicts.

Pushed as commit e03a594.

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

@YaSuenag YaSuenag deleted the JDK-8262504 branch March 16, 2021 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
3 participants