Re: RFR: 8266593: vmTestbase/nsk/jvmti/PopFrame/popframe011 fails with "assert(java_thread == _state->get_thread()) failed: Must be" [v3]
On Thu, 18 Nov 2021 17:15:06 GMT, Daniel D. Daugherty wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> get rid of the checks in jvmti handshakes: java_thread->threadObj() == NULL > > src/hotspot/share/prims/jvmtiEnvBase.cpp line 1533: > >> 1531: return; /* JVMTI_ERROR_THREAD_NOT_ALIVE (default) */ >> 1532: } >> 1533: assert(java_thread == _state->get_thread(), "Must be"); > > This `assert()` is the site of the original test failure. I haven't yet > looked at the locations of the other changes. > > The `is_exiting()` check is made under the protection of the > `JvmtiThreadState_lock` so an unsuspended target thread that is > exiting cannot reach the point where the `_state` is updated to > clear the `JavaThread*` so we can't fail the `assert()` if the > `is_exiting()` check has returned `false`. Dan, Thank you for reviewing this! I'm not sure, I correctly understand you here. Are you saying that you agree with this change? In fact, the thread state can not be changed (and the assert fired) after the `is_exiting()` check is made even without `JvmtiThreadState_lock` protection because it is inside of a handshake execution. - PR: https://git.openjdk.java.net/jdk/pull/6440
Re: RFR: 8266593: vmTestbase/nsk/jvmti/PopFrame/popframe011 fails with "assert(java_thread == _state->get_thread()) failed: Must be" [v3]
On Thu, 18 Nov 2021 17:18:23 GMT, Daniel D. Daugherty
wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> get rid of the checks in jvmti handshakes: java_thread->threadObj() == NULL
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1401:
>
>> 1399: if (!self) {
>> 1400: if (!java_thread->is_suspended()) {
>> 1401: _result = JVMTI_ERROR_THREAD_NOT_SUSPENDED;
>
> I don't see an obvious reason for this `is_exiting()` check.
Okay. I see similar check in the `force_early_return()` function:
if (state == NULL) {
return JVMTI_ERROR_THREAD_NOT_ALIVE;
}
Would it better to replace it with this check instead? :
if (java_thread->is_exiting()) {
return JVMTI_ERROR_THREAD_NOT_ALIVE;
}
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1625:
>
>> 1623: return; /* JVMTI_ERROR_THREAD_NOT_ALIVE (default) */
>> 1624: }
>> 1625: assert(_state->get_thread() == java_thread, "Must be");
>
> The `assert()` on L1625 is subject to the same race as the original site.
> This `is_exiting()` check is made under the protection of the
> `JvmtiThreadState_lock` so it is sufficient to protect that `assert()`.
Okay, thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/6440
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Looks good.
-
Marked as reviewed by alanb (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: 8266593: vmTestbase/nsk/jvmti/PopFrame/popframe011 fails with "assert(java_thread == _state->get_thread()) failed: Must be" [v3]
On Thu, 18 Nov 2021 09:34:13 GMT, Serguei Spitsyn wrote: >> The test fails when the target JavaThread has is_exiting() status. In such a >> case the JvmtiExport::cleanup_thread(this) has already made a clean up of >> its jvmtiThreadState, so the JavaThread address returned by >> _state->get_thread() is 0xbabababababababa. >> The fix is to add a check for is_exiting() status into handshake closure >> do_thread() early. >> There following handshake closures are fixed by this update: >> - UpdateForPopTopFrameClosure >> - SetForceEarlyReturn >> - SetFramePopClosure > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > get rid of the checks in jvmti handshakes: java_thread->threadObj() == NULL Marked as reviewed by dcubed (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6440
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
LGTM
-
Marked as reviewed by rriggs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: 8266593: vmTestbase/nsk/jvmti/PopFrame/popframe011 fails with "assert(java_thread == _state->get_thread()) failed: Must be" [v3]
On Fri, 19 Nov 2021 10:14:23 GMT, Serguei Spitsyn wrote:
>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1401:
>>
>>> 1399: if (!self) {
>>> 1400: if (!java_thread->is_suspended()) {
>>> 1401: _result = JVMTI_ERROR_THREAD_NOT_SUSPENDED;
>>
>> I don't see an obvious reason for this `is_exiting()` check.
>
> Okay. I see similar check in the `force_early_return()` function:
>
> if (state == NULL) {
> return JVMTI_ERROR_THREAD_NOT_ALIVE;
> }
>
> Would it better to replace it with this check instead? :
>
> if (java_thread->is_exiting()) {
> return JVMTI_ERROR_THREAD_NOT_ALIVE;
> }
>
> Removing this check and keep the one inside the handshake would be even
> better.
>
> I would also add this line for symmetry with two other cases:
>
> + MutexLocker mu(JvmtiThreadState_lock);
> SetForceEarlyReturn op(state, value, tos);
My point is that I don't see why you added the `is_exiting()` check
since I don't see a race in that function, i.e., there's no `assert()` in
this function that you need to protect.
As for adding the `MutexLocker mu(JvmtiThreadState_lock)`, you'll
have to analyze and justify why you would need to add that lock grab
independent of this fix. I'm not seeing a bug there, but I haven't looked
very closely.
>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1533:
>>
>>> 1531: return; /* JVMTI_ERROR_THREAD_NOT_ALIVE (default) */
>>> 1532: }
>>> 1533: assert(java_thread == _state->get_thread(), "Must be");
>>
>> This `assert()` is the site of the original test failure. I haven't yet
>> looked at the locations of the other changes.
>>
>> The `is_exiting()` check is made under the protection of the
>> `JvmtiThreadState_lock` so an unsuspended target thread that is
>> exiting cannot reach the point where the `_state` is updated to
>> clear the `JavaThread*` so we can't fail the `assert()` if the
>> `is_exiting()` check has returned `false`.
>
> Dan,
> Thank you for reviewing this!
> I'm not sure, I correctly understand you here.
> Are you saying that you agree with this change?
> In fact, the thread state can not be changed (and the assert fired) after the
> `is_exiting()` check is made even without `JvmtiThreadState_lock` protection
> because it is inside of a handshake execution.
I agree with the `is_exiting()` check addition.
I forgot that we're executing a Handshake `doit()` function. So we have a couple
of reasons why an unsuspended target thread can't change from `!is_exiting()`
to `is_exiting()` while we are in this function.
-
PR: https://git.openjdk.java.net/jdk/pull/6440
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Marked as reviewed by lancea (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Marked as reviewed by darcy (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
src/java.base/share/classes/java/lang/Object.java line 481:
> 479: * system resources or to perform other cleanup.
> 480: *
> 481: * When running in a Java virtual machine in which finalization
> has been
Disabling finalization is not part of the Java SE spec. This paragraph
describes the implementation of HotSpot VM and I think it does not belong to
this javadoc.
-
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Fri, 19 Nov 2021 18:06:39 GMT, Mandy Chung wrote:
>> Here are the code changes for the "Deprecate finalizers in the standard Java
>> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
>> review.
>>
>> This change makes the indicated deprecations, and updates the API spec for
>> JEP 421. It also updates the relevant @SuppressWarning annotations.
>>
>> The CSR has been approved.
>> An automated test build+test run passes cleanly (FWIW :D ).
>
> src/java.base/share/classes/java/lang/Object.java line 481:
>
>> 479: * system resources or to perform other cleanup.
>> 480: *
>> 481: * When running in a Java virtual machine in which finalization
>> has been
>
> Disabling finalization is not part of the Java SE spec. This paragraph
> describes the implementation of HotSpot VM and I think it does not belong to
> this javadoc.
The coupling between this change and
[8276422](https://bugs.openjdk.java.net/browse/JDK-8276422) ("Add command-line
option to disable finalization") is probably tighter than would be ideal. That
[CSR](https://bugs.openjdk.java.net/browse/JDK-8276773) allows for finalization
to be disabled in the SE Platform Spec and the JLS.
-
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: 8266593: vmTestbase/nsk/jvmti/PopFrame/popframe011 fails with "assert(java_thread == _state->get_thread()) failed: Must be" [v3]
On Thu, 18 Nov 2021 09:34:13 GMT, Serguei Spitsyn wrote: >> The test fails when the target JavaThread has is_exiting() status. In such a >> case the JvmtiExport::cleanup_thread(this) has already made a clean up of >> its jvmtiThreadState, so the JavaThread address returned by >> _state->get_thread() is 0xbabababababababa. >> The fix is to add a check for is_exiting() status into handshake closure >> do_thread() early. >> There following handshake closures are fixed by this update: >> - UpdateForPopTopFrameClosure >> - SetForceEarlyReturn >> - SetFramePopClosure > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > get rid of the checks in jvmti handshakes: java_thread->threadObj() == NULL Dan, thank you for review! - PR: https://git.openjdk.java.net/jdk/pull/6440
Re: RFR: 8266593: vmTestbase/nsk/jvmti/PopFrame/popframe011 fails with "assert(java_thread == _state->get_thread()) failed: Must be" [v3]
On Thu, 18 Nov 2021 09:34:13 GMT, Serguei Spitsyn wrote: >> The test fails when the target JavaThread has is_exiting() status. In such a >> case the JvmtiExport::cleanup_thread(this) has already made a clean up of >> its jvmtiThreadState, so the JavaThread address returned by >> _state->get_thread() is 0xbabababababababa. >> The fix is to add a check for is_exiting() status into handshake closure >> do_thread() early. >> There following handshake closures are fixed by this update: >> - UpdateForPopTopFrameClosure >> - SetForceEarlyReturn >> - SetFramePopClosure > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > get rid of the checks in jvmti handshakes: java_thread->threadObj() == NULL David, Thank you for your questions. I'm not sure if all of them are resolved though. :) Please, let me know if it is the case. - PR: https://git.openjdk.java.net/jdk/pull/6440
Re: RFR: 8266593: vmTestbase/nsk/jvmti/PopFrame/popframe011 fails with "assert(java_thread == _state->get_thread()) failed: Must be" [v3]
On Thu, 18 Nov 2021 09:34:13 GMT, Serguei Spitsyn wrote: >> The test fails when the target JavaThread has is_exiting() status. In such a >> case the JvmtiExport::cleanup_thread(this) has already made a clean up of >> its jvmtiThreadState, so the JavaThread address returned by >> _state->get_thread() is 0xbabababababababa. >> The fix is to add a check for is_exiting() status into handshake closure >> do_thread() early. >> There following handshake closures are fixed by this update: >> - UpdateForPopTopFrameClosure >> - SetForceEarlyReturn >> - SetFramePopClosure > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > get rid of the checks in jvmti handshakes: java_thread->threadObj() == NULL Still good. Thumbs up from my side. - Marked as reviewed by mdoerr (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6440
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Marked as reviewed by mchung (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Fri, 19 Nov 2021 18:15:46 GMT, Brent Christian wrote:
>> src/java.base/share/classes/java/lang/Object.java line 481:
>>
>>> 479: * system resources or to perform other cleanup.
>>> 480: *
>>> 481: * When running in a Java virtual machine in which finalization
>>> has been
>>
>> Disabling finalization is not part of the Java SE spec. This paragraph
>> describes the implementation of HotSpot VM and I think it does not belong to
>> this javadoc.
>
> The coupling between this change and
> [8276422](https://bugs.openjdk.java.net/browse/JDK-8276422) ("Add
> command-line option to disable finalization") is probably tighter than would
> be ideal. That [CSR](https://bugs.openjdk.java.net/browse/JDK-8276773)
> allows for finalization to be disabled in the SE Platform Spec and the JLS.
Thanks for pointing out that the CSR for JDK-8276422 ("Add command-line option
to disable finalization") includes the change of the SE Platform Spec and JLS
to allow an implementation for finalization to be disabled. This makes senses
now.
-
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Marked as reviewed by serb (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/6465
RFR: 8265795: vmTestbase/nsk/jvmti/AttachOnDemand/attach022/TestDescription.java fails when running with JEP 416
The VMObjectAlloc jvmti event was not generated for objects created using MethodHanldle. The fix adds posting of the event into Unsafe_AllocateInstance. While fixing this bug I noticed that event is not posted in the intrinsics version for many functions where it is used. Including but not limited to clone(), invoke()m allocateInstance() and allocateUninitializedArray(). There are might be other intensified functions (not analogs JVM_ENTRY versions) that allocate objects without post events. I think it is needed to implement some common way to handle this and cover it in another issue. - Commit messages: - switch to collerctor. - test added. - update - fix Changes: https://git.openjdk.java.net/jdk/pull/6478/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6478&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265795 Stats: 148 lines in 4 files changed: 146 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6478.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6478/head:pull/6478 PR: https://git.openjdk.java.net/jdk/pull/6478
