Re: RFR: 8322811: jcmd System.dump_map help info has conflicting statements
On Tue, 11 Jun 2024 07:45:29 GMT, Kevin Walls wrote:
>> src/hotspot/share/services/diagnosticCommand.cpp line 1211:
>>
>>> 1209: } else {
>>> 1210: name = _filename.value();
>>> 1211: }
>>
>> This code should be considering if a default it specified or not, in case
>> the specified value is identical to what is contained in `default_filename`.
>> With the current solution, if the user literally specifies
>> vm_memory_map_.txt, the part will be expanded to the actual pid.
>> See how [JDK-8323546](https://bugs.openjdk.org/browse/JDK-8323546) handled
>> this.
>
> Oops yes should definitely be using _filename.is_set() rather than strcmp.
I filed [JDK-8334164](https://bugs.openjdk.org/browse/JDK-8334164) for not
using `_filename.is_set()` and
[JDK-8334145](https://bugs.openjdk.org/browse/JDK-8334145) for the issue with
missing from the default filename in the jcmd.l.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19596#discussion_r1637173696
Re: RFR: 8327793: Deprecate jstatd for removal [v4]
On Tue, 11 Jun 2024 18:11:55 GMT, Kevin Walls wrote: >> jstatd is an RMI server application which monitors HotSpot VMs, and provides >> an interface to the monitoring tool jstat, for use across a remote RMI >> connection. >> >> RMI is not how modern applications communicate. It is an old transport with >> long term security concerns, and configuration difficulties with firewalls. >> >> The jstatd tool should be removed. Deprecating and removing jstatd will not >> affect usage of jstat for monitoring local VMs using the Attach API. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Remove annotations Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19658#pullrequestreview-2111433619
Re: RFR: 8330534: Update nsk/jdwp tests to use driver instead of othervm [v4]
On Tue, 11 Jun 2024 18:48:41 GMT, Leonid Mesnik wrote: >> The jdwp tests use debugger and debugee. There is no goal to execute >> debugger part with all VM flags, they are needed to be used with debugee VM >> only. >> The change is all tests is to don't use System.exit() and use 'driver' >> instead of othervm. >> test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeBinder.java >> is updated to correctly set classpath for debugee > > Leonid Mesnik 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 five additional > commits since the last revision: > > - Merge branch 'master' of https://github.com/openjdk/jdk into 8330534 > - reverted jdi > - jdi cleanup. > - identation updated. > - 8330534: Update nsk/jdwp tests to use driver instead of othervm Looks good. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18826#pullrequestreview-2111403784
Re: RFR: 8333827: JDK 23 RDP1 L10n resource files update [v2]
On Mon, 10 Jun 2024 21:58:29 GMT, Damon Nguyen wrote: >> This issue is responsible for updating the translations of all the >> localize(able) resources in the JDK. Primarily, the changes between JDK 22 >> RDP 1 and the integration of the JDK 23 RDP 1 L10n drop will be translated. >> >> The translation tool adjusted some definitions, which causes some changes in >> localized files where the source file had no changes. This causes some words >> being reverted from localized languages to English, and some had its >> definitions changed. >> >> Alternatively, the diffs are viewable here and was generated using Jonathan >> Gibbons' diff tool for l10n: >> https://cr.openjdk.org/~dnguyen/output2/ > > Damon Nguyen has updated the pull request incrementally with one additional > commit since the last revision: > > Remove double quotes jdk.jconsole and jdk.jdi changes look fine. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19609#pullrequestreview-208012
Re: RFR: 8333730: ubsan: FieldIndices/libFieldIndicesTest.cpp:276:11: runtime error: null pointer passed as argument 2, which is declared to never be null
On Tue, 11 Jun 2024 13:00:44 GMT, Matthias Baesken wrote:
> When running HS :tier1 tests with ubsan-enabled binaries, the following issue
> is reported :
> test
> serviceability/jvmti/FollowReferences/FieldIndices/FieldIndicesTest.jtr
>
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp:276:11:
> runtime error: null pointer passed as argument 2, which is declared to never
> be null
> #0 0x7efea442e379 in Klass::explore_interfaces(JNIEnv_*)
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp:276
> #1 0x7efea443280d in Klass::explore(JNIEnv_*, _jclass*)
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp:322
> #2 0x7efea4433adf in Object::explore(JNIEnv_*, _jobject*)
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp:349
> #3 0x7efea443443b in Java_FieldIndicesTest_prepare
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp:489
> #4 0x7efe7f8d1e7b ()
Marked as reviewed by cjplummer (Reviewer).
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 278:
> 276: if (super_klass->interfaces != nullptr) {
> 277: memcpy(interfaces, super_klass->interfaces, sizeof(Klass*) *
> super_klass->interface_count);
> 278: }
Looks good. I suppose the other approach would be to check if `interface_count
== 0`. I'm not sure there is a reason to prefer one solution over the other.
-
PR Review: https://git.openjdk.org/jdk/pull/19657#pullrequestreview-2111059926
PR Review Comment: https://git.openjdk.org/jdk/pull/19657#discussion_r1635247789
Re: RFR: 8330534: Update nsk/jdwp tests to use driver instead of othervm [v2]
On Sun, 5 May 2024 18:07:47 GMT, Leonid Mesnik wrote: >> The jdwp tests use debugger and debugee. There is no goal to execute >> debugger part with all VM flags, they are needed to be used with debugee VM >> only. >> The change is all tests is to don't use System.exit() and use 'driver' >> instead of othervm. >> test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeBinder.java >> is updated to correctly set classpath for debugee > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > identation updated. There is something wrong with this diff. The original commit showed a lot of diffs to change main/othervm to driver (the main purpose of this PR), but now I don't see any of those changes and instead for the most part all I see is the very minor change to add a space in the `run()` call arguments. I think for some reason only your 2nd commit is being applied to this PR and the first commit was lost. However, the loss of the first commit is not showing up in the incremental diff of the two commits. I'm not sure what's going on. - PR Comment: https://git.openjdk.org/jdk/pull/18826#issuecomment-2161262911
Re: [jdk23] RFR: 8333931: Problemlist serviceability/jvmti/vthread/CarrierThreadEventNotification
On Tue, 11 Jun 2024 11:34:34 GMT, Serguei Spitsyn wrote: > Please, review a jdk23 backport of the: > [8333931](https://bugs.openjdk.org/browse/JDK-8333931): Problemlist > serviceability/jvmti/vthread/CarrierThreadEventNotification > > Thanks Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19651#pullrequestreview-2111015465
Re: RFR: 8333756: java/lang/instrument/NativeMethodPrefixApp.java failed due to missing intrinsic [v2]
On Tue, 11 Jun 2024 02:10:46 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this test-only change which fixes an issue that
>> was introduced due to the refactoring that we did in
>> https://bugs.openjdk.org/browse/JDK-8333130? This PR addresses the failure
>> reported in https://bugs.openjdk.org/browse/JDK-8333756.
>>
>> The `NativeMethodPrefixApp` test uses a `javaagent`
>> `NativeMethodPrefixAgent` which modifies the name of the native methods
>> using the `java.lang.instrument.Instrumentation` instance:
>>
>> public static void premain (String agentArgs, Instrumentation instArg) {
>> inst = instArg;
>> System.out.println("Premain");
>>
>> ...
>> instArg.setNativeMethodPrefix(t0, "wrapped_tr0_");
>> instArg.setNativeMethodPrefix(t1, "wrapped_tr1_");
>> instArg.setNativeMethodPrefix(t2, "wrapped_tr2_");
>>
>>
>> The Hotspot VM allows for methods on a class to be annotated with an (VM
>> internal) `jdk.internal.vm.annotation.IntrinsicCandidate` annotation. When a
>> class that contains any methods that are annotated with
>> `@IntrinsicCandidate` is loaded, the VM checks that the corresponding
>> method(s) have an intrinsic available. It uses the method name to check for
>> the presence of the intrinsic. In the absence of an intrinsic for a
>> `@IntrinsicCandidate` method, the VM throws an error and exits. This
>> behaviour is controlled by the `-XX:+/-CheckIntrinsics` option. By default
>> that option is enabled, implying that an error will be thrown if the
>> intrinsic isn't found.
>>
>> In the case where/when this test fails, it so happens that the JVM loads a
>> class which has a `@IntrinsicCandidate` on a `native` Java method. For
>> example, on the failing host, I could see this class loading sequence:
>>
>>
>> tr2: Loading java/util/Date
>> tr1: Loading java/util/Date
>> tr0: Loading java/util/Date
>> tr2: Loading sun/util/calendar/CalendarSystem
>> tr1: Loading sun/util/calendar/CalendarSystem
>> tr0: Loading sun/util/calendar/CalendarSystem
>> tr2: Loading sun/util/calendar/CalendarSystem$GregorianHolder
>> tr1: Loading sun/util/calendar/CalendarSystem$GregorianHolder
>> tr0: Loading sun/util/calendar/CalendarSystem$GregorianHolder
>> ...
>> tr2: Loading sun/util/calendar/ZoneInfoFile$Checksum
>> tr1: Loading sun/util/calendar/ZoneInfoFile$Checksum
>> tr0: Loading sun/util/calendar/ZoneInfoFile$Checksum
>> tr2: Loading java/util/zip/Checksum
>> tr1: Loading java/util/zip/Checksum
>> tr0: Loading java/util/zip/Checksum
>> tr2: Loading java/util/zip/CRC32
>> tr1: Loading java/util/zip/CRC32
>> tr0: Loading java/util/zip/CRC32
>> Method [java.util.zi...
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update code comment
Marked as reviewed by cjplummer (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/19595#pullrequestreview-2109149823
Re: RFR: 8333827: JDK 23 RDP1 L10n resource files update [v2]
On Mon, 10 Jun 2024 21:58:29 GMT, Damon Nguyen wrote:
>> This issue is responsible for updating the translations of all the
>> localize(able) resources in the JDK. Primarily, the changes between JDK 22
>> RDP 1 and the integration of the JDK 23 RDP 1 L10n drop will be translated.
>>
>> The translation tool adjusted some definitions, which causes some changes in
>> localized files where the source file had no changes. This causes some words
>> being reverted from localized languages to English, and some had its
>> definitions changed.
>>
>> Alternatively, the diffs are viewable here and was generated using Jonathan
>> Gibbons' diff tool for l10n:
>> https://cr.openjdk.org/~dnguyen/output2/
>
> Damon Nguyen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove double quotes
src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTYResources_de.java
line 325:
> 323: {"Unexpected event type", "Unerwarteter Ereignistyp: {0}"},
> 324: {"unknown", "unbekannt"},
> 325: {"Unmonitoring", "Monitoring von {0} aufheben"},
The English entry is:
{"Unmonitoring", "Unmonitoring {0} "},
But the German entry now says "Monitoring". I'm sure what the original
"\u00DCberwachung" translates to, other than berwachung is monitoring. Now this
resource is partly in English, and is the incorrect English.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19609#discussion_r1633902362
Re: RFR: 8333756: java/lang/instrument/NativeMethodPrefixApp.java failed due to missing intrinsic
On Fri, 7 Jun 2024 10:31:22 GMT, Jaikiran Pai wrote:
> Can I please get a review of this test-only change which fixes an issue that
> was introduced due to the refactoring that we did in
> https://bugs.openjdk.org/browse/JDK-8333130? This PR addresses the failure
> reported in https://bugs.openjdk.org/browse/JDK-8333756.
>
> The `NativeMethodPrefixApp` test uses a `javaagent` `NativeMethodPrefixAgent`
> which modifies the name of the native methods using the
> `java.lang.instrument.Instrumentation` instance:
>
> public static void premain (String agentArgs, Instrumentation instArg) {
> inst = instArg;
> System.out.println("Premain");
>
> ...
> instArg.setNativeMethodPrefix(t0, "wrapped_tr0_");
> instArg.setNativeMethodPrefix(t1, "wrapped_tr1_");
> instArg.setNativeMethodPrefix(t2, "wrapped_tr2_");
>
>
> The Hotspot VM allows for methods on a class to be annotated with an (VM
> internal) `jdk.internal.vm.annotation.IntrinsicCandidate` annotation. When a
> class that contains any methods that are annotated with `@IntrinsicCandidate`
> is loaded, the VM checks that the corresponding method(s) have an intrinsic
> available. It uses the method name to check for the presence of the
> intrinsic. In the absence of an intrinsic for a `@IntrinsicCandidate` method,
> the VM throws an error and exits. This behaviour is controlled by the
> `-XX:+/-CheckIntrinsics` option. By default that option is enabled, implying
> that an error will be thrown if the intrinsic isn't found.
>
> In the case where/when this test fails, it so happens that the JVM loads a
> class which has a `@IntrinsicCandidate` on a `native` Java method. For
> example, on the failing host, I could see this class loading sequence:
>
>
> tr2: Loading java/util/Date
> tr1: Loading java/util/Date
> tr0: Loading java/util/Date
> tr2: Loading sun/util/calendar/CalendarSystem
> tr1: Loading sun/util/calendar/CalendarSystem
> tr0: Loading sun/util/calendar/CalendarSystem
> tr2: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> tr1: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> tr0: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> ...
> tr2: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr1: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr0: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr2: Loading java/util/zip/Checksum
> tr1: Loading java/util/zip/Checksum
> tr0: Loading java/util/zip/Checksum
> tr2: Loading java/util/zip/CRC32
> tr1: Loading java/util/zip/CRC32
> tr0: Loading java/util/zip/CRC32
> Method [java.util.zip.CRC32.wrapped_tr2_update(II)I] is annotated with
> @IntrinsicCandidate, but no...
test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 103:
> 101: // the native method names, which then causes a failure
> in the VM check
> 102: // for the presence of an intrinsic on a
> @IntrinsicCandidate native method
> 103: "-XX:+UnlockDiagnosticVMOptions", "-XX:-CheckIntrinsics",
Can you update both comments to begin with a capital letter and end with a
period. Thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19595#discussion_r1633711272
Re: RFR: 8333841: Add more logging into setfldw001 tests.
On Sat, 8 Jun 2024 17:00:04 GMT, Leonid Mesnik wrote: > Tests > SetFieldAccessWatch/setfldw001 > SetFieldModificationWatch/setfmodw001 > intermittently fail with Xcomp. I was unable to reproduce the problem. > The fix adds more checks and variants with jvmti logging. > > The goal is to understand why the test fails. > 1. Confirm that the event is not sent. Currently, the test doesn't differ > between sending "NULL" event and not sending an event at all. > 2. Check if the interpreter-only mode is switched too late in Thread-1. The > jvmti logging shows when field events are enabled and when each thread > actually switches to interpreter-only and enables event handling. > > The plan is to try to reproduce the failure and remove '@test id=logging' > after https://bugs.openjdk.org/browse/JDK-8205957 is fixed. Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19612#pullrequestreview-2108557198
Re: RFR: 8322811: jcmd System.dump_map help info has conflicting statements
On Fri, 7 Jun 2024 10:40:07 GMT, Thomas Stuefe wrote:
> @dholmes-ora this is one of yours.
>
> This was a tad annoying to fix (fix is simple though), since the jcmd
> framework has no good way to allow for default parameters that are not used
> literally. E.g. in this case, the real value for the file name will contain
> the process pid, which of course cannot be hard-coded.
>
> New output:
>
>
> Syntax : System.dump_map [options]
>
> Options: (options must be specified using the or = syntax)
> -H : [optional] Human readable format (BOOLEAN, false)
> -F : [optional] file path (STRING, vm_memory_map_.txt)
jcmd.l should also have been fixed. It currently reads:
options:
-H: (Optional) Human readable format (BOOLEAN, false)
-F: (Optional) File path (STRING, "vm_memory_map_.txt")
Note thet `` is missing. You can use the changes for
[JDK-8323546](https://bugs.openjdk.org/browse/JDK-8323546) as a model on how
to specify and describe the default parameter.
src/hotspot/share/services/diagnosticCommand.cpp line 1211:
> 1209: } else {
> 1210: name = _filename.value();
> 1211: }
This code should be considering if a default it specified or not, in case the
specified value is identical to what is contained in `default_filename`. With
the current solution, if the user literally specifies vm_memory_map_.txt,
the part will be expanded to the actual pid. See how
[JDK-8323546](https://bugs.openjdk.org/browse/JDK-8323546) handled this.
-
PR Comment: https://git.openjdk.org/jdk/pull/19596#issuecomment-2158908825
PR Review Comment: https://git.openjdk.org/jdk/pull/19596#discussion_r1633575756
Withdrawn: 8333813: Seviceability tests fail due to stderr containing Temporarily processing option UseNotificationThread
On Fri, 7 Jun 2024 19:56:03 GMT, Chris Plummer wrote: > Allow warning messages such as the following to appear in stderr: > > Java HotSpot(TM) 64-Bit Server VM warning: Temporarily processing option > UseNotificationThread; support is scheduled for removal in 24.0 > > Tested by running CI tier5, which reproduced the issue. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/19606
Re: RFR: 8333813: Seviceability tests fail due to stderr containing Temporarily processing option UseNotificationThread
On Sat, 8 Jun 2024 07:07:41 GMT, Jaikiran Pai wrote:
>> Possibly. It means updating 15 tests. Also need to come up with a new name.
>> Any suggestions?
>
> Hello Chris, given these similary named methods in this class, perhaps we
> should instead just have one single `stderrShouldBeEmptyIgnoring(...)` method
> which takes the messages that should be ignored. Something like:
>
>
> public static final String VM_WARNING_MSG = ".* VM warning:.*";
>
> public static final String VM_WARNING_DEPRECATED_MSG = ".* VM warning:.*
> deprecated.*";
> ...
>
> public OutputAnalyzer stderrShouldBeEmptyIgnoring(String ignoreMsg, String...
> additionalIgnoreMsgs) {
> String stdErrContent = getStderr().replaceAll(ignoreMsg + "\\R", "");
> if (additionalIgnoreMsgs != null) {
> for (String additionalIgnore : additionalIgnoreMsgs) {
> stdErrContent = stdErrContent.replaceAll(additionalIgnore +
> "\\R", "");
> }
> }
> if (!stdErrContent.isEmpty()) {
> reportDiagnosticSummary();
> throw new RuntimeException("stderr was not empty");
> }
> return this;
> }
>
>
> We make those private fields in OutputAnalyzer public and have the caller
> pass them:
>
>
> oa.stderrShouldBeEmptyIgnoring(OutputAnalyzer.VM_WARNING_DEPRECATED_MSG)
That seems like a lot of busy work and over abstraction for trying to solve a
rather trivial issue that already has a very simple solution, but maybe could
use a better API name.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19606#discussion_r1631928608
Re: RFR: 8333813: Seviceability tests fail due to stderr containing Temporarily processing option UseNotificationThread
On Sat, 8 Jun 2024 02:21:34 GMT, David Holmes wrote:
>> Allow warning messages such as the following to appear in stderr:
>>
>> Java HotSpot(TM) 64-Bit Server VM warning: Temporarily processing option
>> UseNotificationThread; support is scheduled for removal in 24.0
>>
>> Tested by running CI tier5, which reproduced the issue.
>
> test/lib/jdk/test/lib/process/OutputAnalyzer.java line 187:
>
>> 185: * If stderr was not empty
>> 186: */
>> 187: public OutputAnalyzer stderrShouldBeEmptyIgnoreDeprecatedWarnings()
>> {
>
> Maybe this should be renamed to reflect that it is about VM option warnings.
Possibly. It means updating 15 tests. Also need to come up with a new name. Any
suggestions?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19606#discussion_r1631916321
RFR: 8333813: Seviceability tests fail due to stderr containing Temporarily processing option UseNotificationThread
Allow warning messages such as the following to appear in stderr: Java HotSpot(TM) 64-Bit Server VM warning: Temporarily processing option UseNotificationThread; support is scheduled for removal in 24.0 Tested by running CI tier5, which reproduced the issue. - Commit messages: - Allow warning messages about obsolete flags Changes: https://git.openjdk.org/jdk/pull/19606/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19606=00 Issue: https://bugs.openjdk.org/browse/JDK-8333813 Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19606.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19606/head:pull/19606 PR: https://git.openjdk.org/jdk/pull/19606
Re: RFR: 8333680: com/sun/tools/attach/BasicTests.java fails with "SocketException: Permission denied: connect"
On Thu, 6 Jun 2024 02:12:11 GMT, Alex Menkov wrote: > The fix updates com/sun/tools/attach/BasicTests.java to listen and connect > using loopback addresses > > Testing: run the test on all Oracle-supported platforms Actually it looks like .jcheck/conf in master has not been updated to 24 yet. Not sure if once it is updated this warning will automatically go away or if you will also have to merge with master. - PR Comment: https://git.openjdk.org/jdk/pull/19571#issuecomment-2152727812
Re: RFR: 8333680: com/sun/tools/attach/BasicTests.java fails with "SocketException: Permission denied: connect"
On Thu, 6 Jun 2024 02:12:11 GMT, Alex Menkov wrote: > The fix updates com/sun/tools/attach/BasicTests.java to listen and connect > using loopback addresses > > Testing: run the test on all Oracle-supported platforms You have an interesting warning about the fixVersion being 24 but .jcheck/conf is 23. I'm not sure, but you may need to merge with the current master in order to avoid a 23 backport being created. - PR Comment: https://git.openjdk.org/jdk/pull/19571#issuecomment-2152706365
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v7]
On Wed, 5 Jun 2024 23:57:02 GMT, Serguei Spitsyn wrote: >> The following RFE was fixed recently: >> [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with >> nullptr in JVMTI generated code >> >> It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI >> agents can be developed in C or C++. >> This update is to make it clear that `nullptr` is C programming language >> `null` pointer. >> >> I think we do not need a CSR for this fix. >> >> Testing: N/A (not needed) > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: add a sub-section: Null Pointers Looks good. Thanks for the changes. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19257#pullrequestreview-2100483644
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v5]
On Wed, 5 Jun 2024 17:00:34 GMT, Chris Plummer wrote: >> Okay, thanks. What about the following: : >> >> diff --git a/src/hotspot/share/prims/jvmti.xml >> b/src/hotspot/share/prims/jvmti.xml >> index a6ebd0d42c5..a81014c70bb 100644 >> --- a/src/hotspot/share/prims/jvmti.xml >> +++ b/src/hotspot/share/prims/jvmti.xml >> @@ -995,7 +995,10 @@ jvmtiEnv *jvmti; >> across threads and are created dynamically. >> >> >> - >> + >> +There are a few functions where a parameter value can be a >> null pointer >> +(C NULL or C++ nullptr), e.g. the thread >> parameter >> +can be a null pointer to mean the current thread. >> functions always return an >> error code via the >> function return value. >> @@ -1004,7 +1007,7 @@ jvmtiEnv *jvmti; >> In some cases, functions allocate memory that your program must >> explicitly deallocate. This is indicated in the individual >> function descriptions. Empty lists, arrays, sequences, etc are >> -returned as a null pointer (C NULL or C++ >> nullptr). >> +returned as a null pointer. >> >> In the event that the function encounters >> an error (any return value other than JVMTI_ERROR_NONE) >> the values >> >> >> I can try to add a couple of more examples where a null pointer can be >> passed as a parameter value if it is desirable. > > I'm still not sure this works. It seems kind of muddled. Rather than trying > to retrofit in the clarifying text, why not start from scratch. That should > result in better organization and clearer descriptions. For example, I think > first you should clarify what is meant by a "null pointer". Maybe even make > that a separate section. I can take a stab at this later today if you want. How about undoing the changes in this subsection and then just add the following as a preceding subsection: **Null Pointers** Parts of this specification refer to a "null pointer" as a possible function parameter or return value. A "null pointer" is C `NULL` or C++ `nullptr`. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1628532037
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v5]
On Tue, 4 Jun 2024 23:52:36 GMT, Serguei Spitsyn wrote: >> Yes, my point was that this section is only for return values. The section >> is titled "Function Return Values". Maybe we should add another short >> section just before this one to describe what is meant by "null pointer". > > Okay, thanks. What about the following: : > > diff --git a/src/hotspot/share/prims/jvmti.xml > b/src/hotspot/share/prims/jvmti.xml > index a6ebd0d42c5..a81014c70bb 100644 > --- a/src/hotspot/share/prims/jvmti.xml > +++ b/src/hotspot/share/prims/jvmti.xml > @@ -995,7 +995,10 @@ jvmtiEnv *jvmti; > across threads and are created dynamically. > > > - > + > +There are a few functions where a parameter value can be a null > pointer > +(C NULL or C++ nullptr), e.g. the thread > parameter > +can be a null pointer to mean the current thread. > functions always return an > error code via the > function return value. > @@ -1004,7 +1007,7 @@ jvmtiEnv *jvmti; > In some cases, functions allocate memory that your program must > explicitly deallocate. This is indicated in the individual > function descriptions. Empty lists, arrays, sequences, etc are > -returned as a null pointer (C NULL or C++ > nullptr). > +returned as a null pointer. > > In the event that the function encounters > an error (any return value other than JVMTI_ERROR_NONE) the > values > > > I can try to add a couple of more examples where a null pointer can be passed > as a parameter value if it is desirable. I'm still not sure this works. It seems kind of muddled. Rather than trying to retrofit in the clarifying text, why not start from scratch. That should result in better organization and clearer descriptions. For example, I think first you should clarify what is meant by a "null pointer". Maybe even make that a separate section. I can take a stab at this later today if you want. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1628123917
Re: RFR: 8333178: ubsan: jvmti_tools.cpp:149:16: runtime error: null pointer passed as argument 2, which is declared to never be null
On Wed, 5 Jun 2024 13:04:03 GMT, Matthias Baesken wrote: > With ubsan enabled binaries we run into the following issue in HS :tier4 > tests : > e.g. > vmTestbase/nsk/jvmti/unit/FollowReferences/followref006/TestDescription.jtr > > /jdk/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp:149:16: > runtime error: null pointer passed as argument 2, which is declared to never > be null > #0 0x7fa7a1049482 in add_option > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp:149 > #1 0x7fa7a1049482 in nsk_jvmti_parseOptions > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp:242 > #2 0x7fa7a10634cd in Agent_Initialize > test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/FollowReferences/followref006/followref006.cpp:216 > #3 0x7fa79a9dbb36 in invoke_Agent_OnLoad > src/hotspot/share/prims/jvmtiAgent.cpp:609 > #4 0x7fa79a9dbb36 in JvmtiAgent::load(outputStream*) > src/hotspot/share/prims/jvmtiAgent.cpp:623 > #5 0x7fa79a9defd4 in load_agents > src/hotspot/share/prims/jvmtiAgentList.cpp:179 > #6 0x7fa79a9defd4 in JvmtiAgentList::load_agents() > src/hotspot/share/prims/jvmtiAgentList.cpp:190 > #7 0x7fa79bdad503 in Threads::create_vm(JavaVMInitArgs*, bool*) > src/hotspot/share/runtime/threads.cpp:505 > #8 0x7fa79a6e531f in JNI_CreateJavaVM_inner > src/hotspot/share/prims/jni.cpp:3581 > #9 0x7fa79a6e531f in JNI_CreateJavaVM src/hotspot/share/prims/jni.cpp:3672 > #10 0x7fa7a11277d5 in InitializeJVM > src/java.base/share/native/libjli/java.c:1550 > #11 0x7fa7a11277d5 in JavaMain > src/java.base/share/native/libjli/java.c:491 > #12 0x7fa7a1130f68 in ThreadJavaMain > src/java.base/unix/native/libjli/java_md.c:653 > #13 0x7fa7a10df6e9 in start_thread (/lib64/libpthread.so.0+0xa6e9) > (BuildId: 2f8d3c2d0f4d7888c2598d2ff6356537f5708a73) > #14 0x7fa7a071550e in clone (/lib64/libc.so.6+0x11850e) (BuildId: > f732026552f6adff988b338e92d466bc81a01c37) Looks good. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19557#pullrequestreview-2099719890
Re: RFR: 8333235: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with C1
On Tue, 4 Jun 2024 17:25:04 GMT, Leonid Mesnik wrote: > The kill sends async exception that is thrown somewhere during > log.display(...) method. > The log shows that it is thrown while PrintStream locking moving it into an > inconsistent state. > So the fix is to use some method that could be safely interrupted by async > exception. Ok. Looks good then. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19547#pullrequestreview-2097415752
Re: RFR: 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share [v4]
On Tue, 4 Jun 2024 18:02:13 GMT, Leonid Mesnik wrote: >> The fix removes finalization cleanup from vmTestbase. >> The last to classes that use it are: DebugeeBinder and SocketIOPipe. >> The DebugeeBinder is used in jdi and jdwp tests and is always linked with >> debuggee process. So the DebugeeProcess.waitFor() is the good place to close >> binder and free all it's resources. >> The SocketIOPipe is used directly in AOD tests where it should be closed >> after test execution. >> >> The OPipe (child of SocketIOPipe) also used in jdi and jdwp tests where it >> is connected directly in tests. However is also connected with debuggee and >> could be closed in DebugeeProcess.waitFor(). >> >> The VMOutOfMemoryException001 test is fixed to release some memory after >> throwing OOME so Sytem.exit() could complete successfully. Previously some >> memory freed during VM shutdown hook. >> >> I verified that cleanup printed that corresponding 'close' method has been >> already called before VM shutdown phase for debugger process. >> Additionally, run all vmTestbase tests to verify there are no failures, > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > added message Looks good. Thanks for cleanup. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19505#pullrequestreview-2097199005
Re: RFR: 8333235: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with C1
On Tue, 4 Jun 2024 17:25:04 GMT, Leonid Mesnik wrote: > The kill sends async exception that is thrown somewhere during > log.display(...) method. > The log shows that it is thrown while PrintStream locking moving it into an > inconsistent state. > So the fix is to use some method that could be safely interrupted by async > exception. test/hotspot/jtreg/vmTestbase/nsk/jdb/kill/kill001/kill001a.java line 153: > 151: } > 152: } > 153: Have you tried an empty method? I don't think it's a matter of how much time you spend in the method, but just whether or not a JVM async exception polling point is reached. I suspect the method call or return will be a polling point, but I'm unsure what happens if the method call is elided by the JIT because it does nothing. - PR Review Comment: https://git.openjdk.org/jdk/pull/19547#discussion_r1626486192
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v5]
On Tue, 4 Jun 2024 07:01:54 GMT, Alan Bateman wrote: >> Thanks, David. I also feel this clarification is still useful. > > I think this is the right place but it is only for return values. There are a > few functions where a parameter value can be a null pointer, e.g. in > GetThreadState, SuspendThread, GetOwnedMonitorInfo the thread parameter can > be a null pointer to mean the current thread. I don't think the introduction > section has anywhere right now to say what a null pointer means. Yes, my point was that this section is only for return values. The section is titled "Function Return Values". Maybe we should add another short section just before this one to describe what is meant by "null pointer". - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1626480428
Re: RFR: 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share [v3]
On Mon, 3 Jun 2024 23:02:28 GMT, Leonid Mesnik wrote:
>> The fix removes finalization cleanup from vmTestbase.
>> The last to classes that use it are: DebugeeBinder and SocketIOPipe.
>> The DebugeeBinder is used in jdi and jdwp tests and is always linked with
>> debuggee process. So the DebugeeProcess.waitFor() is the good place to close
>> binder and free all it's resources.
>> The SocketIOPipe is used directly in AOD tests where it should be closed
>> after test execution.
>>
>> The OPipe (child of SocketIOPipe) also used in jdi and jdwp tests where it
>> is connected directly in tests. However is also connected with debuggee and
>> could be closed in DebugeeProcess.waitFor().
>>
>> The VMOutOfMemoryException001 test is fixed to release some memory after
>> throwing OOME so Sytem.exit() could complete successfully. Previously some
>> memory freed during VM shutdown hook.
>>
>> I verified that cleanup printed that corresponding 'close' method has been
>> already called before VM shutdown phase for debugger process.
>> Additionally, run all vmTestbase tests to verify there are no failures,
>
> Leonid Mesnik has updated the pull request incrementally with one additional
> commit since the last revision:
>
> moved return out of try/catch
test/hotspot/jtreg/vmTestbase/nsk/jdi/VMOutOfMemoryException/VMOutOfMemoryException001/VMOutOfMemoryException001t.java
line 50:
> 48: } catch (OutOfMemoryError e) {
> 49: isOutOfMemory = true;
> 50: buf = null;
Do you think maybe it is worth logging "Got OOME" or something like that? Also,
I think this could use a comment, and `buf` could use a more descriptive name.
Maybe "reservedMem".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19505#discussion_r1625192562
Re: RFR: 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share [v2]
On Sat, 1 Jun 2024 21:11:26 GMT, Leonid Mesnik wrote:
>> The fix removes finalization cleanup from vmTestbase.
>> The last to classes that use it are: DebugeeBinder and SocketIOPipe.
>> The DebugeeBinder is used in jdi and jdwp tests and is always linked with
>> debuggee process. So the DebugeeProcess.waitFor() is the good place to close
>> binder and free all it's resources.
>> The SocketIOPipe is used directly in AOD tests where it should be closed
>> after test execution.
>>
>> The OPipe (child of SocketIOPipe) also used in jdi and jdwp tests where it
>> is connected directly in tests. However is also connected with debuggee and
>> could be closed in DebugeeProcess.waitFor().
>>
>> The VMOutOfMemoryException001 test is fixed to release some memory after
>> throwing OOME so Sytem.exit() could complete successfully. Previously some
>> memory freed during VM shutdown hook.
>>
>> I verified that cleanup printed that corresponding 'close' method has been
>> already called before VM shutdown phase for debugger process.
>> Additionally, run all vmTestbase tests to verify there are no failures,
>
> Leonid Mesnik has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixed try/finally
test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeProcess.java line 201:
> 199: try {
> 200: int exitCode = waitForDebugee();
> 201: return exitCode;
I don't think I've ever run across a try block with a return statement before,
especially when there is also a finally block. The reader is likely to miss the
fact that before the return is done the finally block is executed. It's also
odd because now there is no return statement at the end of the method. Although
the compiler is smart enough to recognize that this is ok, it is another point
of confusion for the reader. Any reason not to just instead do the return at
the end of the method?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19505#discussion_r1625109575
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers [v5]
On Mon, 3 Jun 2024 09:58:38 GMT, Serguei Spitsyn wrote: >> The following RFE was fixed recently: >> [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with >> nullptr in JVMTI generated code >> >> It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI >> agents can be developed in C or C++. >> This update is to make it clear that `nullptr` is C programming language >> `null` pointer. >> >> I think we do not need a CSR for this fix. >> >> Testing: N/A (not needed) > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: consistency and stylistical corrections src/hotspot/share/prims/jvmti.xml line 1007: > 1005: explicitly deallocate. This is indicated in the individual > 1006: function descriptions. Empty lists, arrays, sequences, etc are > 1007: returned as a null pointer (C NULL or C++ > nullptr). Why describe what is meant by a "null pointer" here when it is not done elsewhere? - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1625105918
Re: RFR: 8307824: Clean up Finalizable.java and finalize terminology in vmTestbase/nsk/share
On Fri, 31 May 2024 18:22:47 GMT, Leonid Mesnik wrote:
> The fix removes finalization cleanup from vmTestbase.
> The last to classes that use it are: DebugeeBinder and SocketIOPipe.
> The DebugeeBinder is used in jdi and jdwp tests and is always linked with
> debuggee process. So the DebugeeProcess.waitFor() is the good place to close
> binder and free all it's resources.
> The SocketIOPipe is used directly in AOD tests where it should be closed
> after test execution.
>
> The OPipe (child of SocketIOPipe) also used in jdi and jdwp tests where it is
> connected directly in tests. However is also connected with debuggee and
> could be closed in DebugeeProcess.waitFor().
>
> The VMOutOfMemoryException001 test is fixed to release some memory after
> throwing OOME so Sytem.exit() could complete successfully. Previously some
> memory freed during VM shutdown hook.
>
> I verified that cleanup printed that corresponding 'close' method has been
> already called before VM shutdown phase for debugger process.
> Additionally, run all vmTestbase tests to verify there are no failures,
test/hotspot/jtreg/vmTestbase/nsk/share/aod/DummyTargetApplication.java line 68:
> 66: if ((signal == null) ||
> !signal.equals(AODTestRunner.SIGNAL_FINISH))
> 67: throw new TestBug("Unexpected signal: '" + signal + "'");
> 68: pipe.close();
Exceptions can be thrown before you get here.
test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeProcess.java line 215:
> 213: if (binder != null) {
> 214: binder.close();
> 215: }
Won't this be skipped if there is an exception during `waitForDebugee` or
`waitForRedirectors`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19505#discussion_r1622898244
PR Review Comment: https://git.openjdk.org/jdk/pull/19505#discussion_r1622896237
Re: RFR: 8333353: Delete extra empty line in CodeBlob.java
On Fri, 31 May 2024 12:29:59 GMT, SendaoYan wrote: > Hi all, > This trivial fix, delete the extra empty line before `getOopMaps` function > in `src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CodeBlob.java` > file. > No risk. Approved and trivial. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19499#pullrequestreview-2091269784
Re: Stepping in debugger switches to interpretation mode
On 5/31/24 7:25 AM, Severin Gehwolf wrote: On Fri, 2024-05-31 at 14:44 +0200, Maksim Zuev wrote: Dear Sir/Madam, I encountered a problem while debugging the code. I am attaching the reproducer to this email in the Main.java file. When running it with the debugger without stepping, the application runs in less than a second (see jdb output in the jdb_run.txt file). However, after performing a single step, the application is running in interpretation mode, becoming very slow (see jdb output in the jdb_step.txt file). I assume running in the interpreter mode, as I see InterpreterRuntime::post_method_exit calls in the profiler. Could you please help me figure out what causes the application to run in the interpreter mode? Is this a bug or an expected behavior? Are there any ways to work around this issue? I think this is: https://bugs.openjdk.org/browse/JDK-8229012 Thanks, Severin Yes, that seems likely. It should resolve itself once you exit the method you were single stepping in. But as mentioned in the bug, if you single step in the main method of a thread, that thread will forever run interpreted. Chris
Re: RFR: 8320215: HeapDumper can use DumpWriter buffer during merge
On Fri, 19 Apr 2024 00:10:12 GMT, Alex Menkov wrote: > The fix updates HeapMerger to use writer buffer (no need to copy memory, also > writer buffer is 1MB instead of 4KB). > Additionally fixed small issue in FileWriter (looks like `ssize_t` instead of > `size_t` is a typo, the argument should be unsigned) > > Testing: all HeapDump-related tests on Oracle supported platforms Looks good. - PR Review: https://git.openjdk.org/jdk/pull/18850#pullrequestreview-2089482773
Integrated: 8332751: Broken link in VirtualMachine.html
On Wed, 29 May 2024 20:17:30 GMT, Chris Plummer wrote: > Fixed broken javadoc link. I confirmed that it currently is broken as can be > seen in the JDK 22 javadocs: > > https://docs.oracle.com/en%2Fjava%2Fjavase%2F22%2Fdocs%2Fapi%2F%2F/jdk.jdi/com/sun/jdi/VirtualMachine.html#allThreads() > > Click on the "virtual threads" link within the allThreads() description to > see the broken link. > > I verified the fix by building the javadocs and checking that the link now > works. This pull request has now been integrated. Changeset: 3634a910 Author:Chris Plummer URL: https://git.openjdk.org/jdk/commit/3634a9105053717f3099982390ce2b9e564f0ac5 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8332751: Broken link in VirtualMachine.html Reviewed-by: amenkov, alanb - PR: https://git.openjdk.org/jdk/pull/19469
Re: RFR: 8332751: Broken link in VirtualMachine.html
On Wed, 29 May 2024 20:17:30 GMT, Chris Plummer wrote: > Fixed broken javadoc link. I confirmed that it currently is broken as can be > seen in the JDK 22 javadocs: > > https://docs.oracle.com/en%2Fjava%2Fjavase%2F22%2Fdocs%2Fapi%2F%2F/jdk.jdi/com/sun/jdi/VirtualMachine.html#allThreads() > > Click on the "virtual threads" link within the allThreads() description to > see the broken link. > > I verified the fix by building the javadocs and checking that the link now > works. Thanks for the reviews Alan and Alex! - PR Comment: https://git.openjdk.org/jdk/pull/19469#issuecomment-2140825215
Integrated: 8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject
On Tue, 28 May 2024 23:20:48 GMT, Chris Plummer wrote: > Fixed "double-checked-locking" bug in `ReferenceImplType.classObject()`. > Details in the first comment. Tested with the following: > - com/sun/jdi > - nsk/jdi > - nsk/jdwp > - nsk/jdb This pull request has now been integrated. Changeset: 922e312b Author:Chris Plummer URL: https://git.openjdk.org/jdk/commit/922e312b0ab3ac54979ffdc53a8d8338e52234df Stats: 26 lines in 1 file changed: 2 ins; 13 del; 11 mod 8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject Reviewed-by: amenkov, sspitsyn - PR: https://git.openjdk.org/jdk/pull/19439
Re: RFR: 8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject [v2]
On Wed, 29 May 2024 01:47:27 GMT, Chris Plummer wrote: >> Fixed "double-checked-locking" bug in `ReferenceImplType.classObject()`. >> Details in the first comment. Tested with the following: >> - com/sun/jdi >> - nsk/jdi >> - nsk/jdwp >> - nsk/jdb > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Add period at end of comments Thanks for the reviews Serguei and Alex! - PR Comment: https://git.openjdk.org/jdk/pull/19439#issuecomment-2140499302
RFR: 8332751: Broken link in VirtualMachine.html
Fixed broken javadoc link. I confirmed that it currently is broken as can be seen in the JDK 22 javadocs: https://docs.oracle.com/en%2Fjava%2Fjavase%2F22%2Fdocs%2Fapi%2F%2F/jdk.jdi/com/sun/jdi/VirtualMachine.html#allThreads() Click on the "virtual threads" link within the allThreads() description to see the broken link. I verified the fix by building the javadocs and checking that the link now works. - Commit messages: - Fix broken link Changes: https://git.openjdk.org/jdk/pull/19469/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19469=00 Issue: https://bugs.openjdk.org/browse/JDK-8332751 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19469.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19469/head:pull/19469 PR: https://git.openjdk.org/jdk/pull/19469
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes
On Tue, 28 May 2024 22:24:53 GMT, Serguei Spitsyn wrote: > Please, review the following `interp-only` issue related to carrier threads. > There are 3 problems fixed here: > - The `EnterInterpOnlyModeClosure::do_threads` is taking the > `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect > when we have a deal with a carrier thread. The target state is known at the > point when the `HandshakeClosure` is set, so the fix is to pass it as a > constructor parameter. > - The `state->is_pending_interp_only_mode())` was processed at mounts only > but it has to be processed for unmounts as well. > - The test > `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp` > has a wrong assumption that there can't be `MethodExit` event on the carrier > thread when the function `breakpoint_hit1` is being executed. However, it can > happen if the virtual thread gets unmounted. > > The fix also includes new test case `vthread/CarrierThreadEventNotification` > developed by Patricio. > > Testing: > - Ran new test case locally > - Ran mach5 tiers 1-6 test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp line 201: > 199: > 200: // need to reset this value after the breakpoint_hit1 > 201: received_method_exit_event = JNI_FALSE; There was a loom-dev email thread regarding this last year. Seems related. I had concluded that the way the test was written that no MethodExit event should have been received. I'm not sure if I missed something in my analysis or if this failure is a result of your changes: https://mail.openjdk.org/pipermail/loom-dev/2023-August/006059.html https://mail.openjdk.org/pipermail/loom-dev/2023-September/006170.html - PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619356206
Integrated: 8332919: SA PointerLocation needs to print a newline after dumping java thread info for JNI Local Ref
On Fri, 24 May 2024 20:03:53 GMT, Chris Plummer wrote: > If PointerLocation discovers that an address is for a JNI local ref, it will > print information about the thread that owns the JNI local ref. For > JavaThreads it calls the printThreadIDOn(tty) method. There's a comment on > the call that says that it 'includes "\n"'. This is actually not true, and a > separate println() is needed. I noticed this when using the clhsdb findpc > command on a JNI local ref and noted that the next "hdsb> " prompt was > printed at the end of the findpc output instead of on a new line. This pull request has now been integrated. Changeset: c8eea59f Author:Chris Plummer URL: https://git.openjdk.org/jdk/commit/c8eea59f508158075382079316cf0990116ff98e Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod 8332919: SA PointerLocation needs to print a newline after dumping java thread info for JNI Local Ref Reviewed-by: kevinw, dholmes - PR: https://git.openjdk.org/jdk/pull/19402
Re: RFR: 8332919: SA PointerLocation needs to print a newline after dumping java thread info for JNI Local Ref
On Fri, 24 May 2024 20:03:53 GMT, Chris Plummer wrote: > If PointerLocation discovers that an address is for a JNI local ref, it will > print information about the thread that owns the JNI local ref. For > JavaThreads it calls the printThreadIDOn(tty) method. There's a comment on > the call that says that it 'includes "\n"'. This is actually not true, and a > separate println() is needed. I noticed this when using the clhsdb findpc > command on a JNI local ref and noted that the next "hdsb> " prompt was > printed at the end of the findpc output instead of on a new line. Thank you for the reviews Kevin and David! - PR Comment: https://git.openjdk.org/jdk/pull/19402#issuecomment-2138012861
Re: RFR: 8333108: Update vmTestbase/nsk/share/DebugeeProcess.java to don't use finalization
On Tue, 28 May 2024 20:24:31 GMT, Leonid Mesnik wrote: > The > test/hotspot/jtreg/vmTestbase/nsk/share/jpda/DebugeeProcess.java > uses cleanup() to kill debuggee process. > > However, most tests kill the debuggee process explicitly. I verified that > debuggee process is killed before test finishes. (Just by printing it's > status.) > > The fix adds a few checks debuggee.waitFor() int tests that didn't check > debuggee process code. Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19437#pullrequestreview-2086107795
Re: RFR: 8320215: HeapDumper can use DumpWriter buffer during merge
On Fri, 19 Apr 2024 00:10:12 GMT, Alex Menkov wrote:
> The fix updates HeapMerger to use writer buffer (no need to copy memory, also
> writer buffer is 1MB instead of 4KB).
> Additionally fixed small issue in FileWriter (looks like `ssize_t` instead of
> `size_t` is a typo, the argument should be unsigned)
>
> Testing: all HeapDump-related tests on Oracle supported platforms
src/hotspot/share/services/heapDumper.cpp line 2137:
> 2135: while ((cnt = segment_fs.read(_writer->buffer(), 1,
> _writer->buffer_size())) != 0) {
> 2136: _writer->set_position(cnt);
> 2137: _writer->flush();
Why flush on each iteration instead of just once after you are done with the
loop?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18850#discussion_r1618079060
Re: RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly [v4]
On Fri, 3 May 2024 01:54:24 GMT, Alex Menkov wrote: >> Some cleanup related to JvmtiEnvBase::get_threadOop_and_JavaThread method >> >> Testing: tier1-6 > > Alex Menkov has updated the pull request incrementally with three additional > commits since the last revision: > > - update > - Revert "renamed current_thread to current" > >This reverts commit d5d614bcf0861466acd695296e974d2253f84c9f. > - Revert "renamed current_thread tp current" > >This reverts commit 4602632221044aa754a1bc8d11e7a3e9a0092590. Looks good. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18986#pullrequestreview-2084100945
Re: RFR: 8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject [v2]
On Wed, 29 May 2024 01:09:50 GMT, Alex Menkov wrote:
>> Chris Plummer has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add period at end of comments
>
> src/jdk.jdi/share/classes/com/sun/tools/jdi/ReferenceTypeImpl.java line 185:
>
>> 183: public String signature() {
>> 184: if (signature == null) {
>> 185: // Does not need synchronization. Worst case is static info
>> is fetched twice
>
> Could you add dot at the end of all updated comments
Ok.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19439#discussion_r1618065025
Re: RFR: 8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject [v2]
> Fixed "double-checked-locking" bug in `ReferenceImplType.classObject()`. > Details in the first comment. Tested with the following: > - com/sun/jdi > - nsk/jdi > - nsk/jdwp > - nsk/jdb Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: Add period at end of comments - Changes: - all: https://git.openjdk.org/jdk/pull/19439/files - new: https://git.openjdk.org/jdk/pull/19439/files/97376705..58295072 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19439=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19439=00-01 Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/19439.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19439/head:pull/19439 PR: https://git.openjdk.org/jdk/pull/19439
Re: RFR: 8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject
On Tue, 28 May 2024 23:20:48 GMT, Chris Plummer wrote: > Fixed "double-checked-locking" bug in `ReferenceImplType.classObject()`. > Details in the first comment. Tested with the following: > - com/sun/jdi > - nsk/jdi > - nsk/jdwp > - nsk/jdb The bug is the classic "double-checked-locking" flaw. In general "double-checked-locking" does not work, but in this case, based on how it is used and the java memory model, it can be made to work by making the field volatile. Although the issue could be fixed by making the classObject field volatile, I decided to get rid of the double-checked-locking instead. There is little benefit to using it here (it simply avoids fetching the info twice when there is a race, which is very rare), and leaving it out simplifies the code and reduces overhead for the usual case (when there is no race). Regarding the pre-existing comment: // Are classObjects unique for an Object, or // created each time? Is this spec'ed? Yes, they are unique. No they are not created each time. Probably this lack of understanding is why double-checked-locking was used here. The ObjectReference spec is a bit lose on the wording, referring to an ObjectReference as "An object that currently exists in the target VM". However, the implementation is not. VirtualMachineImpl.objectMirror() is used to create all ObjectReference instances, and it only creates one ObjectReference per JDWP objectID. I tested this by making classObject() fetch the ObjectReference on every call and compare the result to the cached value, and they always match. Also, the JDWP spec calls out that each java Object instance has 1 unique objectID. The following is the JDWP spec description of the objectID type. "Uniquely identifies an object in the target VM. A particular object will be identified by exactly one objectID in JDWP commands and replies throughout its lifetime (or until the objectID is explicitly disposed). An ObjectID is not reused to identify a different object unless it has been explicitly disposed, regardless of whether the referenced object has been garbage collected. An objectID of 0 represents a null object. Note that the existence of an object ID does not prevent the garbage collection of the object. Any attempt to access a a garbage collected object with its object ID will result in the INVALID_OBJECT error code. Garbage collection can be disabled with the DisableCollection command, but it is not usually necessary to do so." While looking into this bug I also ran across the similar classLoader() API, and noticed that it did not use synchronized and explained why in a comment. That is where I got the motivation to remove synchronized from classObject(). Then I found a bunch more similar APIs. I cleaned up their comment to be consistent. - PR Comment: https://git.openjdk.org/jdk/pull/19439#issuecomment-2136257466
RFR: 8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject
Fixed "double-checked-locking" bug in `ReferenceImplType.classObject()`. Details in the first comment. Tested with the following: - com/sun/jdi - nsk/jdi - nsk/jdwp - nsk/jdb - Commit messages: - Update copyright - Unified 'worst case' comment. - Fix volatile issue with classObject() API. Changes: https://git.openjdk.org/jdk/pull/19439/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19439=00 Issue: https://bugs.openjdk.org/browse/JDK-8328611 Stats: 26 lines in 1 file changed: 2 ins; 13 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/19439.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19439/head:pull/19439 PR: https://git.openjdk.org/jdk/pull/19439
Re: RFR: 8333108: Update vmTestbase/nsk/share/DebugeeProcess.java to don't use finalization
On Tue, 28 May 2024 20:24:31 GMT, Leonid Mesnik wrote:
> I verified that debuggee process is killed before test finishes. (Just by
> printing it's status.)
What about when the test fails?
test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/exit/exit001.java line 177:
> 175:
> 176: int code = debuggee.waitFor();
> 177: if (code != 0) {
Why do the other tests check for Consts.JCK_STATUS_BASE, but here we check for
0?
-
PR Review: https://git.openjdk.org/jdk/pull/19437#pullrequestreview-2083968139
PR Review Comment: https://git.openjdk.org/jdk/pull/19437#discussion_r1617970400
Re: RFR: 8333013: Update vmTestbase/nsk/share/LocalProcess.java to don't use finalization [v2]
On Tue, 28 May 2024 20:55:30 GMT, Leonid Mesnik wrote: >> The vmTestbase/nsk/share/LocalProcess.java is a wrapper for debuggee >> process. It extends FinalizableObject to kill the debuggee process. >> >> The debuggee process is used by nsk.jdb tests only, see runTest(...) in >> vmTestbase/nsk/share/jdb/JdbTest.java: >> https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/share/jdb/JdbTest.java#L189 >> >> I verified that process is always already terminated when is cleaned during >> VM shutdown hook, >> So the fix is just to remove the finalization. >> >> I also moved LocalProcess into nsk.share.jdb to reduce the visibility of >> class and hardened checks in runTest. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > fixed check Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19418#pullrequestreview-2083955546
Re: RFR: 8333013: Update vmTestbase/nsk/share/LocalProcess.java to don't use finalization
On Tue, 28 May 2024 03:15:43 GMT, Leonid Mesnik wrote:
> The vmTestbase/nsk/share/LocalProcess.java is a wrapper for debuggee process.
> It extends FinalizableObject to kill the debuggee process.
>
> The debuggee process is used by nsk.jdb tests only, see runTest(...) in
> vmTestbase/nsk/share/jdb/JdbTest.java:
> https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/share/jdb/JdbTest.java#L189
>
> I verified that process is always already terminated when is cleaned during
> VM shutdown hook,
> So the fix is just to remove the finalization.
>
> I also moved LocalProcess into nsk.share.jdb to reduce the visibility of
> class and hardened checks in runTest.
test/hotspot/jtreg/vmTestbase/nsk/share/jdb/JdbTest.java line 221:
> 219: }
> 220:
> 221: if (debuggee != null && debuggee.terminated()) {
Shouldn't this be `!debuggee.terminated()`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19418#discussion_r1617771060
RFR: 8332919: SA PointerLocation needs to print a newline after dumping java thread info for JNI Local
If PointerLocation discovers that an address is for a JNI local ref, it will print information about the thread that owns the JNI local ref. For JavaThreads it calls the printThreadIDOn(tty) method. There's a comment on the call that says that it 'includes "\n"'. This is actually not true, and a separate println() is needed. I noticed this when using the clhsdb findpc command on a JNI local ref and noted that the next "hdsb> " prompt was printed at the end of the findpc output instead of on a new line. - Commit messages: - Need a newline after printThreadIDOn() call Changes: https://git.openjdk.org/jdk/pull/19402/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19402=00 Issue: https://bugs.openjdk.org/browse/JDK-8332919 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19402.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19402/head:pull/19402 PR: https://git.openjdk.org/jdk/pull/19402
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v6]
On Fri, 24 May 2024 18:22:18 GMT, Kevin Walls wrote: >> Running JConsole from a previous JDK, and attaching to jdk-23 (after >> [JDK-832](https://bugs.openjdk.org/browse/JDK-832): Remove the Java >> Management Extension (JMX) Subject Delegation feature), the MBean tab is >> blank. >> >> In javax/management/remote/rmi/RMIConnectionImpl.java: >> addNotificationListener rejects a non-null delegationSubjects array, but >> older JDKs will send such an array. It could accept the array, and only >> reject/throw if it contains a non-null Subject (i.e. if an attempt to use >> subject delegation is really happening). >> >> Manually testing JConsole, the MBean tab is fully populated and usable. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > entries consistency in param and throws text Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19253#pullrequestreview-2077642914
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v5]
On Fri, 24 May 2024 17:15:31 GMT, Kevin Walls wrote:
>> Running JConsole from a previous JDK, and attaching to jdk-23 (after
>> [JDK-832](https://bugs.openjdk.org/browse/JDK-832): Remove the Java
>> Management Extension (JMX) Subject Delegation feature), the MBean tab is
>> blank.
>>
>> In javax/management/remote/rmi/RMIConnectionImpl.java:
>> addNotificationListener rejects a non-null delegationSubjects array, but
>> older JDKs will send such an array. It could accept the array, and only
>> reject/throw if it contains a non-null Subject (i.e. if an attempt to use
>> subject delegation is really happening).
>>
>> Manually testing JConsole, the MBean tab is fully populated and usable.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove should... from delegationSubjects param
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
line 978:
> 976: * @throws IOException if a general communication exception occurred.
> 977: * @throws UnsupportedOperationException if {@code
> delegationSubjects}
> 978: * is non-null and contains any non-null values.
Minor consistency issue. For the `delegationSubjects` comment above, you refer
to "non-null entries". Here you refer to "non-null values". I don't have a
preference on which you use, but they should be the same in both cases.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1613843199
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v3]
On Fri, 24 May 2024 16:43:41 GMT, Kevin Walls wrote: >> How about "must be null or an array of all null entries". You could still >> have an `@apiNote` explaining why. > > Thanks, appreciate the effort trying to make it perfect. > Can't quite say "must be null or an array of all null entries" ..because I > suppose it could be an empty array. > > In reality, the only caller is our code that wraps a null Subject value, in > an array, so it's generally a single null in an array. > > I hope we are OK sticking with "which must not contain any non-null entries" > as that does cover it (and implicitly does tell you an empty array is fine). For me the main hold up is using "should". Maybe: "Must be null or an array that doesn't contain any non-null entries." - PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1613765813
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v3]
On Fri, 17 May 2024 10:35:39 GMT, Alan Bateman wrote:
>> Kevin Walls has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - add an 'also'
>> - typo
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
> line 961:
>
>> 959: * @param delegationSubjects should be {@code null}, but a non-null
>> 960: * array is also accepted for compatibility reasons, which must not
>> 961: * contain any non-null entries.
>
> The wording is bit unusual for a parameter description. Just wondering if
> might be clearer to say "null or an array of null elements" and put add an
> `@apiNote` to explain that it allows an array with null elements for
> compatibility reasons. What you have is okay too course, I'm just trying to
> think of another way to present this odd case.
How about "must be null or an array of all null entries". You could still have
an `@apiNote` explaining why.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1613693492
Re: RFR: 8330694: Rename 'HeapRegion' to 'G1HeapRegion' [v13]
On Fri, 24 May 2024 13:04:14 GMT, Lei Zaakjyu wrote: >> follow up 8267941 > > Lei Zaakjyu has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - review > - Merge branch 'master' of https://git.openjdk.org/jdk into JDK-8330694 > - restore > - Merge branch 'master' of https://git.openjdk.org/jdk into JDK-8330694 > - review > - Merge branch 'master' into JDK-8330694 > - fix indentation > - also tidy up > - tidy up > - rename Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18871#pullrequestreview-2077242456
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v7]
On Wed, 22 May 2024 17:43:53 GMT, Chris Plummer wrote: >>> Is that "then popFrameEventLock second" >>> >> Yes. I'll fix. >> >>> Drawing these out in two columns I can't see a deadlock either 8-) >> >> Ironically right now I'm looking at a very rare deadlock that involves this >> code. It doesn't seem to happen when I disabled ranked locking. It might be >> instigated by the dbgRawMonitor that ranked locking uses. > >> Ironically right now I'm looking at a very rare deadlock that involves this >> code. It doesn't seem to happen when I disabled ranked locking. It might be >> instigated by the dbgRawMonitor that ranked locking uses. > > @kevinjwalls I tracked down this deadlock and filed > [JDK-8332738](https://bugs.openjdk.org/browse/JDK-8332738). It's really a > pre-existing issue, but we get lucky with the current implementation because > RawMonitorExit does not self suspend until after releasing the monitor, thus > avoiding the deadlock. The ranked monitors implementation adds a self suspend > opportunity when we release a raw monitor, which is it hits this bug. This is > a very ugly issue that involves two threads getting events at the same time, > and the debugger doing a StackFrame.PopFrames. Fortunately the "pop frames" > locks are not involved. I should also add that this issue was only turning up when I had virtual threads enabled, because that caused one of the threads to trigger a class load of some CarrierThread inner class the first time it parked. This generated a ClassPrepare event with unfortunately timing (it would be on Thread 2 of my description in [JDK-8332738](https://bugs.openjdk.org/browse/JDK-8332738)). However, I now have a much more direct test that doesn't require virtual threads and more readily reproduces the deadlock. - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1610413014
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v7]
On Thu, 16 May 2024 20:02:22 GMT, Chris Plummer wrote: > Ironically right now I'm looking at a very rare deadlock that involves this > code. It doesn't seem to happen when I disabled ranked locking. It might be > instigated by the dbgRawMonitor that ranked locking uses. @kevinjwalls I tracked down this deadlock and filed [JDK-8332738](https://bugs.openjdk.org/browse/JDK-8332738). It's really a pre-existing issue, but we get lucky with the current implementation because RawMonitorExit does not self suspend until after releasing the monitor, thus avoiding the deadlock. The ranked monitors implementation adds a self suspend opportunity when we release a raw monitor, which is it hits this bug. This is a very ugly issue that involves two threads getting events at the same time, and the debugger doing a StackFrame.PopFrames. Fortunately the "pop frames" locks are not involved. - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1610404920
Re: RFR: 8332641: Update nsk.share.jpda.Jdb to don't use finalization
On Tue, 21 May 2024 21:49:51 GMT, Leonid Mesnik wrote: > The nsk.share.jdb.Jdb has finalize() nethods that close jdb connection and > output streams. > > The fix renames the method to close() and calls it explicitly after the test > finishes. I verified that close() called for each nsk share jdb test. > > The jdb is also LocalProcess with it's own cleanup(). This part still remains > the same so far. Looks good. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19336#pullrequestreview-2071834856
Re: RFR: 8332631: Update nsk.share.jpda.BindServer to don't use finalization
On Tue, 21 May 2024 19:55:01 GMT, Leonid Mesnik wrote: > The BindServer starts several threads and opens streams. > > It registered them for cleanup using "Finalizer" from nsk.share.framework. > Currently, it cleanup resources during shutdown hook. > > This fix changes BindServer to explicitly close streams and finish threads > after test is completed. The exceptions are just printed like it was done > previously. I haven't caught any exception during in close method during > testing. Looks good. Just one minor comment suggestion. test/hotspot/jtreg/vmTestbase/nsk/share/jpda/BindServer.java line 634: > 632: /** > 633: * Close thread by closing all connections and waiting > 634: * for thread finished. Suggestion: * for thread to finish. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19335#pullrequestreview-2071799883 PR Review Comment: https://git.openjdk.org/jdk/pull/19335#discussion_r1610313535
Re: RFR: 8331683: Clean up GetCarrierThread
On Sat, 18 May 2024 00:47:59 GMT, Alex Menkov wrote: > JVMTI GetCarrierThread extension function was introduced by loom for testing. > It's used by several tests in hotspot/jtreg/serviceability. > > Testings: tier1..tier6 Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19289#pullrequestreview-2069434194
Re: RFR: 8330694: Rename 'HeapRegion' to 'G1HeapRegion' [v12]
On Sat, 18 May 2024 09:07:18 GMT, Lei Zaakjyu wrote:
>> follow up 8267941
>
> Lei Zaakjyu has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> restore
Changes requested by cjplummer (Reviewer).
test/hotspot/jtreg/runtime/cds/appcds/sharedStrings/SharedStringsHumongous.java
line 90:
> 88: // before dumping the string table. That means the heap should
> contain no
> 89: // humongous regions.
> 90: dumpOutput.shouldNotMatch("gc,region,cds. G1HeapRegion
> 0x[0-9a-f]* HUM");
Just a minor nit. I noticed a pre-existing typo on line 87 above. It says
"kelp" instead of "kept". Can you fix it?
-
PR Review: https://git.openjdk.org/jdk/pull/18871#pullrequestreview-2067620880
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1607694956
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v8]
On Mon, 20 May 2024 04:25:49 GMT, David Holmes wrote: > Okay but how does this `dbgRawMonitor` lock prevent the GC from clearing a > jobject? The JNI ref. It's actually a global ref (I said local ref earlier). As long as any setters of DebugRawMonitor::ownerThread (setting the thread or clearing it) grab dbgRawMonitor first, and the current thread accessing ownerThread does the same, then you know that whatever ownerThread points to is either NULL, the current thread, or some other thread that is still alive and is actually holding the monitor that the DebugRawMonitor is encapsulating. As long as the current thread continues to hold dbgRawMonitor, no other thread will be able to change ownerThread. This means that if some other thread currently owns the monitor that the DebugRawMonitor is encapsulating, then it will first block on dbgRawMonitor before trying to release it and clear ownerThread. - PR Comment: https://git.openjdk.org/jdk/pull/19044#issuecomment-2119650743
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v8]
On Mon, 20 May 2024 01:47:31 GMT, David Holmes wrote: > That smells fishy to me. A thread can always safely check if it owns > something because it can't race with itself and get a wrong answer. Unfortunately checking ownership means comparing jobjects, which you can't safely do if you are comparing to a jobject that could be freed at any moment (I'm referring the JNI ref being freed, not the object being GC'd). I ran into asserts in JNI IsSameObject() due to this happening. Basically the local ref became invalid between the time it was fetched from the DebugRawMonitor and when is was used by IsSameObject(). - PR Comment: https://git.openjdk.org/jdk/pull/19044#issuecomment-2119592870
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v6]
On Fri, 17 May 2024 20:03:11 GMT, Alex Menkov wrote: >> Chris Plummer has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Flip rank order. Some cleanup and better comments for verifyMonitorRank(). > > src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1145: > >> 1143: */ >> 1144: >> 1145: static jrawMonitorID dbgRawMonitor; > > As the monitor is used to synchronize access to dbg_monitors array, maybe > rename it to something like dbg_monitors_lock? ok - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1605602835
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v8]
On Fri, 17 May 2024 19:52:00 GMT, Alex Menkov wrote: >> After calling getLocks(), there may still be attempts to enter locks. The >> locks should already be held. I filed >> [JDK-8330193](https://bugs.openjdk.org/browse/JDK-8330193) to assert that. >> However, even when entering an already entered lock, we still need to grab >> dbgRawMonitor. I found that out the hard way when there were deadlocks on >> dbgRawMonitor because it was not entered it here. > > I think expose dbgRawMonitor outside of util.c is wrong (and use > gdata->rankedMonitors outside of utils.c too). > If it's really required, it would be better to add functions to > disable/enable monitor locks. > But I still don't understand why we need this (and what is JDK-8330193 about) > Both JVMTI raw monitors and DebugRawMonitor support re-entrance, so this > thread may enter the locks again (and other threads will wait for the locks > if they try to enter them). > And I don't see how we can get deadlocks on dbgRawMonitor, could you please > elaborate on that. The comment above getLocks() pretty much explains it. Before doing any thread suspension we can't allow any other thread to hold a lock that might be needed during suspension. This is why getLocks() is used to grab all these locks in advanced. If they were not grabbed in advanced and one of the locks was held by another thread that got suspended, then eventually the current thread (the one that initiated the thread suspension) would block on the suspended thread, which means it would never get resumed, so we have a deadlock. If we don't include dbgRawMonitor in this set of locks and we suspend a thread while it holds it, this prevents the current thread from doing a debugMonitorEnter on any lock, even ones it already holds. Note we can't check if the current thread owns a lock without grabbing dbgRawMonitor. In fact the main purpose of dbgRawMonitor is to allow the current thread to check if it owns the monitor. I don't disagree that it's a bit of a wart that dbgRawMonitor is exposed in this manner. I just don't see a way around it. I can hide gdata->rankedMonitors in dbgRawMonitor_lock/unlock() if you want, but I can't see of way to not export dbgRawMonitor_lock/unlock() in some fashion, possibly with a different name. - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1605602137
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v8]
> This PR adds ranked monitor support to the debug agent. The debug agent has a > large number of monitors, and it's really hard to know which order to grab > them in, and for that matter which monitors might already be held at any > given moment. By imposing a rank on each monitor, we can check to make sure > they are always grabbed in the order of their rank. Having this in place when > I was working on [JDK-8324868](https://bugs.openjdk.org/browse/JDK-8324868) > would have made it much easier to detect a deadlock that was occuring, and > the reason for it. That's what motivated me to do this work > > There were 2 or 3 minor rank issues discovered as a result of these changes. > I also learned a lot about some of the more ugly details of the locking > support in the process. > > Tested with the following on all supported platforms and with virtual threads: > > com/sun/jdi > vmTestbase/nsk/jdi > vmTestbase/nsk/jdb > vmTestbase/nsk/jdwp > > Still need to run tier2 and tier5. > > Details of the changes follow in the first comment. Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: Fix a couple of minor typos in comments. - Changes: - all: https://git.openjdk.org/jdk/pull/19044/files - new: https://git.openjdk.org/jdk/pull/19044/files/c3bd1716..33fcea3e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19044=07 - incr: https://webrevs.openjdk.org/?repo=jdk=19044=06-07 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19044.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19044/head:pull/19044 PR: https://git.openjdk.org/jdk/pull/19044
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v7]
On Thu, 16 May 2024 17:37:30 GMT, Kevin Walls wrote: > Is that "then popFrameEventLock second" > Yes. I'll fix. > Drawing these out in two columns I can't see a deadlock either 8-) Ironically right now I'm looking at a very rare deadlock that involves this code. It doesn't seem to happen when I disabled ranked locking. It might be instigated by the dbgRawMonitor that ranked locking uses. - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1603965727
Re: RFR: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers
On Thu, 16 May 2024 07:57:58 GMT, Alan Bateman wrote: >> The following RFE was fixed recently: >> [8324680](https://bugs.openjdk.org/browse/JDK-8324680): Replace NULL with >> nullptr in JVMTI generated code >> >> It replaced all the `NULL`'s in the generated spec with`nullptr`. JVMTI >> agents can be developed in C or C++. >> This update is to make it clear that `nullptr` is C programming language >> `null` pointer. >> >> I think we do not need a CSR for this fix. >> >> Testing: N/A (not needed) > > src/hotspot/share/prims/jvmti.xml line 1008: > >> 1006: function descriptions. Empty lists, arrays, sequences, etc are >> 1007: returned as nullptr which is C programming language >> 1008: null pointer. > > Shouldn't this be "NULL"? In any case, I think it would be helpful to expand > this a bit to make it clear that usages of "nullptr" in parameter and error > descriptions should be read or treated as "NULL" when developing an agent in > C rather than C++. Yes, I think it should by NULL. - PR Review Comment: https://git.openjdk.org/jdk/pull/19257#discussion_r1603929609
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v7]
> This PR adds ranked monitor support to the debug agent. The debug agent has a > large number of monitors, and it's really hard to know which order to grab > them in, and for that matter which monitors might already be held at any > given moment. By imposing a rank on each monitor, we can check to make sure > they are always grabbed in the order of their rank. Having this in place when > I was working on [JDK-8324868](https://bugs.openjdk.org/browse/JDK-8324868) > would have made it much easier to detect a deadlock that was occuring, and > the reason for it. That's what motivated me to do this work > > There were 2 or 3 minor rank issues discovered as a result of these changes. > I also learned a lot about some of the more ugly details of the locking > support in the process. > > Tested with the following on all supported platforms and with virtual threads: > > com/sun/jdi > vmTestbase/nsk/jdi > vmTestbase/nsk/jdb > vmTestbase/nsk/jdwp > > Still need to run tier2 and tier5. > > Details of the changes follow in the first comment. Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: Simplify by getting rid of special logic around leaf monitors. - Changes: - all: https://git.openjdk.org/jdk/pull/19044/files - new: https://git.openjdk.org/jdk/pull/19044/files/1c6a2e34..c3bd1716 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19044=06 - incr: https://webrevs.openjdk.org/?repo=jdk=19044=05-06 Stats: 36 lines in 2 files changed: 3 ins; 23 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/19044.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19044/head:pull/19044 PR: https://git.openjdk.org/jdk/pull/19044
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Wed, 15 May 2024 21:38:54 GMT, Kevin Walls wrote: > > ...Is there any way to run jconsole in a way that would result in it > > passing a non-empty delegationSubjects, resulting in this issue still > > reproducing? > > I don't think there is, JConsole has a hard null in this call. Also I don't > see in JConsole any calls of getMBeanServerConnection with a Subject being > passed, that's the main gateway method to use the removed feature. > > If there was a way to use Subject Delegation with JConsole (or with anything > else), and you try to attach to a jdk-23, then that will fail with the > UnsupportedOperationException and that's what we want as the feature is gone. > Realistically it's a feature with no known usage as discussed in the > deprecation and removal changes. If jconsole is passing null, why is it triggering this exception? - PR Comment: https://git.openjdk.org/jdk/pull/19253#issuecomment-2113656022
Re: RFR: 8332327: Return _methods_jmethod_ids field back in VMStructs
On Wed, 15 May 2024 21:12:03 GMT, Andrei Pangin wrote: > The fix for [JDK-8313332](https://bugs.openjdk.org/browse/JDK-8313332) has > [removed](https://github.com/openjdk/jdk/commit/21867c929a2f2c961148f2cd1e79d672ac278d27#diff-7d448441e80a0b784429d5d8aee343fcb131c224b8ec7bc70ea636f78d561ecd > ) `InstanceKlass::_methods_jmethod_ids` field from VMStructs. > > This broke > [async-profiler](https://github.com/async-profiler/async-profiler/), since > the profiler needs this field to obtain jmethodID in some corner cases. > > There was no actual need for removal, as the field is still there in > InstanceKlass. So, I propose to return the field back to restore the broken > functionality of async-profiler. This is a no risk change, because it only > exports an offset of one field and does not affect the JVM otherwise. LGTM - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19254#pullrequestreview-2059047063
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Wed, 15 May 2024 16:59:59 GMT, Kevin Walls wrote: > Running JConsole from a previous JDK, and attaching to jdk-23 (after > [JDK-832](https://bugs.openjdk.org/browse/JDK-832): Remove the Java > Management Extension (JMX) Subject Delegation feature), the MBean tab is > blank. > > In javax/management/remote/rmi/RMIConnectionImpl.java: > addNotificationListener rejects a non-null delegationSubjects array, but > older JDKs will send such an array. It could accept the array, and only > reject/throw if it contains a non-null Subject (i.e. if an attempt to use > subject delegation is really happening). > > Manually testing JConsole, the MBean tab is fully populated and usable. Ok. So the older version of jconsole does pass the empty delegationSubjects, not null. Is there any way to run jconsole in a way that would result in it passing a non-empty delegationSubjects, resulting in this issue still reproducing? - PR Comment: https://git.openjdk.org/jdk/pull/19253#issuecomment-2113474704
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v7]
On Wed, 15 May 2024 20:29:17 GMT, Serguei Spitsyn wrote: >> The fix is to degrade virtual threads support in the JVM TI >> `GetObjectMonitorUsage` function so that it is specified to only return an >> owner when the owner is a platform thread. Also, virtual threads are not >> listed in the both `waiters` and `notify_waiters` lists returned in the >> `jvmtiMonitorUsage` structure. Java 19 re-specified a number of JVMTI >> functions and events for virtual threads, we missed this one. >> >> The main motivation for degrading it now is that the object monitor >> implementation is being updated to allow virtual threads unmount while >> owning monitors. It would add overhead to record monitor usage when >> freezing/unmount, overhead that couldn't be tied to a JVMTI capability as >> the capability can be enabled at any time. >> >> `GetObjectMonitorUsage` was broken for 20+ years >> ([8247972](https://bugs.openjdk.org/browse/JDK-8247972)) without bug reports >> so it seems unlikely that the function is widely used. Degrading it to only >> return an owner when the owner is a platform thread has no compatibility >> impact for tooling that uses it in conjunction with `HotSpot` thread dumps >> or `ThreadMXBean`. >> >> One other point about `GetObjectMonitorUsage` is that it pre-dates >> j.u.concurrent in Java 5 so it can't be used to get a full picture of the >> lock usage in a program. >> >> The specs of the impacted `JDWP ObjectReference.MonitorInfo` command and the >> JDI `ObjectReference` `ownerThread()`, `waitingThreads()` and `entryCount()` >> methods are updated to match the JVM TI spec. >> >> Also, please, review the related CSR and Release Note: >> - CSR: [8331422](https://bugs.openjdk.org/browse/JDK-8331422): degrade >> virtual thread support for GetObjectMonitorUsage >> - RN: [8331465](https://bugs.openjdk.org/browse/JDK-8331465): Release Note: >> degrade virtual thread support for GetObjectMonitorUsage >> >> Testing: >> - tested impacted and updated tests locally >> - tested with mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: UNDO: removed incorrect simplification that removed a tmp local > skipped Changes look good now. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19030#pullrequestreview-2059028269
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v4]
On Tue, 14 May 2024 23:56:14 GMT, Serguei Spitsyn wrote:
>> The fix is to degrade virtual threads support in the JVM TI
>> `GetObjectMonitorUsage` function so that it is specified to only return an
>> owner when the owner is a platform thread. Also, virtual threads are not
>> listed in the both `waiters` and `notify_waiters` lists returned in the
>> `jvmtiMonitorUsage` structure. Java 19 re-specified a number of JVMTI
>> functions and events for virtual threads, we missed this one.
>>
>> The main motivation for degrading it now is that the object monitor
>> implementation is being updated to allow virtual threads unmount while
>> owning monitors. It would add overhead to record monitor usage when
>> freezing/unmount, overhead that couldn't be tied to a JVMTI capability as
>> the capability can be enabled at any time.
>>
>> `GetObjectMonitorUsage` was broken for 20+ years
>> ([8247972](https://bugs.openjdk.org/browse/JDK-8247972)) without bug reports
>> so it seems unlikely that the function is widely used. Degrading it to only
>> return an owner when the owner is a platform thread has no compatibility
>> impact for tooling that uses it in conjunction with `HotSpot` thread dumps
>> or `ThreadMXBean`.
>>
>> One other point about `GetObjectMonitorUsage` is that it pre-dates
>> j.u.concurrent in Java 5 so it can't be used to get a full picture of the
>> lock usage in a program.
>>
>> The specs of the impacted `JDWP ObjectReference.MonitorInfo` command and the
>> JDI `ObjectReference` `ownerThread()`, `waitingThreads()` and `entryCount()`
>> methods are updated to match the JVM TI spec.
>>
>> Also, please, review the related CSR and Release Note:
>> - CSR: [8331422](https://bugs.openjdk.org/browse/JDK-8331422): degrade
>> virtual thread support for GetObjectMonitorUsage
>> - RN: [8331465](https://bugs.openjdk.org/browse/JDK-8331465): Release Note:
>> degrade virtual thread support for GetObjectMonitorUsage
>>
>> Testing:
>> - tested impacted and updated tests locally
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: 1. clarifications in JDWP and JDI spec; 2. clarifications in test
> comments.
Changes requested by cjplummer (Reviewer).
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1535:
> 1533: bool is_virtual =
> java_lang_VirtualThread::is_instance(thread_oop);
> 1534: if (is_virtual) {
> 1535: skipped++;
Do we really need to maintain `skipped`. Isn't not adding to `nWait` the same
as skipping?
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1583:
> 1581: assert(w != nullptr, "sanity check");
> 1582: if (java_lang_VirtualThread::is_instance(thread_oop)) {
> 1583: skipped++;
I don't think maintaining `skipped` does anything here.
-
PR Review: https://git.openjdk.org/jdk/pull/19030#pullrequestreview-2058882144
PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1602170079
PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1602171199
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v4]
On Wed, 1 May 2024 20:49:02 GMT, Chris Plummer wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: 1. clarifications in JDWP and JDI spec; 2. clarifications in test
>> comments.
>
> src/jdk.jdi/share/classes/com/sun/jdi/ObjectReference.java line 369:
>
>> 367:
>> 368: /**
>> 369: * Returns an {@link ThreadReference} for the platform thread, if
>> any,
>
> Pre-existing issue: It should be "a" not "an", but then in the `@return`
> section we are using "the", so maybe we should use similar wording here:
> `...the {@link ThreadReference} of the platform thread...`
The above comment has not been addressed yet. Should be "a ThreadReference" or
"the ThreadReference"
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1602163046
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v4]
On Tue, 14 May 2024 23:19:14 GMT, Serguei Spitsyn wrote: >> Okay, please, let me explain this one more time. >> The original comments before method `check()` calls describe the testing >> scenario (or configuration setup before the verifying check) but not the >> numbers expected to be returned by the JVMTI `GetObjectMonitorUsage`. For >> instance, if the testing scenario says: "count of threads waiting to enter: >> NUMBER_OF_ENTERING_THREADS" then it means there is a real number of these >> threads waiting to enter the monitor. And it does not matter if they are >> platform or virtual threads. They are really waiting to enter the monitor. >> However, the JVMTI `GetObjectMonitorUsage` won't include virtual threads >> into the returned results. >> >> Now, I'm suggesting to add the following header for comments before each >> `check()` method call: >> >> +// The numbers below describe the testing scenario, not the >> expected results. >> +// The expected numbers are different for virtual threads >> because >> +// they are not supported by JVMTI GetObjectMonitorUsage. >> >> Would it work for you (I've pushed an update)? > >> BTW, the "re-enter" comment should continue to be "i + 1". >> I'm not sure why it was changed to "expEntryCount()". > > It depends on what are we trying to describe. We either describe the testing > scenario (the number of threads doing something) or the expected results. I > understood that you wanted to describe the results instead of the scenario. > And then it becomes problematic to do so as you can see. Ok, I understand what you are saying now. I think your clarifying comment above helps a lot. - PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1602155030
Re: RFR: 8307778: com/sun/jdi/cds tests are not compatible with jtreg test factory
On Wed, 15 May 2024 05:59:56 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which removes the > `test/jdk/com/sun/jdi/cds/` tests from being problem listed when jtreg > launches these tests through a virtual thread? > > These tests aren't actually incompatible with virtual threads. The real issue > is that in https://bugs.openjdk.org/browse/JDK-8305608, a test infrastructure > class `test/jdk/com/sun/jdi/VMConnection.java` was changed to use the > `test.class.path` system property instead of the previously used > `test.classes`. > > That change missed out updating the `test/jdk/com/sun/jdi/cds/` tests to pass > along the classpath through the `test.class.path` system property. As a > result these tests still use the old `test.classes` system property to pass > around the test classpath and thus causes these tests to fail. The reason why > this only impacts virtual threads is noted by Chris in this JBS comment > https://bugs.openjdk.org/browse/JDK-8305608?focusedId=14572100=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14572100. > > The commit in this PR updates the `CDSJDITest` to pass along > `test.class.path` instead of `test.classes`. The `CDSJDITest` is only used in > the tests under `test/jdk/com/sun/jdi/cds/`, so nothing else should be > impacted by this change. > > I ran these changes against both launching with platform thread as well as > virtual thread and these tests now pass in both these cases. Changes look good. I think the CR needs a better title. Something that mentions "Virtual". Also, it's not the "test factory" we area talking about here. It is the "test thread factory". - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19244#pullrequestreview-2058820760
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Wed, 15 May 2024 16:59:59 GMT, Kevin Walls wrote: > Running JConsole from a previous JDK, and attaching to jdk-23 (after > [JDK-832](https://bugs.openjdk.org/browse/JDK-832): Remove the Java > Management Extension (JMX) Subject Delegation feature), the MBean tab is > blank. > > In javax/management/remote/rmi/RMIConnectionImpl.java: > addNotificationListener rejects a non-null delegationSubjects array, but > older JDKs will send such an array. It could accept the array, and only > reject/throw if it contains a non-null Subject (i.e. if an attempt to use > subject delegation is really happening). > > Manually testing JConsole, the MBean tab is fully populated and usable. I'm just trying to understand current and previous behavior of jconsole a bit better. It sounds like jconsole always passes a non-null `delegationSubjects`, which results in an UOE, and you've changed to code to accept it as long as the entries are all null, resulting in no UOE and jconsole properly displaying the mbean tab. But what I don't understand is why we are seeing this issue with jconsole when it appears it is already passes null for the `delegationSubjects` argument, even back as far as JDK 8u. - PR Comment: https://git.openjdk.org/jdk/pull/19253#issuecomment-2113277694
Re: RFR: 8332112: Update nsk.share.Log to don't print summary during VM shutdown hook [v2]
On Tue, 14 May 2024 22:19:19 GMT, Leonid Mesnik wrote:
>> The nsk.share.Log doing some cleanup and reporting errors in the cleanup
>> method. This method is supposed to be executed by finalizer originally.
>> However, now it is called only during shutdown hook.
>> The cleanup using Cleaner doesn't work. See
>> https://bugs.openjdk.org/browse/JDK-8330760
>>
>> The cleanup() method flush stream and print summary which should be already
>> printed by complain method.
>>
>> This cleanup is not necessary and printing summary usually is just disabled.
>> It is enabled if the test called 'complain' method. However, the error
>> should have been printed already in this method.
>>
>> So it would be simple to remove this cleanup and reduce usage of Finalizable
>> in vmTestbase tests.
>>
>> Note: The 'verboseOnErrorEnabled' is just not used.
>>
>> See isVerboseOnErrorEnabled.
>>
>> public boolean isVerboseOnErrorEnabled() {
>> return errorsSummaryEnabled;
>> }
>>
>>
>> Tested with by running tests with different combinations (tier4-7) and tier1.
>
> Leonid Mesnik has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixed after comments
Marked as reviewed by cjplummer (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/19209#pullrequestreview-2056579895
Re: RFR: 8332112: Update nsk.share.Log to don't print summary during VM shutdown hook
On Sun, 12 May 2024 21:34:41 GMT, Leonid Mesnik wrote:
> The nsk.share.Log doing some cleanup and reporting errors in the cleanup
> method. This method is supposed to be executed by finalizer originally.
> However, now it is called only during shutdown hook.
> The cleanup using Cleaner doesn't work. See
> https://bugs.openjdk.org/browse/JDK-8330760
>
> The cleanup() method flush stream and print summary which should be already
> printed by complain method.
>
> This cleanup is not necessary and printing summary usually is just disabled.
> It is enabled if the test called 'complain' method. However, the error should
> have been printed already in this method.
>
> So it would be simple to remove this cleanup and reduce usage of Finalizable
> in vmTestbase tests.
>
> Note: The 'verboseOnErrorEnabled' is just not used.
>
> See isVerboseOnErrorEnabled.
>
> public boolean isVerboseOnErrorEnabled() {
> return errorsSummaryEnabled;
> }
>
>
> Tested with by running tests with different combinations (tier4-7) and tier1.
Copyrights needs updating.
test/hotspot/jtreg/vmTestbase/nsk/share/Log.java line 587:
> 585: * print a warning message first.
> 586: */
> 587: private synchronized void printErrorsSummary() {
There is a comment above that still references this method.
-
PR Review: https://git.openjdk.org/jdk/pull/19209#pullrequestreview-2056296736
PR Review Comment: https://git.openjdk.org/jdk/pull/19209#discussion_r1600577870
Re: RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v3]
On Fri, 3 May 2024 10:42:45 GMT, Serguei Spitsyn wrote:
>> ...and there are also comments above with this issue.
>
>> expEnteringCount/expWaitingCount contain the tested patterns.
>
> I kind of disagree.
> Please, take look at the loop below:
>
> for (int i = 0; i < NUMBER_OF_WAITING_THREADS; i++) {
> expEnteringCount = isVirtual ? 0 : NUMBER_OF_ENTERING_THREADS
> + i + 1;
> expWaitingCount = isVirtual ? 0 : NUMBER_OF_WAITING_THREADS
> - i - 1;
> lockCheck.notify(); // notify waiting threads one by one
> // now the notified WaitingTask has to be blocked on the
> lockCheck re-enter
>
> // entry count: 1
> // count of threads waiting to enter:
> NUMBER_OF_ENTERING_THREADS
> // count of threads waiting to re-enter:i + 1
> // count of threads waiting to be notified:
> NUMBER_OF_WAITING_THREADS - i - 1
> check(lockCheck, expOwnerThread(), expEntryCount(),
> expEnteringCount,
> expWaitingCount);
> }
>
> The comment fixed as you suggest does not look useful anymore as the tested
> pattern is lost:
>
> // entry count: expOwnerThread()
> // count of threads waiting to enter: expEnteringCount
> // count of threads waiting to re-enter: expEntryCount()
> // count of threads waiting to be notified: expWaitingCount
> check(lockCheck, expOwnerThread(), expEntryCount(),
> expEnteringCount,
> expWaitingCount);
> }
>
>
> I understand your concern but your suggestion is not that good.
> We could remove these comments but the tested pattern will be thrown away
> with the comments.
> Would it help if we add clarifications that the comments are correct for
> platform threads only?
I don't understand the issue with the updated commented. It is precisely
telling you what the expected "count" values should be. If you leave the macros
in the comment, then the comment is wrong for virtual threads. If you want to
keep the macros in the comment, you need to add something like "... or 0 for
virtual threads".
BTW, the "re-enter" comment should continue to be "i + 1". I'm not sure why it
was changed to "expEntryCount()".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1600436569
Re: RFR: 8330066: HeapDumpPath and HeapDumpGzipLevel VM options do not mention HeapDumpBeforeFullGC and HeapDumpAfterFullGC [v2]
On Tue, 14 May 2024 01:53:20 GMT, Alex Menkov wrote: >> The fix updates descriptions of `HeapDumpPath`/`HeapDumpGzipLevel` and >> `HeapDumpBeforeFullGC`/`HeapDumpAfterFullGC`/`HeapDumpOnOutOfMemoryError` VM >> options > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > align Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19224#pullrequestreview-2054228792
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v6]
On Mon, 13 May 2024 21:47:32 GMT, Alex Menkov wrote: > If debugger attaches to the debuggee, detaches and re-attaches again, are the > monitors recreated again? > (with rankedMonitor you added an assert if `DebugRawMonitor::monitor` is not > null) Almost all of the debugMonitorCreate() calls seem to come (indirectly) from initialize(), which is only called once. cmdQueueLock, which is the one monitor we call debugMonitorDestroy() on, is called from debugLoop_run(), which is called whenever a debugger connection is initiated. This is the only case of creating, destroying, and then recreating. I wonder if there is some need for that. popFrameEventLock and popFrameProceedLock are created lazily when first needed. There is a check to make sure they are not recreated if already created. They are never destroyed. That seems to be it. So we pretty much create all monitors upon initialization and never recreate those that are created during initialization. With regard to the assert, this can only be an issue for cmdQueueLock, and since debugMonitorDestroy() is called on it (and that clears the needed fields), the assert shouldn't be triggered. - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1599160098
Re: RFR: 8332098: Add missing @ since tags to jdk.jdi [v2]
On Mon, 13 May 2024 19:30:31 GMT, Nizar Benalla wrote: >> Please review this simple change where I add "@ since" tags to the >> package-info file of the following packages >> >> com.sun.jdi >> com.sun.jdi.connect >> com.sun.jdi.connect.spi >> com.sun.jdi.event >> com.sun.jdi.request >> >> I used the unix grep command to find the oldest "@ since" in each package >> and used that value, as it's hard to get the source code of JDK 1-5 >> TIA > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > empty commit Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19200#pullrequestreview-2053753720
Re: RFR: 8332098: Add missing `@since` tags to `jdk.jdi`
On Sun, 12 May 2024 01:58:38 GMT, Nizar Benalla wrote: > Please review this simple change where I add `@since` tags to the > package-info file of the following packages > ```com.sun.jdi > com.sun.jdi.connect > com.sun.jdi.connect.spi > com.sun.jdi.event > com.sun.jdi.request > > I used the unix `grep` command to find the oldest `@since` in each package > and used that value, as it's hard to get the source code of JDK 1-5 > TIA Can you fix the summary of the CR to not have backwards quotes. This seems to cause git some grief, as can be seen where the CR is mentioned in the Issue section of the PR description. - PR Comment: https://git.openjdk.org/jdk/pull/19200#issuecomment-2108563473
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Thu, 9 May 2024 12:32:12 GMT, Serguei Spitsyn wrote:
>> But 2 and 1 are very different. You can call them both leaf violations, but
>> they are leaf violations for very different reasons, and 2 is more akin to a
>> rank violation than a leaf violation.
>>
>> I'm reversing the ranks and reworking the loop a bit (both the comments and
>> how the errors are reported). I'll try to post later tonight after testing
>> is done.
>
> This is updated function `assertOrderFailure()` to print "RANK ORDER" failure
> for the case #2 above:
>
> static void
> assertOrderFailure(jthread thread, DebugRawMonitorRank rank,
> DebugRawMonitor* dbg_monitor, char* msg)
> {
> char* threadName = getThreadName(thread);
> tty_message("DebugRawMonitor %s failure: (%s: %d > %d) for thread (%s)",
> msg, dbg_monitor->name, dbg_monitor->rank, rank, threadName);
>
> if (rank < FIRST_LEAF_DEBUG_RAW_MONITOR &&
> dbg_monitor ->rank >= FIRST_LEAF_DEBUG_RAW_MONITOR) {
> tty_message("DebugRawMonitor RANK ORDER failure: (%s: %d > %d) for
> thread (%s)",
> msg, dbg_monitor->name, dbg_monitor->rank, rank,
> threadName);
> }
> jvmtiDeallocate(threadName);
> JDI_ASSERT(JNI_FALSE);
> }
I've given some more thought to this "leaf monitor" concept and decided it's
not worth having around. The only thing that results in branding a monitor as a
leaf monitor is the fact that no additional monitors are locked while the leaf
monitor is held. This is not actually a requirement, but simply an observation
made for these monitors. If any additional locking was introduced while holding
a leaf monitor, that is not necessarily a bad thing. The list of leaf monitors
is simply what was left over at the bottom of the rankings after all monitors
that needed a higher rank were given one (and this was the result of an
iterative process of running tests and seeing what asserts were triggered
because of misranking some monitors). This list started out long, but after
fixing all the rank issues we ended up with just 4.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1598892053
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v6]
On Thu, 9 May 2024 21:40:42 GMT, David Holmes wrote: >> I guess there could be a race if one thread is destroying this monitor while >> another is trying to use it. Thus a thread could be doing something like a >> RawMonitorEnter in the middle of (or after) the monitor being destroyed. >> Fixing that would require holding dbgRawMonitor during RawMonitorEnter, and >> that would cause deadlocks all over the place. Also, this same potential >> issue exists already, but doesn't seem to ever arise. It seems we only call >> debugMonitorDestroy for cmdQueueLock. Not sure why that is. > > FWIW Deleting monitors is a tricky business and needs to be done with great > care. You have to ensure all threads using the monitor are completely done > with it. Simply locking it first to check it is "unused" is not sufficient. This is probably the reason that the debug agent only destroys one of the 20 or so monitors it creates. I'm not sure how it got to that point. It may have destroyed more at one point but there were issues, and only one survived destruction without issue. - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1598885045
Re: RFR: 8330694: Rename 'HeapRegion' to 'G1HeapRegion' [v10]
On Sun, 12 May 2024 03:04:45 GMT, Lei Zaakjyu wrote: > Should we also rename 'HeapRegionType' to 'G1HeapRegionType', then rename the > current 'G1HeapRegionType' to 'G1 HeapRegionTypeEnum'? For this PR the SA renames should match the hotspot renames. It looks like you have not renamed this in hotspot yet so it should not be renamed in SA either. - PR Comment: https://git.openjdk.org/jdk/pull/18871#issuecomment-2108362277
Re: RFR: 8330694: Rename 'HeapRegion' to 'G1HeapRegion' [v9]
On Sat, 11 May 2024 11:12:44 GMT, Lei Zaakjyu wrote: > > I noticed that the HeapRegionManager and HeapRegionClosure classes were not > > renamed (in the hotspot source). Is this intentional or an oversite? > > OK, I will do all the SA part here. However, I do think that the other > classes named 'HeapRegion*' in the hotspot source should be dealt with in > follow-up PRs. I think there was a misunderstanding here. I was not asking you to rename these in SA. I was asking why they were not renamed in hotspot. If you want to do them in a follow-up then that is ok, but both hotspot and SA should be done together. So that means either undoing the most recent SA change or applying the same rename change to hotspot. - PR Comment: https://git.openjdk.org/jdk/pull/18871#issuecomment-2108342113
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v6]
> This PR adds ranked monitor support to the debug agent. The debug agent has a > large number of monitors, and it's really hard to know which order to grab > them in, and for that matter which monitors might already be held at any > given moment. By imposing a rank on each monitor, we can check to make sure > they are always grabbed in the order of their rank. Having this in place when > I was working on [JDK-8324868](https://bugs.openjdk.org/browse/JDK-8324868) > would have made it much easier to detect a deadlock that was occuring, and > the reason for it. That's what motivated me to do this work > > There were 2 or 3 minor rank issues discovered as a result of these changes. > I also learned a lot about some of the more ugly details of the locking > support in the process. > > Tested with the following on all supported platforms and with virtual threads: > > com/sun/jdi > vmTestbase/nsk/jdi > vmTestbase/nsk/jdb > vmTestbase/nsk/jdwp > > Still need to run tier2 and tier5. > > Details of the changes follow in the first comment. Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: Flip rank order. Some cleanup and better comments for verifyMonitorRank(). - Changes: - all: https://git.openjdk.org/jdk/pull/19044/files - new: https://git.openjdk.org/jdk/pull/19044/files/301da7d5..1c6a2e34 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19044=05 - incr: https://webrevs.openjdk.org/?repo=jdk=19044=04-05 Stats: 98 lines in 2 files changed: 31 ins; 18 del; 49 mod Patch: https://git.openjdk.org/jdk/pull/19044.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19044/head:pull/19044 PR: https://git.openjdk.org/jdk/pull/19044
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Wed, 8 May 2024 23:48:08 GMT, Serguei Spitsyn wrote: >> Yes, the 2nd loop is a different check. That's why I said it also checks all >> the leaf monitors "but for a different reason". Your two loops do not flag >> a rank violation if both monitors are leafs, even if grabbed in the wrong >> order. It only flags the leaf violation. Your two checks will always catch >> any violation, it just a matter of whether my example (which is both a leaf >> and a rank violation) is flagged as a leaf violation or a rank violation (or >> even both could be indicated if we choose). Yours flags it as a leaf >> violation. My code flags it as a rank violation. Mine could flag both >> violations without any additional iterations. Your would need to iterate >> over the leaf monitors twice to detect if there is both a rank and a leaf >> violation. > > Okay, thanks. Please, let me list the variants with some analysis. > We have 3 variants: > 1. Both monitors are leaf: Entering any monitor while a leaf monitor has > been entered is a violation (`LEAF RULE` violation). The order does not > matter. Any order is unacceptable. > > 2. A non-leaf monitor is being entered while a leaf monitor has been entered: > This is also a `LEAF RULE` violation. This violation is at the same time > always a "RANK ORDER VIOLATION" for the non-leaf monitor. My view is there is > no need to report this "RANK ORDER VIOLATION" as it always coexists with the > `LEAF RULE` violation for non-leaf monitors. But if you insist on reporting > it additionally then it is not a problem to check and report it in the > `assertOrderFailure()` function. It has all needed info available for it. > > 3. A non-leaf monitor is being entered while a non-leaf monitor has been > entered: It is a case+report for "RANK ORDER VIOLATION". > > There is one more variant: > 4. A leaf monitor is being entered while a non-leaf monitor has been entered: > It is never a violation, so this variant is excluded from the list above. But 2 and 1 are very different. You can call them both leaf violations, but they are leaf violations for very different reasons, and 2 is more akin to a rank violation than a leaf violation. I'm reversing the ranks and reworking the loop a bit (both the comments and how the errors are reported). I'll try to post later tonight after testing is done. - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594847727
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Wed, 8 May 2024 22:40:26 GMT, Serguei Spitsyn wrote: >> If the following are all true >> - you are entering is a leaf monitor >> - the current thread has already entered a leaf monitor >> - the rank of the monitor you are entering is lower than the rank of the >> monitor already held >> >> Then you have both a rank order violation and a violation for entering a >> leaf monitor when you have already entered a leaf monitor. My current >> implementation will complain about the rank violation (although can easily >> be made to instead complain about the leaf violation or complain about >> both). Yours will complain about the leaf violation. To make yours instead >> complain about the rank violation, the first loop needs to also check all >> the leaf monitors. Since the 2nd loop also checks all the leaf monitors (but >> for a different reason), this means iterating over the leaf monitors twice. >> Complaining about both violations would also require iterating over the leaf >> monitors twice. > > Okay, thanks. >> Then you have both a rank order violation and a violation for entering a >> leaf monitor when you have already entered a leaf monitor. > > I kind of disagree with the second part of this statement. > My understanding is that the second loop does a little bit different check: > "Violation for entering ANY monitor when you have already entered a leaf > monitor." > Please, note that `rank` can be any rank including those with conditions: > >rank < FIRST_LEAF_DEBUG_RAW_MONITOR >rank >= FIRST_LEAF_DEBUG_RAW_MONITOR > > It seems to me that if the second rule (in my edition) is violated then the > first rule does not matter. > It means that the first rule is for both ranks from the range: > >0 <= rank < FIRST_LEAF_DEBUG_RAW_MONITOR > ``` Yes, the 2nd loop is a different check. That's why I said it also checks all the leaf monitors "but for a different reason". Your two loops do not flag a rank violation if both monitors are leafs, even if grabbed in the wrong order. It only flags the leaf violation. Your two checks will always catch any violation, it just a matter of whether my example (which is both a leaf and a rank violation) is flagged as a leaf violation or a rank violation (or even both could be indicated if we choose). Yours flags it as a leaf violation. My code flags it as a rank violation. Mine could flag both violations without any additional iterations. Your would need to iterate over the leaf monitors twice to detect if there is both a rank and a leaf violation. - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594784168
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Wed, 8 May 2024 20:10:15 GMT, Serguei Spitsyn wrote:
>> I think you are suggesting that all monitors be created up front during
>> util_intialize(). The debug agent in the past has always created them lazily
>> (and sometimes some of them end up never being created because they are not
>> needed). I don't really see a big advantage in creating them all up front.
>>
>> Alex's suggestion was a very different one, and was just a suggestion to
>> initialize all the DebugRawMonitor ranks up front rather than when the
>> jMonitorID is created, and the reason for the suggestion was to avoid having
>> to grab the dbgRawMonitor when setting the rank, but I wasn't convinced that
>> it actually allowed you to not grab the dbgRawMonitor.
>
> I just wanted to say that Alex's suggestion is in a similar direction.
> I do not see a big advantage to save just on creation of a few monitors. It
> is not a scalability problem as their number is constant. Creation of the
> monitors at initialization time is a simplification as we do not care about
> synchronization at this phase. Also, it is easier to understand because there
> is no interaction with other threads.
The init code would have to know the name of each monitor since it is no longer
being passed in. Something like the following might work:
static DebugRawMonitor dbg_monitors[] = {
{ NULL, "JDWP VM_DEATH Lock", vmDeathLockForDebugLoop_Rank, NULL, 0 },
{ NULL, "JDWP Callback Block", callbackBlock_Rank, NULL, 0 },
{ NULL, "JDWP Callback Lock", callbackLock_Rank, NULL, 0 },
...
};
And then the init() function can iterate over the array and allocate the
monitor for each entry. Note that this array needs to be kept in sync with the
DebugRawMonitorRank enum (same entries and in the same order). I can assert
during the initialization that the rank field for each entry is equal to its
array index and that the array size is NUM_DEBUG_RAW_MONITORS.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594777084
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Wed, 8 May 2024 21:44:37 GMT, Serguei Spitsyn wrote: >>> A "leaf" failure is more specific than a "rank order" failure, so it is >>> better to report it first. Each "leaf" failure is also a "rank order" >>> failure (AFAICS). >> >> It's not just a matter of which is reported first. Even if you swap the >> order of your two loops you get the same result. The problem is the "rank" >> loop does not check if any of the leaf monitors are already held. I think to >> fix that it would have to iterate up to NUM_DEBUG_RAW_MONITORS, which means >> there is overlap with the range that the "leaf" loop iterators over. > > In fact, I do not understand why reporting the "rank order" violation is > important what "leaf rank order" is violated. I feel that I'm missing > something. Let me think on it a little bit. If the following are all true - you are entering is a leaf monitor - the current thread has already entered a leaf monitor - the rank of the monitor you are entering is lower than the rank of the monitor already held Then you have both a rank order violation and a violation for entering a leaf monitor when you have already entered a leaf monitor. My current implementation will complain about the rank violation (although can easily be made to instead complain about the leaf violation or complain about both). Yours will complain about the leaf violation. To make yours instead complain about the rank violation, the first loop needs to also check all the leaf monitors. Since the 2nd loop also checks all the leaf monitors (but for a different reason), this means iterating over the leaf monitors twice. Complaining about both violations would also require iterating over the leaf monitors twice. - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594757398
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Wed, 8 May 2024 20:23:21 GMT, Serguei Spitsyn wrote: > A "leaf" failure is more specific than a "rank order" failure, so it is > better to report it first. Each "leaf" failure is also a "rank order" failure > (AFAICS). It's not just a matter of which is reported first. Even if you swap the order of your two loops you get the same result. The problem is the "rank" loop does not check if any of the leaf monitors are already held. I think to fix that it would have to iterate up to NUM_DEBUG_RAW_MONITORS, which means there is overlap with the range that the "leaf" loop iterators over. - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594593936
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Wed, 8 May 2024 15:47:09 GMT, Serguei Spitsyn wrote:
>> Chris Plummer has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Fix jcheck extra whitespace.
>> - Fix comment typo.
>
> src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1297:
>
>> 1295: // We must hold the dbgRawMonitor when calling verifyMonitorRank()
>> 1296:
>> 1297: // Iterate over all the monitors and makes sure we don't already
>> hold one that
>
> Nit: Typo: "makes sure" => "make sure".
Fixed.
> src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1343:
>
>> 1341: }
>> 1342: }
>> 1343: }
>
> Nit: I'd suggest to consider two separate loops instead of one, something
> like below:
>
>
> static void
> assertOrderFailure(jthread thread, DebugRawMonitorRank rank,
> DebugRawMonitor* dbg_monitor, char* msg)
> {
> char* threadName = getThreadName(thread);
> tty_message("DebugRawMonitor %s failure: (%s: %d > %d) for thread (%s)",
> msg, dbg_monitor->name, dbg_monitor->rank, rank, threadName);
> jvmtiDeallocate(threadName);
> JDI_ASSERT(JNI_FALSE);
> }
>
> static void
> verifyMonitorRank(JNIEnv *env, DebugRawMonitorRank rank, jthread thread)
> {
> DebugRawMonitorRank i;
>
> // 1. Verify the held monitor's rank is lower than the rank of the
> monitor we are trying to enter.
> for (i = rank + 1; i < FIRST_LEAF_DEBUG_RAW_MONITOR; i++) {
> DebugRawMonitor* dbg_monitor = _monitors[i];
> if (dbg_monitor->ownerThread != NULL &&
> isSameObject(env, dbg_monitor->ownerThread, thread)) {
> // the lower ranked monitor is already owned by this thread
> assertOrderFailure(thread, rank, dbg_monitor, "RANK ORDER");
> }
> }
> // 2. Verify there are no other leaf monitors owned but this thread.
> for (i = FIRST_LEAF_DEBUG_RAW_MONITOR; i < NUM_DEBUG_RAW_MONITORS; i++) {
> DebugRawMonitor* dbg_monitor = _monitors[i];
> if (i != rank && isSameObject(env, dbg_monitor->ownerThread, thread))
> {
> // the leaf ranked monitor is already owned by this thread
> assertOrderFailure(thread, rank, dbg_monitor, "LEAF RANK");
> }
> }
>
> I hope, this can be tweaked to adopt the `popFrame` locks correction.
The special popFrame check needs to go in the first loop only, so it shouldn't
be a problem or add any complexity that we don't already have.
One things to resolve is if we enter a regular monitor while holding a leaf
monitor, is this a "rank" failure or "leaf" failure. Currently in the code it
is a "rank" failure. Your change would make it a "leaf" failure.
I'm not sure why you added the "i != rank" check with the leaf check. We should
never be re-entering a leaf monitor. The same "dbg_monitor->ownerThread !=
NULL" check as in the first loop should be used.
assertOrderFailure() won't be able to print as descriptive of a message as what
I currently have.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594526975
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594525608
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v5]
> This PR adds ranked monitor support to the debug agent. The debug agent has a > large number of monitors, and it's really hard to know which order to grab > them in, and for that matter which monitors might already be held at any > given moment. By imposing a rank on each monitor, we can check to make sure > they are always grabbed in the order of their rank. Having this in place when > I was working on [JDK-8324868](https://bugs.openjdk.org/browse/JDK-8324868) > would have made it much easier to detect a deadlock that was occuring, and > the reason for it. That's what motivated me to do this work > > There were 2 or 3 minor rank issues discovered as a result of these changes. > I also learned a lot about some of the more ugly details of the locking > support in the process. > > Tested with the following on all supported platforms and with virtual threads: > > com/sun/jdi > vmTestbase/nsk/jdi > vmTestbase/nsk/jdb > vmTestbase/nsk/jdwp > > Still need to run tier2 and tier5. > > Details of the changes follow in the first comment. Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: Fix minor comment typo. - Changes: - all: https://git.openjdk.org/jdk/pull/19044/files - new: https://git.openjdk.org/jdk/pull/19044/files/049d3884..301da7d5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19044=04 - incr: https://webrevs.openjdk.org/?repo=jdk=19044=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19044.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19044/head:pull/19044 PR: https://git.openjdk.org/jdk/pull/19044
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v4]
On Wed, 8 May 2024 16:46:33 GMT, Serguei Spitsyn wrote: >> Chris Plummer has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Fix jcheck extra whitespace. >> - Fix comment typo. > > src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1117: > >> 1115: jrawMonitorID monitor; >> 1116: const char* name; >> 1117: DebugRawMonitorRank rank; > > Nit: Please, consider to add same comment for lines #1116-1117 as for the > line #1119 below. > It might be useful to move the field `ownerThread` before the other fields > (not sure about the `monitor` field), so the same comment can be shared by > multiple fields. I kind of don't want to distract from the point that ownerThread is protected by the dbgRawMonitor, and that is the main purpose of dbgRawMonitor. Once you have verified that ownerThread is the current thread, you can release the dbgRawMonitor and access the rank and name fields safely. There is a race issue with all the fields during debugMonitorCreate() and verifyMonitorRank(), which is why the debugMonitorCreate() must hold the dbgRawMonitor(). However, there is not a race with the fields (other than ownerThread) for the DebugRawMonitor passed into the debugMonitorXXX() APIs because they are not called until the DebugRawMonitor is done being initialized. So this means that debugMonitorXXX() can access the monitor, name, and rank fields without holding dbgRawMonitor, because they are guaranteed to be fully initialized by that point, and they don't change. ownerThread does change, so you must access it while holding the dbgRawMonitor. Technically speaking entryCount could also be changed by other threads, but we don't access it until we know the current thread owns the DebugRawMonitor, so it is safe to access (no other thread would modify or access it). To put this another way, the debugMonitorXXX() APIs can safely access most of the DebugRawMonitor fields for the DebugRawMonitor passed in because we know they are initialized by this point and don't change. ownerThread (and to some extent entryCount) are the exception. verifyMonitorRank() introduces additional synchronization issues because it iterates over all DebugRawMonitors, and some might be in the middle of creation, so some synchronization with the creation code is needed. However, I now realize that if we initialized all the monitors up front, then there would be no need to hold dbgRawMonitor during debugMonitorCreate(), although this does not allow for any improvements in verifyMonitorRank() because it still needs to hold the dbgRawMonitor due to accessing the ownerThread field. - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1594512889
