Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v8]
On Tue, 26 Mar 2024 17:01:49 GMT, Kevin Walls wrote: >> Introduce the jcmd "VM.inspect" to implement access to detailed JVM object >> information. >> >> Not recommended for live production use. Requires UnlockDiagnosticVMOptions >> and not included in jcmd help output, to remind us this is not a >> general-purpose customer-facing tool. > > Kevin Walls has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 20 additional > commits since the last revision: > > - Change to jcmd VM.inspect > - Merge remote-tracking branch 'upstream/master' into > 8318026_jcmd_VMdebug_command > - Test update > - Show description if unknown subcommand. > - Remove unnecessary 'events' subcommand. > - Usage correction > - Help to clarify this is VM inspection. Comment to relate source to > debug.cpp. > - jcheck trailing whitespace > - Test update omitted from previous commit. > - Merge remote-tracking branch 'upstream/master' into > 8318026_jcmd_VMdebug_command > - ... and 10 more: https://git.openjdk.org/jdk/compare/730651a5...739bcbfa Kevin, the PR title needs an update to match the CR title: `jcmd should provide access to detailed JVM object information` - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-2021479991
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v8]
On Tue, 26 Mar 2024 17:01:49 GMT, Kevin Walls wrote: >> Introduce the jcmd "VM.inspect" to implement access to detailed JVM object >> information. >> >> Not recommended for live production use. Requires UnlockDiagnosticVMOptions >> and not included in jcmd help output, to remind us this is not a >> general-purpose customer-facing tool. > > Kevin Walls has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 20 additional > commits since the last revision: > > - Change to jcmd VM.inspect > - Merge remote-tracking branch 'upstream/master' into > 8318026_jcmd_VMdebug_command > - Test update > - Show description if unknown subcommand. > - Remove unnecessary 'events' subcommand. > - Usage correction > - Help to clarify this is VM inspection. Comment to relate source to > debug.cpp. > - jcheck trailing whitespace > - Test update omitted from previous commit. > - Merge remote-tracking branch 'upstream/master' into > 8318026_jcmd_VMdebug_command > - ... and 10 more: https://git.openjdk.org/jdk/compare/0a4d6c74...739bcbfa It looks good in general. Still need to review the test. src/hotspot/share/services/diagnosticCommand.cpp line 59: > 57: #include "runtime/jniHandles.hpp" > 58: #include "runtime/os.hpp" > 59: #include "runtime/threads.hpp" Is the `#include` lines a leftover from the initial version? Do we still need them? - PR Review: https://git.openjdk.org/jdk/pull/17655#pullrequestreview-1961713172 PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1540097938
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v8]
> Introduce the jcmd "VM.inspect" to implement access to detailed JVM object > information. > > Not recommended for live production use. Requires UnlockDiagnosticVMOptions > and not included in jcmd help output, to remind us this is not a > general-purpose customer-facing tool. Kevin Walls has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 20 additional commits since the last revision: - Change to jcmd VM.inspect - Merge remote-tracking branch 'upstream/master' into 8318026_jcmd_VMdebug_command - Test update - Show description if unknown subcommand. - Remove unnecessary 'events' subcommand. - Usage correction - Help to clarify this is VM inspection. Comment to relate source to debug.cpp. - jcheck trailing whitespace - Test update omitted from previous commit. - Merge remote-tracking branch 'upstream/master' into 8318026_jcmd_VMdebug_command - ... and 10 more: https://git.openjdk.org/jdk/compare/872dc5a5...739bcbfa - Changes: - all: https://git.openjdk.org/jdk/pull/17655/files - new: https://git.openjdk.org/jdk/pull/17655/files/3f566649..739bcbfa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17655=07 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=06-07 Stats: 458556 lines in 4892 files changed: 36637 ins; 91690 del; 330229 mod Patch: https://git.openjdk.org/jdk/pull/17655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655 PR: https://git.openjdk.org/jdk/pull/17655
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v7]
On Tue, 5 Mar 2024 11:31:13 GMT, Kevin Walls wrote: >> Introduce the jcmd "VM.debug" to implement access to a useful set of the >> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". >> >> Not recommended for live production use. Calling these "debug" utilities, >> and not including them in the jcmd help output, is to remind us they are not >> general customer-facing tools. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Test update Kevin, I hope you will update the CSR with new approach, so I'll review it then. - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-2016262416
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v7]
On Tue, 5 Mar 2024 11:31:13 GMT, Kevin Walls wrote: >> Introduce the jcmd "VM.debug" to implement access to a useful set of the >> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". >> >> Not recommended for live production use. Calling these "debug" utilities, >> and not including them in the jcmd help output, is to remind us they are not >> general customer-facing tools. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Test update Having worked with this for a while, I'm quite liking simply adding jcmds, and not adding a group of commands as I initially proposed. The VM.debug group had a story to it, as I mentioned, but maybe that's just history. A user just wants to use jcmd, and if VM.inspect is the command to inspect a JVM object, then OK. The history in debug.cpp may not be that interesting. The command group does make it easy to restrict the commands with UnlockDiagnosticVMOptions, for example, so that was a benefit, but minor as it's just a flag check. Another difference with the command group was you have to read the first argument and direct to the right utility, so you're doing some minor parsing rather than relying on the DiagnosticCommand parser, which is a minor downside. Ease of adding commands is generally similar either way. VM.inspect is the primary utility I want to add here. I plan to rework this do to that. Others can be added in separate jcmds as needs arise. - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-2015039530
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v7]
On Tue, 5 Mar 2024 11:31:13 GMT, Kevin Walls wrote: >> Introduce the jcmd "VM.debug" to implement access to a useful set of the >> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". >> >> Not recommended for live production use. Calling these "debug" utilities, >> and not including them in the jcmd help output, is to remind us they are not >> general customer-facing tools. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Test update Hi Kevin, > > (If you find existing jcmds a mess, feel free to suggest changes as > appropriate.) you touch on a problem here, and why I think adding commands should be done more carefully (and I am guilty of adding commands too). Once these commands are rolled out, they find themselves in blogs, knowledge bases, scripts that are reused for different JDK releases, and so on. It is difficult to change them post-release. That is why good names are important. Cheers, Thomas - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1980807020
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v7]
On Tue, 5 Mar 2024 11:31:13 GMT, Kevin Walls wrote: >> Introduce the jcmd "VM.debug" to implement access to a useful set of the >> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". >> >> Not recommended for live production use. Calling these "debug" utilities, >> and not including them in the jcmd help output, is to remind us they are not >> general customer-facing tools. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Test update Hi, > once you start poking at the JVM innards at this level, I guess you will be > quickly at the limit of what this command can do for you and need to attach a > debugger anyway. I think we all expect a collection of features in the toolbox. This jcmd will not remove the need for a native debugger. I do think that the JVM should be in charge of decoding its own internals, and the built-in tools that do that should be more accessible. > Could this be an own command, e,g, VM.inspect, and possibly limited to debug > VMs? Yes, could be VM.find or VM.inspect. I liked the grouping into VM.debug, they have some common features: a conceptual group, tools that you might want to call alongside another debugger origins in debug.cpp utils (which are still there) "debug" label may discourage casual users from experimenting require UnlockDiagnosticVMOptions common ..but I'm not emotionally connected to the name or the group. I appreciate the feedback. As noted, there is no rule for jcmd grouping. VM.info yes is an example that doesn't fit one particular category, find/inspect is certainly another. (If you find existing jcmds a mess, feel free to suggest changes as appropriate.) > Do we really need this feature in production? Yes I am saying this should be in all builds. It's not only debug builds that people debug, or reproduce problems with. > Don't we have jhsdb for that? [core file/minidump handling] We have jhsdb and the SA tools in general. These have ongoing maintenance issues, the game of catch up as the VM changes is not solved. We need to have options. I will find a different place to propose that alternative in detail. I'll wait and see if there's any other input on the naming... 8-) - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1980761622
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v4]
On Mon, 4 Mar 2024 22:02:53 GMT, Kevin Walls wrote: >> Kevin Walls has updated the pull request incrementally with three additional >> commits since the last revision: >> >> - Usage correction >> - Help to clarify this is VM inspection. Comment to relate source to >> debug.cpp. >> - jcheck trailing whitespace > > Hi Thomas @tstuefe - > > Security concerns certainly needed some thought. We have remote access to > DiagnosticCommands over JMX. I don't see a particular need for that for this > command. I think it was keeping the DCmd flagged as hidden that hides it > from the list of dcmds available in that way. So we have the attach api > controls using local user identity, as we do for everything else. > > Yes, "events" is not that useful, I should remove it. I was taking the > useful parts of debug.cpp, and although the events were in VM.info, I had > missed that since your JDK-8224600, VM.events is its own command now! > > Thread.print with stacks, vs "VM.debug threads" which is just a thread list, > so you have e.g. JavaThread*, name, native id, stack range. > > "find": Usage at the moment, would likely be MessageBoxOnError, and a native > debugger to get the native stacktrace and parameters that include an object > reference you care about. It might be other jcmds if e.g. events capture a > relevant problematic pointer, but likely it involves a native debugger. > Using jcmd behaves better, showing the output where you run jcmd, not in the > VM's current output as the debug.cpp utils do. > Native debugger syntax to call abritrary functions can be awkward, > particularly on Windows, so the jcmd should be a better experience. > > This might be more compelling in post-mortem usage. Am working on that. > i.e. jcmd on a core file. But I am saying it offers some value today. > > The class/method finders, I've heard some enthusiasm for their inclusion. We > don't want to encourage overlap but yes we do have some overlapping jcmds. > > > This is an umbrella but I don't think it's vague. VMDebugDcmd is for > inspecting VM state. It's inspired by debug.cpp utilities, does not need to > implement all of them, but does aim to make them more accessible (I will > assert that they are not well known, which is hard to prove.) > > Do we have a problem with jcmd feature creep? If anything has crept too far > it can be addressed. Looks like the DCmd classes have approximately doubled > since jdk8u but this looks like growth in a good way. > > Thanks > Kevin Hi Kevin, @kevinjwalls , thank you for your explanations. Please find answers inline. > Security concerns certainly needed some thought. We have remote access to > DiagnosticCommands over JMX. I don't see a particular need for that for this > command. I think it was keeping the DCmd flagged as hidden that hides it from > the list of dcmds available in that way. So we have the attach api controls > using local user identity, as we do for everything else. > > Yes, "events" is not that useful, I should remove it. I was taking the useful > parts of debug.cpp, and although the events were in VM.info, I had missed > that since your JDK-8224600, VM.events is its own command now! > > Thread.print with stacks, vs "VM.debug threads" which is just a thread list, > so you have e.g. JavaThread*, name, native id, stack range. > > "find": Usage at the moment, would likely be MessageBoxOnError, and a native > debugger to get the native stacktrace and parameters that include an object > reference you care about. It might be other jcmds if e.g. events capture a > relevant problematic pointer, but likely it involves a native debugger. Using > jcmd behaves better, showing the output where you run jcmd, not in the VM's > current output as the debug.cpp utils do. Native debugger syntax to call > abritrary functions can be awkward, particularly on Windows, so the jcmd > should be a better experience. I remain sceptic here, because in my experience, once you start poking at the JVM innards at this level, I guess you will be quickly at the limit of what this command can do for you and need to attach a debugger anyway. Could this be an own command, e,g, `VM.inspect`, and possibly limited to debug VMs? Do we really need this feature in production? > > This might be more compelling in post-mortem usage. Am working on that. i.e. > jcmd on a core file. But I am saying it offers some value today. Don't we have jhsdb for that? > > The class/method finders, I've heard some enthusiasm for their inclusion. We > don't want to encourage overlap but yes we do have some overlapping jcmds. Yeah, I can see this being useful. > > This is an umbrella but I don't think it's vague. VMDebugDcmd is for > inspecting VM state. It's inspired by debug.cpp utilities, does not need to > implement all of them, but does aim to make them more accessible (I will > assert that they are not well known, which is hard to prove.) > > Do we have
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v7]
> Introduce the jcmd "VM.debug" to implement access to a useful set of the > established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". > > Not recommended for live production use. Calling these "debug" utilities, > and not including them in the jcmd help output, is to remind us they are not > general customer-facing tools. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Test update - Changes: - all: https://git.openjdk.org/jdk/pull/17655/files - new: https://git.openjdk.org/jdk/pull/17655/files/98a46d09..3f566649 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17655=06 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=05-06 Stats: 7 lines in 1 file changed: 0 ins; 4 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655 PR: https://git.openjdk.org/jdk/pull/17655
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v6]
> Introduce the jcmd "VM.debug" to implement access to a useful set of the > established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". > > Not recommended for live production use. Calling these "debug" utilities, > and not including them in the jcmd help output, is to remind us they are not > general customer-facing tools. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Show description if unknown subcommand. - Changes: - all: https://git.openjdk.org/jdk/pull/17655/files - new: https://git.openjdk.org/jdk/pull/17655/files/6ba1c909..98a46d09 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17655=05 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=04-05 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655 PR: https://git.openjdk.org/jdk/pull/17655
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v5]
> Introduce the jcmd "VM.debug" to implement access to a useful set of the > established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". > > Not recommended for live production use. Calling these "debug" utilities, > and not including them in the jcmd help output, is to remind us they are not > general customer-facing tools. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Remove unnecessary 'events' subcommand. - Changes: - all: https://git.openjdk.org/jdk/pull/17655/files - new: https://git.openjdk.org/jdk/pull/17655/files/1d1e20e2..6ba1c909 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17655=04 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=03-04 Stats: 4 lines in 2 files changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655 PR: https://git.openjdk.org/jdk/pull/17655
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v4]
On Mon, 4 Mar 2024 15:10:12 GMT, Kevin Walls wrote: >> Introduce the jcmd "VM.debug" to implement access to a useful set of the >> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". >> >> Not recommended for live production use. Calling these "debug" utilities, >> and not including them in the jcmd help output, is to remind us they are not >> general customer-facing tools. > > Kevin Walls has updated the pull request incrementally with three additional > commits since the last revision: > > - Usage correction > - Help to clarify this is VM inspection. Comment to relate source to > debug.cpp. > - jcheck trailing whitespace Hi Thomas @tstuefe - Security concerns certainly needed some thought. We have remote access to DiagnosticCommands over JMX. I don't see a particular need for that for this command. I think it was keeping the DCmd flagged as hidden that hides it from the list of dcmds available in that way. So we have the attach api controls using local user identity, as we do for everything else. Yes, "events" is not that useful, I should remove it. I was taking the useful parts of debug.cpp, and although the events were in VM.info, I had missed that since your JDK-8224600, VM.events is its own command now! Thread.print with stacks, vs "VM.debug threads" which is just a thread list, so you have e.g. JavaThread*, name, native id, stack range. "find": Usage at the moment, would likely be MessageBoxOnError, and a native debugger to get the native stacktrace and parameters that include an object reference you care about. It might be other jcmds if e.g. events capture a relevant problematic pointer, but likely it involves a native debugger. Using jcmd behaves better, showing the output where you run jcmd, not in the VM's current output as the debug.cpp utils do. Native debugger syntax to call abritrary functions can be awkward, particularly on Windows, so the jcmd should be a better experience. This might be more compelling in post-mortem usage. Am working on that. i.e. jcmd on a core file. But I am saying it offers some value today. The class/method finders, I've heard some enthusiasm for their inclusion. We don't want to encourage overlap but yes we do have some overlapping jcmds. This is an umbrella but I don't think it's vague. VMDebugDcmd is for inspecting VM state. It's inspired by debug.cpp utilities, does not need to implement all of them, but does aim to make them more accessible (I will assert that they are not well known, which is hard to prove.) Do we have a problem with jcmd feature creep? If anything has crept too far it can be addressed. Looks like the DCmd classes have approximately doubled since jdk8u but this looks like growth in a good way. Thanks Kevin - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1977543469
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v4]
On Mon, 4 Mar 2024 15:10:12 GMT, Kevin Walls wrote: >> Introduce the jcmd "VM.debug" to implement access to a useful set of the >> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". >> >> Not recommended for live production use. Calling these "debug" utilities, >> and not including them in the jcmd help output, is to remind us they are not >> general customer-facing tools. > > Kevin Walls has updated the pull request incrementally with three additional > commits since the last revision: > > - Usage correction > - Help to clarify this is VM inspection. Comment to relate source to > debug.cpp. > - jcheck trailing whitespace Of the sub commands given in the CSR: eventsHotSpot JVM Event log threads Thread list find ADDRESS Describe an address given by the argument findclass CLASS_PATTERN FLAGS findmethod CLASS_PATTERN METHOD_PATTERN FLAGS 1) `events` is redundant (we have VM.events, and events are printed as part of VM.info) 2) `threads` is handled by Thread.info 3) I am still puzzled about `find`. This is to ask a running VM about an arbitrary address. But how do you get such an address? Usually only via debugging. Then I am already attached with a system debugger. Under which circumstances would this be needed? Can you give a usage example? 4) and 5) shared some overlap with VM.classes, VM.class_hierarchy, VM.metaspace and various Compiler.xx commands. I am mostly worried about adding such a low-level debug command to jcmd. Especially in such a vague umbrella form. Experiences show that these commands get extended easily, often without CSR. So its an open door for feature creep. I mostly worry about unforeseen security consequences, especially if jcmd reach is extended across machine boundaries. Weighted against that, the benefits seem a bit slim to me. Again, real-world use cases would help. - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1976941359
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v4]
> Introduce the jcmd "VM.debug" to implement access to a useful set of the > established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". > > Not recommended for live production use. Calling these "debug" utilities, > and not including them in the jcmd help output, is to remind us they are not > general customer-facing tools. Kevin Walls has updated the pull request incrementally with three additional commits since the last revision: - Usage correction - Help to clarify this is VM inspection. Comment to relate source to debug.cpp. - jcheck trailing whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/17655/files - new: https://git.openjdk.org/jdk/pull/17655/files/cacbca27..1d1e20e2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17655=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=02-03 Stats: 7 lines in 3 files changed: 1 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/17655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655 PR: https://git.openjdk.org/jdk/pull/17655
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v4]
On Tue, 27 Feb 2024 01:30:27 GMT, Serguei Spitsyn wrote:
>> Kevin Walls has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - Usage correction
>> - Help to clarify this is VM inspection. Comment to relate source to
>> debug.cpp.
>> - jcheck trailing whitespace
>
> src/hotspot/share/services/diagnosticCommand.cpp line 1200:
>
>> 1198: void VMDebugDCmd::find() {
>> 1199: if (!_arg1.has_value()) {
>> 1200: output()->print_cr("missing argument");
>
> I'm thinking if it would be useful to tell what arguments are expected? This
> is for all cases where the `"missing argument"` message is returned.
Thanks Serguei, have filled in explicit usage instructions when the commands
are missing an argument.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1511313428
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v3]
> Introduce the jcmd "VM.debug" to implement access to a useful set of the > established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". > > Not recommended for live production use. Calling these "debug" utilities, > and not including them in the jcmd help output, is to remind us they are not > general customer-facing tools. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Test update omitted from previous commit. - Changes: - all: https://git.openjdk.org/jdk/pull/17655/files - new: https://git.openjdk.org/jdk/pull/17655/files/f57f7274..cacbca27 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17655=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655 PR: https://git.openjdk.org/jdk/pull/17655
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v2]
On Tue, 27 Feb 2024 01:27:54 GMT, Serguei Spitsyn wrote:
>> Kevin Walls has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 11 additional
>> commits since the last revision:
>>
>> - Merge remote-tracking branch 'upstream/master' into
>> 8318026_jcmd_VMdebug_command
>> - Require UnlockDiagnosticVMOptions
>> - Require
>> UnlockDiagnosticVMOptionstest/hotspot/jtreg/serviceability/dcmd/vm/VMDebugTest.java
>> - Tidy up the safety checks
>> - Whitebox not required, just check option properties.
>> - unnecessary parameter to find
>> - Test update. Recognise ZGC oops differently. Formatting.
>> - typo
>> - Separate is_good_oop... method to avoid changing existing asserts.
>> - More oop safety
>> - ... and 1 more: https://git.openjdk.org/jdk/compare/add19854...f57f7274
>
> src/hotspot/share/services/diagnosticCommand.cpp line 1245:
>
>> 1243: }
>> 1244: } else if (strcmp("find", _subcommand.value()) == 0) {
>> 1245: if (!UnlockDiagnosticVMOptions) {
>
> Would it make sense to require enabling `UnlockDiagnosticVMOptions` for all
> sub-commands, so that it is clear this is not for live production use?
Yes sure I've made that apply to all of them for consistency.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1507894462
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information [v2]
> Introduce the jcmd "VM.debug" to implement access to a useful set of the > established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". > > Not recommended for live production use. Calling these "debug" utilities, > and not including them in the jcmd help output, is to remind us they are not > general customer-facing tools. Kevin Walls has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 11 additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into 8318026_jcmd_VMdebug_command - Require UnlockDiagnosticVMOptions - Require UnlockDiagnosticVMOptionstest/hotspot/jtreg/serviceability/dcmd/vm/VMDebugTest.java - Tidy up the safety checks - Whitebox not required, just check option properties. - unnecessary parameter to find - Test update. Recognise ZGC oops differently. Formatting. - typo - Separate is_good_oop... method to avoid changing existing asserts. - More oop safety - ... and 1 more: https://git.openjdk.org/jdk/compare/add19854...f57f7274 - Changes: - all: https://git.openjdk.org/jdk/pull/17655/files - new: https://git.openjdk.org/jdk/pull/17655/files/9d127e32..f57f7274 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17655=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=00-01 Stats: 58500 lines in 2215 files changed: 25779 ins; 14954 del; 17767 mod Patch: https://git.openjdk.org/jdk/pull/17655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655 PR: https://git.openjdk.org/jdk/pull/17655
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
On Wed, 31 Jan 2024 14:22:44 GMT, Kevin Walls wrote:
> Introduce the jcmd "VM.debug" to implement access to a useful set of the
> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...".
>
> Not recommended for live production use. Calling these "debug" utilities,
> and not including them in the jcmd help output, is to remind us they are not
> general customer-facing tools.
Kevin, thank you for working on this! I've posted several comments/questions.
src/hotspot/share/services/diagnosticCommand.cpp line 1200:
> 1198: void VMDebugDCmd::find() {
> 1199: if (!_arg1.has_value()) {
> 1200: output()->print_cr("missing argument");
I'm thinking if it would be useful to tell what arguments are expected? This is
for all cases where the `"missing argument"` message is returned.
src/hotspot/share/services/diagnosticCommand.cpp line 1245:
> 1243: }
> 1244: } else if (strcmp("find", _subcommand.value()) == 0) {
> 1245: if (!UnlockDiagnosticVMOptions) {
Would it make sense to require enabling `UnlockDiagnosticVMOptions` for all
sub-commands, so that it is clear this is not for live production use?
-
PR Review: https://git.openjdk.org/jdk/pull/17655#pullrequestreview-1902349676
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1503520152
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1503518811
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
On 2/5/24 1:56 AM, Kevin Walls wrote: On Mon, 5 Feb 2024 09:25:24 GMT, Yi Yang wrote: jcmd VM.debug MyDebugCode.java Are you thinking to run some Java code in the target JVM? VM.debug as presented here is all about inspecting JVM state, mostly using existing mechanisms but which were not already exposed in jcmd. I think this could be extended in future if needed. But I think executing arbitrary Java code is quite different and raises more questions. not too mention this is what is already supported with the attach API loadAgent facility, and is being disabled by default for security reasons. - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1926598031
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
On Mon, 5 Feb 2024 09:25:24 GMT, Yi Yang wrote: > jcmd VM.debug MyDebugCode.java Are you thinking to run some Java code in the target JVM? VM.debug as presented here is all about inspecting JVM state, mostly using existing mechanisms but which were not already exposed in jcmd. I think this could be extended in future if needed. But I think executing arbitrary Java code is quite different and raises more questions. - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1926598031
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
On Sun, 4 Feb 2024 23:07:27 GMT, David Holmes wrote: > > not including them in the jcmd help output, is to remind us they are not > > general customer-facing tools. > > Then who are they for? and do they really belong in the `jcmd` tool, or is > that just convenient? > > Without help information who will know what can be done with this tool? I want to suggest that these commands are not for everybody. But people investigating bugs in detail, including JVM crashes, might find these appealing, and should find the jcmd interface convenient. (It is convenient for the user, and also convenient to implement.) Proposing not having them in the standard jcmd help: I don't think most users should be attaching debuggers to their production JVMs, and I don't think most users should be doing these debug operations via jcmd on the same JVM. But we have a variety of expert users who want certain information. But I do think we should find other ways of documenting this, which might include our existing Troubleshooting Guide, but might well be something else. - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1926559303
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
On Wed, 31 Jan 2024 14:22:44 GMT, Kevin Walls wrote: > Introduce the jcmd "VM.debug" to implement access to a useful set of the > established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". > > Not recommended for live production use. Calling these "debug" utilities, > and not including them in the jcmd help output, is to remind us they are not > general customer-facing tools. Could it be extend to something like jcmd VM.debug MyDebugCode.java - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1926547862
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
On Mon, 5 Feb 2024 07:21:05 GMT, Alan Bateman wrote: > ...needs wider review and a CSR should be submitted for this. Sure no problem I will refresh the linked CSR. - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1926513588
RE: RFR: 8318026: jcmd should provide access to low-level JVM debug information
Hi Thomas - Yes, it goes into all builds, and yes this is a new jcmd with sub-commands, you could call it a group. It's not a large group of commands, and the commands themselves are not exactly new (but no objection to a CSR). Getting these debug utilities into a jcmd is about making them more discoverable and accessible (discoverability with bounds, still aimed at a narrow set of technical users), and also more testable. This does not compete with or replace existing debuggers if that's anybody's chosen route. I envisage just adding to the toolkit we have available. Live or at the time of a fault (e.g. MessageBoxOnError) we can inspect JVM state using existing jcmds. Now we can also inspect arbitrary Java heap objects, and items in other VM locations. This may be in parallel with a debugger. You can't run these while the JVM is stopped in another debugger 8-) but you may well attach something temporarily and see references that the JVM itself is well-placed to decode. Where VM.info or Thread.print give you Java object references, "VM.debug find" lets you inspect them, and continue following references with additional invocations. > many folks are at fosdem right now. I hope they have a great time. 8-) Thanks Kevin From: Thomas Stüfe Sent: 02 February 2024 21:42 To: Kevin Walls Cc: hotspot-runtime-...@openjdk.org; serviceability-dev@openjdk.org Subject: Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information Hi Kevin, Having a clear command spec to read and argue about would be helpful, especially since this is not a simple commnd but a whole command group. Exposing such a low level interface (this is supposed to go into product VMs, right?) may carry some risks that could arguably fall unter CSR. That said, what use case do you envisage? To me, these commands are usually only useful in the debugger, when I deal with numerical adresses. Cheers, Thomas p.s. please note that many folks are at fosdem right now. On Fri 2. Feb 2024 at 19:35, Kevin Walls mailto:kev...@openjdk.org>> wrote: Introduce the jcmd "VM.debug" to implement access to a useful set of the established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". Not recommended for live production use. Calling these "debug" utilities, and not including them in the jcmd help output, is to remind us they are not general customer-facing tools. - Commit messages: - Tidy up the safety checks - Whitebox not required, just check option properties. - unnecessary parameter to find - Test update. Recognise ZGC oops differently. Formatting. - typo - Separate is_good_oop... method to avoid changing existing asserts. - More oop safety - 8318026: jcmd should provide access to low-level JVM debug information Changes: https://git.openjdk.org/jdk/pull/17655/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17655=00 Issue: https://bugs.openjdk.org/browse/JDK-8318026 Stats: 371 lines in 5 files changed: 369 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655 PR: https://git.openjdk.org/jdk/pull/17655
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
On Wed, 31 Jan 2024 14:22:44 GMT, Kevin Walls wrote:
> Introduce the jcmd "VM.debug" to implement access to a useful set of the
> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...".
>
> Not recommended for live production use. Calling these "debug" utilities,
> and not including them in the jcmd help output, is to remind us they are not
> general customer-facing tools.
This proposal effectively opens a new side channel for debugging with several
commands ("events", "find", "findclass", ...). This will surely end up in
troubleshooting recipes and other documentation. For now, the commands look
benign but it may be tempting for people to show up with changes that extend
this for other things. So I think this feature needs wider review and a CSR
should be submitted for this.
-
PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1926365013
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
On Wed, 31 Jan 2024 14:22:44 GMT, Kevin Walls wrote: > not including them in the jcmd help output, is to remind us they are not > general customer-facing tools. Then who are they for? and do they really belong in the `jcmd` tool, or is that just convenient? Without help information who will know what can be done with this tool? - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1925959472
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
Hi Kevin, Having a clear command spec to read and argue about would be helpful, especially since this is not a simple commnd but a whole command group. Exposing such a low level interface (this is supposed to go into product VMs, right?) may carry some risks that could arguably fall unter CSR. That said, what use case do you envisage? To me, these commands are usually only useful in the debugger, when I deal with numerical adresses. Cheers, Thomas p.s. please note that many folks are at fosdem right now. On Fri 2. Feb 2024 at 19:35, Kevin Walls wrote: > Introduce the jcmd "VM.debug" to implement access to a useful set of the > established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". > > Not recommended for live production use. Calling these "debug" utilities, > and not including them in the jcmd help output, is to remind us they are > not general customer-facing tools. > > - > > Commit messages: > - Tidy up the safety checks > - Whitebox not required, just check option properties. > - unnecessary parameter to find > - Test update. Recognise ZGC oops differently. Formatting. > - typo > - Separate is_good_oop... method to avoid changing existing asserts. > - More oop safety > - 8318026: jcmd should provide access to low-level JVM debug information > > Changes: https://git.openjdk.org/jdk/pull/17655/files > Webrev: https://webrevs.openjdk.org/?repo=jdk=17655=00 > Issue: https://bugs.openjdk.org/browse/JDK-8318026 > Stats: 371 lines in 5 files changed: 369 ins; 0 del; 2 mod > Patch: https://git.openjdk.org/jdk/pull/17655.diff > Fetch: git fetch https://git.openjdk.org/jdk.git > pull/17655/head:pull/17655 > > PR: https://git.openjdk.org/jdk/pull/17655 >
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
On Wed, 31 Jan 2024 14:22:44 GMT, Kevin Walls wrote:
> Introduce the jcmd "VM.debug" to implement access to a useful set of the
> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...".
>
> Not recommended for live production use. Calling these "debug" utilities,
> and not including them in the jcmd help output, is to remind us they are not
> general customer-facing tools.
test/hotspot/jtreg/serviceability/dcmd/vm/VMDebugTest.java line 178:
> 176: String p = m.group(regexGroup);
> 177: ptr = new BigInteger(p, 16);
> 178: System.out.println("Found '" + pattern +"', using
> pointer: 0x" + ptr.toString(16));
Would it be more logical to say this?
"Found pointer: 0x" + ptr.toString(16)'" + " for pattern " +pattern
test/hotspot/jtreg/serviceability/dcmd/vm/VMDebugTest.java line 201:
> 199: run(new JMXExecutor());
> 200: }
> 201:*/
comment to remove
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1474686608
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1474688481
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
On Thu, 1 Feb 2024 15:47:24 GMT, Ludvig Janiuk wrote: >> Introduce the jcmd "VM.debug" to implement access to a useful set of the >> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". >> >> Not recommended for live production use. Calling these "debug" utilities, >> and not including them in the jcmd help output, is to remind us they are not >> general customer-facing tools. > > test/hotspot/jtreg/serviceability/dcmd/vm/VMDebugTest.java line 201: > >> 199: run(new JMXExecutor()); >> 200: } >> 201:*/ > > comment to remove plus some formatting below in this file - PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1474688937
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
On Wed, 31 Jan 2024 14:22:44 GMT, Kevin Walls wrote: > Introduce the jcmd "VM.debug" to implement access to a useful set of the > established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". > > Not recommended for live production use. Calling these "debug" utilities, > and not including them in the jcmd help output, is to remind us they are not > general customer-facing tools. (I started a CSR but have withdraw it, as it is established that jcmd is a diagnostic tool, and does not require a CSR for new options, similar to HotSpot diagnostic flags). Adding jcmd access to debug tools, for a live process. That includes a process started with -XX:+ShowMessageBoxOnError which has stopped with an error. At that point, the VM has not created the hs_err fatal error log, but we can already run "jcmd PID VM.info", to provide much of what would appear in the hs_err log, and a native debugger can be attached to discover the call stack. The "find" subcommand reads an address from the text of its parameter, to lookup pointers found in VM.info output or in gdb sessions. It is protected by the UnlockDiagnosticVMOptions option (so is always enabled in debug builds). With the dbg_is_good_oop changes here, I can examine hundreds of pointers around a known good oop, recognise bad objects as such and skip them, without crashing. Before the additions, it was possible to crash the target, e.g. following a null klass pointer when looking at an address that is not an object. The "not recommended for live production use" advice still stands, and diagnostic option requirements remain, in case a crash is possible when the find command is given a pointer to the right (or wrong) kind of data in the Java heap. Aimed at people with source code access/familiarity, hence not documenting the findclass/findmethod flags in great detail. Commands and output should be expected to evolve. Thanks @LudwikJaniuk for the nits above, and for the other testing you did with this. - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1921013935 PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1923692015
