Re: [jdk21] RFR: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
On Wed, 5 Jul 2023 19:33:16 GMT, Serguei Spitsyn wrote: > Clean backport from mainline jdk repo to jdk21 for the fix of: > [8303086](https://bugs.openjdk.org/browse/JDK-8303086): SIGSEGV in > JavaThread::is_interp_only_mode() > > Testing: > - TBD: mach5 tiers 1-5 Thank you for review, Patricio! - PR Comment: https://git.openjdk.org/jdk21/pull/96#issuecomment-1622765842
Re: [jdk21] RFR: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
On Wed, 5 Jul 2023 19:33:16 GMT, Serguei Spitsyn wrote: > Clean backport from mainline jdk repo to jdk21 for the fix of: > [8303086](https://bugs.openjdk.org/browse/JDK-8303086): SIGSEGV in > JavaThread::is_interp_only_mode() > > Testing: > - TBD: mach5 tiers 1-5 Marked as reviewed by pchilanomate (Reviewer). - PR Review: https://git.openjdk.org/jdk21/pull/96#pullrequestreview-1515324386
[jdk21] RFR: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
Clean backport from mainline jdk repo to jdk21 for the fix of: [8303086](https://bugs.openjdk.org/browse/JDK-8303086): SIGSEGV in JavaThread::is_interp_only_mode() Testing: - TBD: mach5 tiers 1-5 - Commit messages: - Backport 971c2efb698065c65dcf7373d8c3027f58d5f503 Changes: https://git.openjdk.org/jdk21/pull/96/files Webrev: https://webrevs.openjdk.org/?repo=jdk21&pr=96&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303086 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk21/pull/96.diff Fetch: git fetch https://git.openjdk.org/jdk21.git pull/96/head:pull/96 PR: https://git.openjdk.org/jdk21/pull/96
Re: RFR: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
On Sun, 2 Jul 2023 22:39:46 GMT, David Holmes wrote:
>> The JVMTI function `SetEventNotificationMode` can set notification mode
>> globally (`event_thread == nullptr`) for all threads or for a specific
>> thread (`event_thread != nullptr`). To get a stable mount/unmount vision of
>> virtual threads a JvmtiVTMSTransitionDisabler helper object is created :
>> `JvmtiVTMSTransitionDisabler disabler(event_thread);`
>>
>> In a case if `event_thread == nullptr` the VTMS transitions are disabled for
>> all virtual thread,
>> otherwise they are disabled for a specific thread if it is virtual.
>> The call to `JvmtiEventController::set_user_enabled()` makes a call to
>> `recompute_enabled()` at the end of its work to do a required bookkeeping.
>> As part of this work, the `recompute_thread_enabled(state)` is called for
>> each thread from the `ThreadsListHandle`, not only for the given
>> `event_thread`:
>>
>> ThreadsListHandle tlh;
>> for (; state != nullptr; state = state->next()) {
>> any_env_thread_enabled |= recompute_thread_enabled(state);
>> }
>>
>> This can cause crashes as VTMS transitions for other virtual threads are
>> allowed.
>> Crashes are observed in this small function:
>>
>> bool is_interp_only_mode() {
>> return _thread == nullptr ? _saved_interp_only_mode != 0 :
>> _thread->is_interp_only_mode();
>> }
>>
>> In a case `_thread != nullptr` then the call needs to be executed:
>> `_thread->is_interp_only_mode()`.
>> But the filed `_thread` can be already changed to `nullptr` by a VTMS
>> transition.
>>
>> The fix is to always disable all transitions.
>> Thanks to Dan and Patricio for great analysis of this crash!
>>
>> Testing:
>> - In progress: mach5 tiers 1-6
>
> src/hotspot/share/prims/jvmtiEnv.cpp line 578:
>
>> 576: record_class_file_load_hook_enabled();
>> 577: }
>> 578: JvmtiVTMSTransitionDisabler disabler;
>
> An explanatory comment would have been good.
The fix has been already integrated.
I'll add a comment when there is a chance.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14728#discussion_r1253537846
Re: RFR: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
On Fri, 30 Jun 2023 11:27:58 GMT, Serguei Spitsyn wrote:
> The JVMTI function `SetEventNotificationMode` can set notification mode
> globally (`event_thread == nullptr`) for all threads or for a specific thread
> (`event_thread != nullptr`). To get a stable mount/unmount vision of virtual
> threads a JvmtiVTMSTransitionDisabler helper object is created :
> `JvmtiVTMSTransitionDisabler disabler(event_thread);`
>
> In a case if `event_thread == nullptr` the VTMS transitions are disabled for
> all virtual thread,
> otherwise they are disabled for a specific thread if it is virtual.
> The call to `JvmtiEventController::set_user_enabled()` makes a call to
> `recompute_enabled()` at the end of its work to do a required bookkeeping. As
> part of this work, the `recompute_thread_enabled(state)` is called for each
> thread from the `ThreadsListHandle`, not only for the given `event_thread`:
>
> ThreadsListHandle tlh;
> for (; state != nullptr; state = state->next()) {
> any_env_thread_enabled |= recompute_thread_enabled(state);
> }
>
> This can cause crashes as VTMS transitions for other virtual threads are
> allowed.
> Crashes are observed in this small function:
>
> bool is_interp_only_mode() {
> return _thread == nullptr ? _saved_interp_only_mode != 0 :
> _thread->is_interp_only_mode();
> }
>
> In a case `_thread != nullptr` then the call needs to be executed:
> `_thread->is_interp_only_mode()`.
> But the filed `_thread` can be already changed to `nullptr` by a VTMS
> transition.
>
> The fix is to always disable all transitions.
> Thanks to Dan and Patricio for great analysis of this crash!
>
> Testing:
> - In progress: mach5 tiers 1-6
src/hotspot/share/prims/jvmtiEnv.cpp line 578:
> 576: record_class_file_load_hook_enabled();
> 577: }
> 578: JvmtiVTMSTransitionDisabler disabler;
An explanatory comment would have been good.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14728#discussion_r1249852707
Re: RFR: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
On Fri, 30 Jun 2023 11:27:58 GMT, Serguei Spitsyn wrote:
> The JVMTI function `SetEventNotificationMode` can set notification mode
> globally (`event_thread == nullptr`) for all threads or for a specific thread
> (`event_thread != nullptr`). To get a stable mount/unmount vision of virtual
> threads a JvmtiVTMSTransitionDisabler helper object is created :
> `JvmtiVTMSTransitionDisabler disabler(event_thread);`
>
> In a case if `event_thread == nullptr` the VTMS transitions are disabled for
> all virtual thread,
> otherwise they are disabled for a specific thread if it is virtual.
> The call to `JvmtiEventController::set_user_enabled()` makes a call to
> `recompute_enabled()` at the end of its work to do a required bookkeeping. As
> part of this work, the `recompute_thread_enabled(state)` is called for each
> thread from the `ThreadsListHandle`, not only for the given `event_thread`:
>
> ThreadsListHandle tlh;
> for (; state != nullptr; state = state->next()) {
> any_env_thread_enabled |= recompute_thread_enabled(state);
> }
>
> This can cause crashes as VTMS transitions for other virtual threads are
> allowed.
> Crashes are observed in this small function:
>
> bool is_interp_only_mode() {
> return _thread == nullptr ? _saved_interp_only_mode != 0 :
> _thread->is_interp_only_mode();
> }
>
> In a case `_thread != nullptr` then the call needs to be executed:
> `_thread->is_interp_only_mode()`.
> But the filed `_thread` can be already changed to `nullptr` by a VTMS
> transition.
>
> The fix is to always disable all transitions.
> Thanks to Dan and Patricio for great analysis of this crash!
>
> Testing:
> - In progress: mach5 tiers 1-6
Patricio, Chris and Leonid, thank you for review.
-
PR Comment: https://git.openjdk.org/jdk/pull/14728#issuecomment-1615188480
Re: RFR: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
On Fri, 30 Jun 2023 11:27:58 GMT, Serguei Spitsyn wrote:
> The JVMTI function `SetEventNotificationMode` can set notification mode
> globally (`event_thread == nullptr`) for all threads or for a specific thread
> (`event_thread != nullptr`). To get a stable mount/unmount vision of virtual
> threads a JvmtiVTMSTransitionDisabler helper object is created :
> `JvmtiVTMSTransitionDisabler disabler(event_thread);`
>
> In a case if `event_thread == nullptr` the VTMS transitions are disabled for
> all virtual thread,
> otherwise they are disabled for a specific thread if it is virtual.
> The call to `JvmtiEventController::set_user_enabled()` makes a call to
> `recompute_enabled()` at the end of its work to do a required bookkeeping. As
> part of this work, the `recompute_thread_enabled(state)` is called for each
> thread from the `ThreadsListHandle`, not only for the given `event_thread`:
>
> ThreadsListHandle tlh;
> for (; state != nullptr; state = state->next()) {
> any_env_thread_enabled |= recompute_thread_enabled(state);
> }
>
> This can cause crashes as VTMS transitions for other virtual threads are
> allowed.
> Crashes are observed in this small function:
>
> bool is_interp_only_mode() {
> return _thread == nullptr ? _saved_interp_only_mode != 0 :
> _thread->is_interp_only_mode();
> }
>
> In a case `_thread != nullptr` then the call needs to be executed:
> `_thread->is_interp_only_mode()`.
> But the filed `_thread` can be already changed to `nullptr` by a VTMS
> transition.
>
> The fix is to always disable all transitions.
> Thanks to Dan and Patricio for great analysis of this crash!
>
> Testing:
> - In progress: mach5 tiers 1-6
Marked as reviewed by lmesnik (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/14728#pullrequestreview-1507763983
Re: RFR: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
On Fri, 30 Jun 2023 11:27:58 GMT, Serguei Spitsyn wrote:
> The JVMTI function `SetEventNotificationMode` can set notification mode
> globally (`event_thread == nullptr`) for all threads or for a specific thread
> (`event_thread != nullptr`). To get a stable mount/unmount vision of virtual
> threads a JvmtiVTMSTransitionDisabler helper object is created :
> `JvmtiVTMSTransitionDisabler disabler(event_thread);`
>
> In a case if `event_thread == nullptr` the VTMS transitions are disabled for
> all virtual thread,
> otherwise they are disabled for a specific thread if it is virtual.
> The call to `JvmtiEventController::set_user_enabled()` makes a call to
> `recompute_enabled()` at the end of its work to do a required bookkeeping. As
> part of this work, the `recompute_thread_enabled(state)` is called for each
> thread from the `ThreadsListHandle`, not only for the given `event_thread`:
>
> ThreadsListHandle tlh;
> for (; state != nullptr; state = state->next()) {
> any_env_thread_enabled |= recompute_thread_enabled(state);
> }
>
> This can cause crashes as VTMS transitions for other virtual threads are
> allowed.
> Crashes are observed in this small function:
>
> bool is_interp_only_mode() {
> return _thread == nullptr ? _saved_interp_only_mode != 0 :
> _thread->is_interp_only_mode();
> }
>
> In a case `_thread != nullptr` then the call needs to be executed:
> `_thread->is_interp_only_mode()`.
> But the filed `_thread` can be already changed to `nullptr` by a VTMS
> transition.
>
> The fix is to always disable all transitions.
> Thanks to Dan and Patricio for great analysis of this crash!
>
> Testing:
> - In progress: mach5 tiers 1-6
Marked as reviewed by cjplummer (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/14728#pullrequestreview-1507636034
Re: RFR: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
On Fri, 30 Jun 2023 11:27:58 GMT, Serguei Spitsyn wrote:
> The JVMTI function `SetEventNotificationMode` can set notification mode
> globally (`event_thread == nullptr`) for all threads or for a specific thread
> (`event_thread != nullptr`). To get a stable mount/unmount vision of virtual
> threads a JvmtiVTMSTransitionDisabler helper object is created :
> `JvmtiVTMSTransitionDisabler disabler(event_thread);`
>
> In a case if `event_thread == nullptr` the VTMS transitions are disabled for
> all virtual thread,
> otherwise they are disabled for a specific thread if it is virtual.
> The call to `JvmtiEventController::set_user_enabled()` makes a call to
> `recompute_enabled()` at the end of its work to do a required bookkeeping. As
> part of this work, the `recompute_thread_enabled(state)` is called for each
> thread from the `ThreadsListHandle`, not only for the given `event_thread`:
>
> ThreadsListHandle tlh;
> for (; state != nullptr; state = state->next()) {
> any_env_thread_enabled |= recompute_thread_enabled(state);
> }
>
> This can cause crashes as VTMS transitions for other virtual threads are
> allowed.
> Crashes are observed in this small function:
>
> bool is_interp_only_mode() {
> return _thread == nullptr ? _saved_interp_only_mode != 0 :
> _thread->is_interp_only_mode();
> }
>
> In a case `_thread != nullptr` then the call needs to be executed:
> `_thread->is_interp_only_mode()`.
> But the filed `_thread` can be already changed to `nullptr` by a VTMS
> transition.
>
> The fix is to always disable all transitions.
> Thanks to Dan and Patricio for great analysis of this crash!
>
> Testing:
> - In progress: mach5 tiers 1-6
Looks good to me!
Patricio
-
Marked as reviewed by pchilanomate (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14728#pullrequestreview-1507404209
RFR: 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
The JVMTI function `SetEventNotificationMode` can set notification mode
globally (`event_thread == nullptr`) for all threads or for a specific thread
(`event_thread != nullptr`). To get a stable mount/unmount vision of virtual
threads a JvmtiVTMSTransitionDisabler helper object is created :
`JvmtiVTMSTransitionDisabler disabler(event_thread);`
In a case if `event_thread == nullptr` the VTMS transitions are disabled for
all virtual thread,
otherwise they are disabled for a specific thread if it is virtual.
The call to `JvmtiEventController::set_user_enabled()` makes a call to
`recompute_enabled()` at the end of its work to do a required bookkeeping. As
part of this work, the `recompute_thread_enabled(state)` is called for each
thread from the `ThreadsListHandle`, not only for the given specific
`event_thread`.
This can cause crashes as VTMS transitions for other virtual threads are
allowed.
Crashes are observed in this small function:
bool is_interp_only_mode() {
return _thread == nullptr ? _saved_interp_only_mode != 0 :
_thread->is_interp_only_mode();
}
In a case `_thread != nullptr` then the call needs to be executed:
`_thread->is_interp_only_mode()`.
But the filed `_thread` can be already changed to `nullptr` by a VTMS
transition.
The fix is to always disable all transitions.
Thanks to Dan and Patricio for great analysis of this crash!
Testing:
- In progress: mach5 tiers 1-6
-
Commit messages:
- 8303086: SIGSEGV in JavaThread::is_interp_only_mode()
Changes: https://git.openjdk.org/jdk/pull/14728/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14728&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8303086
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/14728.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14728/head:pull/14728
PR: https://git.openjdk.org/jdk/pull/14728
