Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v9]
On Tue, 26 Mar 2024 21:55:38 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 incrementally with one additional
> commit since the last revision:
>
> Undo include
It looks like the test only inspects a thread and a java object. Perhaps you
could add tests for additional VM objects. Maybe grab a frame PC from a thread
stack trace.
src/hotspot/share/services/diagnosticCommand.hpp line 590:
> 588: }
> 589: static const char* description() {
> 590: return "Inspect VM object at address.";
Maybe distinguishing between java heap objects and VM objects would be useful
to the reader/user.
src/hotspot/share/utilities/debug.cpp line 679:
> 677: }
> 678:
> 679: // Additional "good oop" checks, separate method to not disturb existing
> asserts.
Suggestion:
// Additional "good oop" checks. Separate method to not disturb existing
asserts.
src/hotspot/share/utilities/debug.cpp line 680:
> 678:
> 679: // Additional "good oop" checks, separate method to not disturb existing
> asserts.
> 680: extern "C" bool dbg_is_good_oop_detailed(oopDesc* o) {
What was the motivation for adding this? Is this copied from other HS code? How
do we know it's doing a good enough job of verification? What happens if
verification is not thorough enough?
test/hotspot/jtreg/serviceability/dcmd/vm/VMInspectTest.java line 83:
> 81: }
> 82:
> 83: // Tests where enabled:
Suggestion:
// Tests run with VM.inspect support enabled:
-
PR Review: https://git.openjdk.org/jdk/pull/17655#pullrequestreview-1962319603
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1540483489
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1540485389
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1540486518
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1540492385
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v3]
On Wed, 27 Mar 2024 02:57:40 GMT, Serguei Spitsyn wrote:
>> This PR fixes a synchronization issue in the test:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest`
>>
>> The method `notifyAtBreakpoint()` can notify the `TestTask` thread when it
>> has not reached an expected breakpoint yet.
>> The fix is to add a call to the method `ensureAtBreakpoint()` one more time
>> in the `B2` sub-test. It is needed after the top-most frame was popped with
>> the JVMTI `PopFrame`, and the target thread needs to reach the breakpoint
>> again after its execution was resumed.
>>
>> The time is very intermittent. At least, I was not able to reproduce the
>> timeout failure in thousands of mach5 runs with the `-Xcomp` option.
>>
>> Testing:
>> - Run the test
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest` thousands
>> times in mach5
>
> Serguei Spitsyn 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 three additional
> commits since the last revision:
>
> - “Merge”
> - review: updated test with one more call to notifyAtBreakpoint to reset the
> native state
> - 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout
test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/PopFrameTest.java
line 147:
> 145: }
> 146: log("Main #B.2: got expected JVMTI_ERROR_NONE");
> 147: notifyAtBreakpoint(); // needed to reset the native state
This doesn't make much sense to me. The purpose of `notifyAtBreakpoint()` is
to unblock the Breakpoint event handler, which is waiting for this notify.
However, the event handler can only ever be entered once, yet we have a
previous `noitfyBreakpoint()` above and a 3rd `notifyAtBreakpoint()` below. It
seems that this call and the one below are no-ops. I don't see how
bp_sync_reached ever gets set true again after the first `notifyAtBreakpoint()`
call is made.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1540476191
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v3]
> This PR fixes a synchronization issue in the test: > `test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest` > > The method `notifyAtBreakpoint()` can notify the `TestTask` thread when it > has not reached an expected breakpoint yet. > The fix is to add a call to the method `ensureAtBreakpoint()` one more time > in the `B2` sub-test. It is needed after the top-most frame was popped with > the JVMTI `PopFrame`, and the target thread needs to reach the breakpoint > again after its execution was resumed. > > The time is very intermittent. At least, I was not able to reproduce the > timeout failure in thousands of mach5 runs with the `-Xcomp` option. > > Testing: > - Run the test > `test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest` thousands > times in mach5 Serguei Spitsyn 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 three additional commits since the last revision: - “Merge” - review: updated test with one more call to notifyAtBreakpoint to reset the native state - 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout - Changes: - all: https://git.openjdk.org/jdk/pull/18419/files - new: https://git.openjdk.org/jdk/pull/18419/files/7843111d..1502492f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18419=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18419=01-02 Stats: 61204 lines in 2409 files changed: 11744 ins; 7474 del; 41986 mod Patch: https://git.openjdk.org/jdk/pull/18419.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18419/head:pull/18419 PR: https://git.openjdk.org/jdk/pull/18419
Integrated: JDK-8315575: Retransform of record class with record component annotation fails with CFE
On Fri, 8 Mar 2024 02:54:49 GMT, Alex Menkov wrote: > RecordComponent class has _attributes_count field. > The only user of the field is JvmtiClassFileReconstituter. Incorrect value of > the field causes producing incorrect data for Record attribute. > Parsing Record attribute ClassFileParser skips unknown attributes and may > skip RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations. > Also annotations can be changed (added/removed) by class redefinition. > The fix removes attributes_count from RecordComponent; > JvmtiClassFileReconstituter calculates correct attributes_count generating > class bytes. > > Testing: > - tier1,tier2,hs-tier5-svc; > - redefineClasses/retransformClasses tests: >- test/jdk/java/lang/instrument >- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses >- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses >- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses This pull request has now been integrated. Changeset: 8fc9097b Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/8fc9097b3720314ef7efaf1f3ac31898c8d6ca19 Stats: 208 lines in 5 files changed: 190 ins; 10 del; 8 mod 8315575: Retransform of record class with record component annotation fails with CFE Reviewed-by: sspitsyn, coleenp - PR: https://git.openjdk.org/jdk/pull/18161
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v9]
On Tue, 26 Mar 2024 21:55:38 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 incrementally with one additional
> commit since the last revision:
>
> Undo include
test/hotspot/jtreg/serviceability/dcmd/vm/VMInspectTest.java line 123:
> 121: ptr = findPointer(threadPrintOutput, waiting_on_mylock, 1);
> 122: output = executor.execute("VM.inspect " + pointerText(ptr));
> 123: System.out.println(output);
Nit: May I ask you to add empty lines after the lines 101 and 123 to make the
code more readable?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1540205562
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v9]
On Tue, 26 Mar 2024 21:55:38 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 incrementally with one additional > commit since the last revision: > > Undo include test/hotspot/jtreg/serviceability/dcmd/vm/VMInspectTest.java line 217: > 215: t.start(); > 216: } > 217: } Nit: The indents in the `work()` method has to be 4, not 2. - PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1540202705
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v8]
On Tue, 26 Mar 2024 20:52:30 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 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/91a3bbb4...739bcbfa > > 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? Yes, undoing that, thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1540153524
Re: RFR: 8318026: jcmd should provide access to detailed JVM object information [v9]
> 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 incrementally with one additional commit since the last revision: Undo include - Changes: - all: https://git.openjdk.org/jdk/pull/17655/files - new: https://git.openjdk.org/jdk/pull/17655/files/739bcbfa..bf5e4b26 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17655=08 - incr: https://webrevs.openjdk.org/?repo=jdk=17655=07-08 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 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: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v9]
On Tue, 26 Mar 2024 13:57:55 GMT, Matthias Baesken wrote: >> Currently jcmd command GC.heap_dump only works with an additionally provided >> file name. >> Syntax : GC.heap_dump [options] >> >> In case the JVM has the XX - flag HeapDumpPath set, we should support an >> additional mode where the is optional. >> In case the filename is NOT set, we take the HeapDumpPath (file or >> directory); >> >> new syntax : >> GC.heap_dump [options] .. has precedence over second option >> GC.heap_dump [options] …in case -XX: HeapDumpPath=p is set >> >> This would be a simplification e.g. for support cases where a filename or >> directory is set at JVM startup with -XX: HeapDumpPath=p and writing to the >> path is intended/recommended for usage also in the jcmd case. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > suggestions from Chris for diagnosticCommand.cpp We have a couple of additional documents that need to be updated: https://docs.oracle.com/en/java/javase/21/troubleshoot/command-line-options1.html#GUID-A84ECBFB-B6CF-44C3-B627-58BB509C8D05 This is an Oracle produced document. A `docs` CR will need to be filed to get it updated (and I see it is already appears out-of-date w.r.t. `HeapDumpGzipLevel`) src/java.base/share/man/java.1 Much like jcmd.1, this one is also derived from a closed markdown file, so eventually someone internal at Oracle will need to update the markdown file, but we need to see the proposed changes in the java.1 file first. This document is also missing the `HeapDumpGzipLevel` update. If your counting, there are 7 places where documentation will need updating 1. Oracle trouble shooting guide 2. open java.1 3. closed markdown file that java.1 is generated from 4. open jcmd.1 5. closed markdown file that jcmd.1 is generated from 6. globals.hpp 7. jcmd help output - PR Comment: https://git.openjdk.org/jdk/pull/18190#issuecomment-2021523855
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: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v7]
On Tue, 26 Mar 2024 19:10:11 GMT, Coleen Phillimore wrote: >> Alex Menkov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> added comment > > test/jdk/java/lang/instrument/RetransformRecordAnnotation.java line 30: > >> 28: * >> 29: * @library /test/lib >> 30: * @run shell MakeJAR.sh retransformAgent > > Does the presence of attributes mean you can't use the simpler > RedefineClassHelper that the tests in > test/hotspot/jtreg/serviceability/jvmti/RedefineClasses use? RedefineClassHelper supports only class redefinition, the test needs class retransformation. - PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1540044735
Re: RFR: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature [v15]
> The deprecated Subject Delegation feature in JMX will be removed. > > This was marked in JDK 21 as deprecated for removal (JDK-8298966). Kevin Walls has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 24 commits: - Merge - Missing code doc nit. - Missing code doc nit. - RMIConnectionImpl_Stub also should explicity inherit the unchecked UOE. - Clarify JMXConnector equivalence comment. - RMIConnectionImpl needs to explicity inherit the unchecked UOE. - typo - Javadoc update - Remove unnecessary constructor in private class - Merge remote-tracking branch 'upstream/master' into 832_SubjectDelegation_remove - ... and 14 more: https://git.openjdk.org/jdk/compare/cc1800fa...903ce55b - Changes: https://git.openjdk.org/jdk/pull/18025/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18025=14 Stats: 2026 lines in 35 files changed: 214 ins; 1632 del; 180 mod Patch: https://git.openjdk.org/jdk/pull/18025.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18025/head:pull/18025 PR: https://git.openjdk.org/jdk/pull/18025
Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v3]
On Thu, 14 Mar 2024 22:09:55 GMT, Alex Menkov wrote: >> It does not matter what the `ClassFileParser` does. >> >>> ` JvmtiClassFileReconstituter` performs the reverse operation. >> >> True. It should know how to put the `attribute_count` value into the class >> file but it does not need to know how to calculate its value. What I do not >> like in your model is that there is no one single place which knows how to >> calculate this value and the existing and potential consumers should have >> this knowledge. >> >>> They may have different `attributute_count `(that's what this bug about). >> >> The bug is that the `attributute_count` field value from the class file >> stored in the `RecordComponent` does not match the calculated value because >> the invisible attribute was not saved (was ignored). My suggestion is not to >> store the `attributute_count` field value from the class file as it was >> before. The suggestion is to place the calculating function >> `attributute_count()` into the `RecordComponent` class. > >> True. It should know how to put the `attribute_count` value into the class >> file but it does not need to know how to calculate its value. What I do not >> like in your model is that there is no one single place which knows how to >> calculate this value and the existing and potential consumers should have >> this knowledge. > > It **must** know how to calculate the value. Because this is number of > `attribute_info_attributes` attributes that follow. And only > `JvmtiClassFileReconstituter` knows how many attributes it's going to write. > >> The bug is that the `attributute_count` field value from the class file >> stored in the `RecordComponent` does not match the calculated value because >> the invisible attribute was not saved (was ignored). > > I'd consider this as design issue with the current implementation - > `JvmtiClassFileReconstituter` gets the value from external source > (`RecordComponent`) and uses it without validation. > This approach is inconsistent with other `JvmtiClassFileReconstituter` code. > For other similar cases it calculates record counts in place (from size of > arrays, string length, from list enumerator, etc.). So this makes sense. You only want the number of attributes that are applicable to the RecordComponent, and that is the three attributes that you then write out in the class file reconstituter: generic_signature_index, annotations and type_annotations. - PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1539950465
Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v7]
On Thu, 21 Mar 2024 18:10:49 GMT, Alex Menkov wrote: >> RecordComponent class has _attributes_count field. >> The only user of the field is JvmtiClassFileReconstituter. Incorrect value >> of the field causes producing incorrect data for Record attribute. >> Parsing Record attribute ClassFileParser skips unknown attributes and may >> skip RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations. >> Also annotations can be changed (added/removed) by class redefinition. >> The fix removes attributes_count from RecordComponent; >> JvmtiClassFileReconstituter calculates correct attributes_count generating >> class bytes. >> >> Testing: >> - tier1,tier2,hs-tier5-svc; >> - redefineClasses/retransformClasses tests: >>- test/jdk/java/lang/instrument >>- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses >>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses >>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > added comment This looks good. test/jdk/java/lang/instrument/RetransformRecordAnnotation.java line 30: > 28: * > 29: * @library /test/lib > 30: * @run shell MakeJAR.sh retransformAgent Does the presence of attributes mean you can't use the simpler RedefineClassHelper that the tests in test/hotspot/jtreg/serviceability/jvmti/RedefineClasses use? - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18161#pullrequestreview-1961478084 PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1539953736
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v4]
> This task addresses an essential aspect of our testing infrastructure: the > proper handling and cleanup of temporary files and socket files created > during test execution. The motivation behind these changes is to prevent the > accumulation of unnecessary files in the default temporary directory, which > can affect the system's storage and potentially influence subsequent test > runs. > > Our review identified that several tests create temporary files or socket > files without ensuring their removal post-execution. > - Direct calls to java.io.File.createTempFile and > java.nio.file.Files.createTempFile without adequate cleanup. > - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not > explicitly removing socket files post-use. Bill Huang has updated the pull request incrementally with one additional commit since the last revision: Update test/jdk/java/nio/channels/unixdomain/Bind.java Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/18352/files - new: https://git.openjdk.org/jdk/pull/18352/files/2517f756..0f4130a9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18352=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18352=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18352.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18352/head:pull/18352 PR: https://git.openjdk.org/jdk/pull/18352
Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v7]
On Thu, 21 Mar 2024 18:10:49 GMT, Alex Menkov wrote: >> RecordComponent class has _attributes_count field. >> The only user of the field is JvmtiClassFileReconstituter. Incorrect value >> of the field causes producing incorrect data for Record attribute. >> Parsing Record attribute ClassFileParser skips unknown attributes and may >> skip RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations. >> Also annotations can be changed (added/removed) by class redefinition. >> The fix removes attributes_count from RecordComponent; >> JvmtiClassFileReconstituter calculates correct attributes_count generating >> class bytes. >> >> Testing: >> - tier1,tier2,hs-tier5-svc; >> - redefineClasses/retransformClasses tests: >>- test/jdk/java/lang/instrument >>- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses >>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses >>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > added comment Can I get 2nd review plase - PR Comment: https://git.openjdk.org/jdk/pull/18161#issuecomment-2021144086
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
Integrated: JDK-8328930: [AIX] remove pase related coding
On Mon, 25 Mar 2024 13:26:12 GMT, Matthias Baesken wrote: > We still have quite a lot of OS400 / pase related coding in the hotspot AIX > codebase. But this does not work and was never supported in OpenJDK. So we > can remove this. This pull request has now been integrated. Changeset: 153410f4 Author:Matthias Baesken URL: https://git.openjdk.org/jdk/commit/153410f480c372604e5825bfcd3a63f137e6a013 Stats: 416 lines in 8 files changed: 9 ins; 363 del; 44 mod 8328930: [AIX] remove pase related coding Reviewed-by: clanger, lucy - PR: https://git.openjdk.org/jdk/pull/18471
Re: RFR: JDK-8328930: [AIX] remove pase related coding [v2]
On Tue, 26 Mar 2024 14:18:34 GMT, Matthias Baesken wrote: >> We still have quite a lot of OS400 / pase related coding in the hotspot AIX >> codebase. But this does not work and was never supported in OpenJDK. So we >> can remove this. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust/remove some comments Hi Lutz and Christoph, thanks for the reviews ! - PR Comment: https://git.openjdk.org/jdk/pull/18471#issuecomment-2020898141
Re: RFR: JDK-8328930: [AIX] remove pase related coding [v2]
On Tue, 26 Mar 2024 14:18:34 GMT, Matthias Baesken wrote: >> We still have quite a lot of OS400 / pase related coding in the hotspot AIX >> codebase. But this does not work and was never supported in OpenJDK. So we >> can remove this. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust/remove some comments Looks good. - Marked as reviewed by lucy (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18471#pullrequestreview-1960988506
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v3]
On Thu, 21 Mar 2024 17:13:46 GMT, Bill Huang wrote: >> This task addresses an essential aspect of our testing infrastructure: the >> proper handling and cleanup of temporary files and socket files created >> during test execution. The motivation behind these changes is to prevent the >> accumulation of unnecessary files in the default temporary directory, which >> can affect the system's storage and potentially influence subsequent test >> runs. >> >> Our review identified that several tests create temporary files or socket >> files without ensuring their removal post-execution. >> - Direct calls to java.io.File.createTempFile and >> java.nio.file.Files.createTempFile without adequate cleanup. >> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not >> explicitly removing socket files post-use. > > Bill Huang has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed potential NPE issues. unixdomain NIO test changes look fine to me - Marked as reviewed by michaelm (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18352#pullrequestreview-1960889711
Re: RFR: JDK-8328930: [AIX] remove pase related coding [v2]
On Tue, 26 Mar 2024 14:18:34 GMT, Matthias Baesken wrote: >> We still have quite a lot of OS400 / pase related coding in the hotspot AIX >> codebase. But this does not work and was never supported in OpenJDK. So we >> can remove this. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust/remove some comments Marked as reviewed by clanger (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18471#pullrequestreview-1960822263
Re: RFR: JDK-8328930: [AIX] remove pase related coding
On Mon, 25 Mar 2024 13:26:12 GMT, Matthias Baesken wrote: > We still have quite a lot of OS400 / pase related coding in the hotspot AIX > codebase. But this does not work and was never supported in OpenJDK. So we > can remove this. Hi Christoph, I adjusted/removed the comments. - PR Comment: https://git.openjdk.org/jdk/pull/18471#issuecomment-2020550254
Re: RFR: JDK-8328930: [AIX] remove pase related coding [v2]
> We still have quite a lot of OS400 / pase related coding in the hotspot AIX > codebase. But this does not work and was never supported in OpenJDK. So we > can remove this. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: adjust/remove some comments - Changes: - all: https://git.openjdk.org/jdk/pull/18471/files - new: https://git.openjdk.org/jdk/pull/18471/files/d1c4c7de..ba02f086 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18471=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18471=00-01 Stats: 10 lines in 1 file changed: 0 ins; 9 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18471.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18471/head:pull/18471 PR: https://git.openjdk.org/jdk/pull/18471
Re: RFR: JDK-8328930: [AIX] remove pase related coding
On Tue, 26 Mar 2024 09:27:01 GMT, Christoph Langer wrote: >> We still have quite a lot of OS400 / pase related coding in the hotspot AIX >> codebase. But this does not work and was never supported in OpenJDK. So we >> can remove this. > > src/hotspot/os/aix/os_aix.cpp line 626: > >> 624: >> 625: // excerpt from >> 626: // http://publib.boulder.ibm.com/infocenter/systems/index.jsp > > Same here. URL on one line. Does it still work? The link is dead, it does not work any more. I think we can remove it. - PR Review Comment: https://git.openjdk.org/jdk/pull/18471#discussion_r1539288321
Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v9]
> Currently jcmd command GC.heap_dump only works with an additionally provided > file name. > Syntax : GC.heap_dump [options] > > In case the JVM has the XX - flag HeapDumpPath set, we should support an > additional mode where the is optional. > In case the filename is NOT set, we take the HeapDumpPath (file or directory); > > new syntax : > GC.heap_dump [options] .. has precedence over second option > GC.heap_dump [options] …in case -XX: HeapDumpPath=p is set > > This would be a simplification e.g. for support cases where a filename or > directory is set at JVM startup with -XX: HeapDumpPath=p and writing to the > path is intended/recommended for usage also in the jcmd case. Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: suggestions from Chris for diagnosticCommand.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/18190/files - new: https://git.openjdk.org/jdk/pull/18190/files/8b48c911..b414b1be Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18190=08 - incr: https://webrevs.openjdk.org/?repo=jdk=18190=07-08 Stats: 12 lines in 1 file changed: 0 ins; 3 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/18190.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18190/head:pull/18190 PR: https://git.openjdk.org/jdk/pull/18190
Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v8]
On Fri, 15 Mar 2024 11:24:53 GMT, Matthias Baesken wrote: >> Currently jcmd command GC.heap_dump only works with an additionally provided >> file name. >> Syntax : GC.heap_dump [options] >> >> In case the JVM has the XX - flag HeapDumpPath set, we should support an >> additional mode where the is optional. >> In case the filename is NOT set, we take the HeapDumpPath (file or >> directory); >> >> new syntax : >> GC.heap_dump [options] .. has precedence over second option >> GC.heap_dump [options] …in case -XX: HeapDumpPath=p is set >> >> This would be a simplification e.g. for support cases where a filename or >> directory is set at JVM startup with -XX: HeapDumpPath=p and writing to the >> path is intended/recommended for usage also in the jcmd case. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Adjust jcmd manpage, help and globals comment Hi Chris, thanks for the suggestions , I added them to the latest commit. - PR Comment: https://git.openjdk.org/jdk/pull/18190#issuecomment-2020494383
Re: RFR: JDK-8328930: [AIX] remove pase related coding
On Mon, 25 Mar 2024 13:26:12 GMT, Matthias Baesken wrote: > We still have quite a lot of OS400 / pase related coding in the hotspot AIX > codebase. But this does not work and was never supported in OpenJDK. So we > can remove this. Welcome cleanup. I made two minor comments. src/hotspot/os/aix/os_aix.cpp line 607: > 605: memset(pmi, 0, sizeof(meminfo_t)); > 606: > 607: // On AIX, I use the (dynamically loaded) perfstat library to retrieve > memory statistics maybe remove the "I" pronoun in the comment here? Also the links could be placed on one line. src/hotspot/os/aix/os_aix.cpp line 626: > 624: > 625: // excerpt from > 626: // http://publib.boulder.ibm.com/infocenter/systems/index.jsp Same here. URL on one line. Does it still work? - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18471#pullrequestreview-1959796540 PR Review Comment: https://git.openjdk.org/jdk/pull/18471#discussion_r1538868553 PR Review Comment: https://git.openjdk.org/jdk/pull/18471#discussion_r1538869884
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v3]
On Thu, 21 Mar 2024 17:13:46 GMT, Bill Huang wrote:
>> This task addresses an essential aspect of our testing infrastructure: the
>> proper handling and cleanup of temporary files and socket files created
>> during test execution. The motivation behind these changes is to prevent the
>> accumulation of unnecessary files in the default temporary directory, which
>> can affect the system's storage and potentially influence subsequent test
>> runs.
>>
>> Our review identified that several tests create temporary files or socket
>> files without ensuring their removal post-execution.
>> - Direct calls to java.io.File.createTempFile and
>> java.nio.file.Files.createTempFile without adequate cleanup.
>> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not
>> explicitly removing socket files post-use.
>
> Bill Huang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed potential NPE issues.
test/jdk/java/nio/channels/unixdomain/Bind.java line 162:
> 160: // address with space should work
> 161: checkNormal(() -> {
> 162: UnixDomainSocketAddress usa =
> UnixDomainSocketAddress.of("with space");
Suggestion:
UnixDomainSocketAddress usa = UnixDomainSocketAddress.of("with
space");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18352#discussion_r1538701870
