Re: RFR: 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in java_lang_Thread::get_thread_status
On Tue, 3 Aug 2021 06:38:55 GMT, David Holmes wrote: >> I was thinking about the same. If we always use _threadObj() then there is >> no need to make sure the threadObj is still valid. > > I'm not quite sure what point is being made here. We don't have to do > anything to "make sure the threadObj is still valid" as we don't have any > code here that could expose a "naked oop". We only need/want to use a handle > when we have to ensure the oop is preserved across calls. That is not the > case here. I agree, it is not the case here. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4921
Re: RFR: 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in java_lang_Thread::get_thread_status
On Tue, 3 Aug 2021 06:11:37 GMT, Serguei Spitsyn wrote: >> src/hotspot/share/services/threadService.cpp line 879: >> >>> 877: // If thread is still attaching then threadObj will be NULL. >>> 878: _thread_status = threadObj == NULL ? JavaThreadStatus::NEW >>> 879: : >>> java_lang_Thread::get_thread_status(threadObj); >> >> I like the use of `JavaThreadStatus::NEW` here. >> >> Since L867 creates the _threadObj OopHandle, do you want to change both >> uses of `threadObj` to `_threadObj()`? Is that still how we fetch the oop >> from >> an OopHandle? Or even use `threadObj()`... Then we don't have to reason >> about whether the `threadObj` oop is still good... > > I was thinking about the same. If we always use _threadObj() then there is no > need to make sure the threadObj is still valid. I'm not quite sure what point is being made here. We don't have to do anything to "make sure the threadObj is still valid" as we don't have any code here that could expose a "naked oop". We only need/want to use a handle when we have to ensure the oop is preserved across calls. That is not the case here. - PR: https://git.openjdk.java.net/jdk/pull/4921
Re: RFR: 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in java_lang_Thread::get_thread_status
On Tue, 3 Aug 2021 06:12:35 GMT, Serguei Spitsyn wrote:
>> If a thread is attaching via JNI and has not yet created its Thread object
>> it can be caught in a ThreadSnapshot during a thread dump (VM_DumpThreads)
>> of all threads**, and the threadObj() will be NULL, so we can't pass it to
>> get_thread_status().
>>
>> ** JMM dumpThreads API
>>
>> The initial fix simply checks for a NULL threadObj() and reports thread
>> status NEW in that case.
>>
>> Alternatively we could filter the attaching thread out completely in
>> VM_DumpThreads::doit by expanding:
>>
>> if (jt->is_exiting() ||
>> jt->is_hidden_from_external_view()) {
>> // skip terminating threads and hidden threads
>> continue;
>> }
>>
>> to also check jt->is_attaching_via_jni().
>>
>> Note that higher-level code already filters out ThreadSnapshots with NULL
>> threadObj() anyway so we could go either way.
>>
>> Testing: manual hacks - see bug report.
>> - tier 1-3 sanity testing
>>
>> Thanks,
>> David
>
> Hi David,
> The fix looks fine to me.
> Thanks,
> Serguei
Thanks for the review @sspitsyn !
-
PR: https://git.openjdk.java.net/jdk/pull/4921
Re: RFR: 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in java_lang_Thread::get_thread_status
On Wed, 28 Jul 2021 15:17:19 GMT, Daniel D. Daugherty
wrote:
>> If a thread is attaching via JNI and has not yet created its Thread object
>> it can be caught in a ThreadSnapshot during a thread dump (VM_DumpThreads)
>> of all threads**, and the threadObj() will be NULL, so we can't pass it to
>> get_thread_status().
>>
>> ** JMM dumpThreads API
>>
>> The initial fix simply checks for a NULL threadObj() and reports thread
>> status NEW in that case.
>>
>> Alternatively we could filter the attaching thread out completely in
>> VM_DumpThreads::doit by expanding:
>>
>> if (jt->is_exiting() ||
>> jt->is_hidden_from_external_view()) {
>> // skip terminating threads and hidden threads
>> continue;
>> }
>>
>> to also check jt->is_attaching_via_jni().
>>
>> Note that higher-level code already filters out ThreadSnapshots with NULL
>> threadObj() anyway so we could go either way.
>>
>> Testing: manual hacks - see bug report.
>> - tier 1-3 sanity testing
>>
>> Thanks,
>> David
>
> src/hotspot/share/services/threadService.cpp line 879:
>
>> 877: // If thread is still attaching then threadObj will be NULL.
>> 878: _thread_status = threadObj == NULL ? JavaThreadStatus::NEW
>> 879: :
>> java_lang_Thread::get_thread_status(threadObj);
>
> I like the use of `JavaThreadStatus::NEW` here.
>
> Since L867 creates the _threadObj OopHandle, do you want to change both
> uses of `threadObj` to `_threadObj()`? Is that still how we fetch the oop from
> an OopHandle? Or even use `threadObj()`... Then we don't have to reason
> about whether the `threadObj` oop is still good...
I was thinking about the same. If we always use _threadObj() then there is no
need to make sure the threadObj is still valid.
-
PR: https://git.openjdk.java.net/jdk/pull/4921
Re: RFR: 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in java_lang_Thread::get_thread_status
On Wed, 28 Jul 2021 10:03:10 GMT, David Holmes wrote:
> If a thread is attaching via JNI and has not yet created its Thread object it
> can be caught in a ThreadSnapshot during a thread dump (VM_DumpThreads) of
> all threads**, and the threadObj() will be NULL, so we can't pass it to
> get_thread_status().
>
> ** JMM dumpThreads API
>
> The initial fix simply checks for a NULL threadObj() and reports thread
> status NEW in that case.
>
> Alternatively we could filter the attaching thread out completely in
> VM_DumpThreads::doit by expanding:
>
> if (jt->is_exiting() ||
> jt->is_hidden_from_external_view()) {
> // skip terminating threads and hidden threads
> continue;
> }
>
> to also check jt->is_attaching_via_jni().
>
> Note that higher-level code already filters out ThreadSnapshots with NULL
> threadObj() anyway so we could go either way.
>
> Testing: manual hacks - see bug report.
> - tier 1-3 sanity testing
>
> Thanks,
> David
Hi David,
The fix looks fine to me.
Thanks,
Serguei
-
Marked as reviewed by sspitsyn (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4921
Re: RFR: 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in java_lang_Thread::get_thread_status
On Wed, 28 Jul 2021 10:03:10 GMT, David Holmes wrote:
> If a thread is attaching via JNI and has not yet created its Thread object it
> can be caught in a ThreadSnapshot during a thread dump (VM_DumpThreads) of
> all threads**, and the threadObj() will be NULL, so we can't pass it to
> get_thread_status().
>
> ** JMM dumpThreads API
>
> The initial fix simply checks for a NULL threadObj() and reports thread
> status NEW in that case.
>
> Alternatively we could filter the attaching thread out completely in
> VM_DumpThreads::doit by expanding:
>
> if (jt->is_exiting() ||
> jt->is_hidden_from_external_view()) {
> // skip terminating threads and hidden threads
> continue;
> }
>
> to also check jt->is_attaching_via_jni().
>
> Note that higher-level code already filters out ThreadSnapshots with NULL
> threadObj() anyway so we could go either way.
>
> Testing: manual hacks - see bug report.
> - tier 1-3 sanity testing
>
> Thanks,
> David
Still looking for a serviceability review please - @sspitsyn , @plummercj ?
Thanks
-
PR: https://git.openjdk.java.net/jdk/pull/4921
Re: RFR: 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in java_lang_Thread::get_thread_status
On Wed, 28 Jul 2021 14:22:07 GMT, Thomas Stuefe wrote:
>> If a thread is attaching via JNI and has not yet created its Thread object
>> it can be caught in a ThreadSnapshot during a thread dump (VM_DumpThreads)
>> of all threads**, and the threadObj() will be NULL, so we can't pass it to
>> get_thread_status().
>>
>> ** JMM dumpThreads API
>>
>> The initial fix simply checks for a NULL threadObj() and reports thread
>> status NEW in that case.
>>
>> Alternatively we could filter the attaching thread out completely in
>> VM_DumpThreads::doit by expanding:
>>
>> if (jt->is_exiting() ||
>> jt->is_hidden_from_external_view()) {
>> // skip terminating threads and hidden threads
>> continue;
>> }
>>
>> to also check jt->is_attaching_via_jni().
>>
>> Note that higher-level code already filters out ThreadSnapshots with NULL
>> threadObj() anyway so we could go either way.
>>
>> Testing: manual hacks - see bug report.
>> - tier 1-3 sanity testing
>>
>> Thanks,
>> David
>
> This looks good to me.
>
> ..Thomas
Thanks for the reviews @tstuefe and @dcubed-ojdk !
-
PR: https://git.openjdk.java.net/jdk/pull/4921
Re: RFR: 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in java_lang_Thread::get_thread_status
On Wed, 28 Jul 2021 10:03:10 GMT, David Holmes wrote:
> If a thread is attaching via JNI and has not yet created its Thread object it
> can be caught in a ThreadSnapshot during a thread dump (VM_DumpThreads) of
> all threads**, and the threadObj() will be NULL, so we can't pass it to
> get_thread_status().
>
> ** JMM dumpThreads API
>
> The initial fix simply checks for a NULL threadObj() and reports thread
> status NEW in that case.
>
> Alternatively we could filter the attaching thread out completely in
> VM_DumpThreads::doit by expanding:
>
> if (jt->is_exiting() ||
> jt->is_hidden_from_external_view()) {
> // skip terminating threads and hidden threads
> continue;
> }
>
> to also check jt->is_attaching_via_jni().
>
> Note that higher-level code already filters out ThreadSnapshots with NULL
> threadObj() anyway so we could go either way.
>
> Testing: manual hacks - see bug report.
> - tier 1-3 sanity testing
>
> Thanks,
> David
Nice catch!
I'm always happy when we find a reason for one of these 24 hour
stress test failures.
Thumbs up! Your call on whether to change the code as I mentioned
in my other comment.
@sspitsyn - It would be great to have a Serviceability person chime in on this
review...
src/hotspot/share/services/threadService.cpp line 879:
> 877: // If thread is still attaching then threadObj will be NULL.
> 878: _thread_status = threadObj == NULL ? JavaThreadStatus::NEW
> 879: :
> java_lang_Thread::get_thread_status(threadObj);
I like the use of `JavaThreadStatus::NEW` here.
Since L867 creates the _threadObj OopHandle, do you want to change both
uses of `threadObj` to `_threadObj()`? Is that still how we fetch the oop from
an OopHandle? Or even use `threadObj()`... Then we don't have to reason
about whether the `threadObj` oop is still good...
-
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4921
Re: RFR: 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in java_lang_Thread::get_thread_status
On Wed, 28 Jul 2021 10:03:10 GMT, David Holmes wrote:
> If a thread is attaching via JNI and has not yet created its Thread object it
> can be caught in a ThreadSnapshot during a thread dump (VM_DumpThreads) of
> all threads**, and the threadObj() will be NULL, so we can't pass it to
> get_thread_status().
>
> ** JMM dumpThreads API
>
> The initial fix simply checks for a NULL threadObj() and reports thread
> status NEW in that case.
>
> Alternatively we could filter the attaching thread out completely in
> VM_DumpThreads::doit by expanding:
>
> if (jt->is_exiting() ||
> jt->is_hidden_from_external_view()) {
> // skip terminating threads and hidden threads
> continue;
> }
>
> to also check jt->is_attaching_via_jni().
>
> Note that higher-level code already filters out ThreadSnapshots with NULL
> threadObj() anyway so we could go either way.
>
> Testing: manual hacks - see bug report.
> - tier 1-3 sanity testing
>
> Thanks,
> David
This looks good to me.
..Thomas
-
Marked as reviewed by stuefe (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4921
RFR: 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in java_lang_Thread::get_thread_status
If a thread is attaching via JNI and has not yet created its Thread object it
can be caught in a ThreadSnapshot during a thread dump (VM_DumpThreads) of all
threads**, and the threadObj() will be NULL, so we can't pass it to
get_thread_status().
** JMM dumpThreads API
The initial fix simply checks for a NULL threadObj() and reports thread status
NEW in that case.
Alternatively we could filter the attaching thread out completely in
VM_DumpThreads::doit by expanding:
if (jt->is_exiting() ||
jt->is_hidden_from_external_view()) {
// skip terminating threads and hidden threads
continue;
}
to also check jt->is_attaching_via_jni().
Note that higher-level code already filters out ThreadSnapshots with NULL
threadObj() anyway so we could go either way.
Testing: manual hacks - see bug report.
- tier 1-3 sanity testing
Thanks,
David
-
Commit messages:
- 8269934: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in
java_lang_Thread::get_thread_status
Changes: https://git.openjdk.java.net/jdk/pull/4921/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4921=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8269934
Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/4921.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4921/head:pull/4921
PR: https://git.openjdk.java.net/jdk/pull/4921
