Fwd: RFR: 8233375: JFR emergency dump do not recover thread state

[David Holmes]

Correction ...

On 4/11/2019 12:11 pm, David Holmes wrote:
> On 4/11/2019 11:19 am, Yasumasa Suenaga wrote:
>> On 2019/11/04 7:38, David Holmes wrote:
>>> On 4/11/2019 2:22 am, Markus Gronlund wrote:
>>>> Hi Yasumasa,
>>>>
>>>> I think you can simplify it to something like this:
>>>>
>>>> http://cr.openjdk.java.net/~mgronlun/8233375/webrev/
>>>
>>> That is more like I had envisaged for this. Reusing existing 
>>> thread-state transition code is preferable to adding more custom code 
>>> that directly manipulates thread-state.
>>
>> I do not agree with this change.
>>
>> VMError::report_and_die() has "Thread* thread" in its arguments. So 
>> Thread::current() might be different with it.
> 
> Not sure what you mean. You only ever manipulate the thread state of the 
> current thread.
> 
>> In addition, ThreadInVMfromUnknown uses transition_from_native() to 
>> change the thread state.
>> It checks (and manipulates?) something which relates to safepoint.
> 
> Yes it does - which would be a problem if a safepoint (or handshake) 
> were pending. But the path through before_exit already has safepoint 
> checks when you acquire the BeforeExit_lock.

But that isn't relevant. The issue is we don't want a safepoint check on 
the report_and_die() path. So a custom transition helper is needed to 
avoid that.

David

> The main problem with the suggestion is it seems we may not be running 
> in a JavaThread:
> 
>   349   Thread* const thread = Thread::current();
>   350   if (thread->is_Watcher_thread()) {
> 
> so we can't use the existing thread-state helpers, unless we narrow the 
> scope (as you do) to after the check for the WatcherThread.
> 
> David
> -----
> 
>> Thus I added ThreadInVMForJFR to new my webrev.
> 
> Your change still seems overly complicated.
> 
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>>> Thanks,
>>> David
>>>
>>>> Thanks
>>>> Markus
>>>>
>>>> -----Original Message-----
>>>> From: Yasumasa Suenaga <suenaga at oss.nttdata.com>
>>>> Sent: den 2 november 2019 16:57
>>>> To: hotspot-jfr-dev at openjdk.java.net; 
>>>> hotspot-runtime-dev at openjdk.java.net
>>>> Cc: David Holmes <david.holmes at oracle.com>; yasuenag at gmail.com; 
>>>> Markus Gronlund <markus.gronlund at oracle.com>
>>>> Subject: Re: Fwd: RFR: 8233375: JFR emergency dump do not recover 
>>>> thread state
>>>>
>>>> Hi,
>>>>
>>>> Markus commented in JBS this change should be kept local to JFR.
>>>> So I updated webrev. Could you review it?
>>>>
>>>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8233375/webrev.01/
>>>>
>>>> This change passed all tests on submit repo 
>>>> (mach5-one-ysuenaga-JDK-8233375-1-20191102-1354-6373703).
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2019/11/01 18:41, Yasumasa Suenaga wrote:
>>>>> Forward to hotspot-runtime-dev.
>>>>>
>>>>> As David commented in JBS, it may need to be fixed in JFR code.
>>>>> But I'm not unclear why thread state is not recover.
>>>>>
>>>>> I'd like to hear about this from JFR folks.
>>>>> If it is just a bug in JFR, I will create a patch which recover it 
>>>>> in JFR code.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> -------- Forwarded Message --------
>>>>> Subject: RFR: 8233375: JFR emergency dump do not recover thread state
>>>>> Date: Fri, 1 Nov 2019 17:08:42 +0900
>>>>> From: Yasumasa Suenaga <suenaga at oss.nttdata.com>
>>>>> To: hotspot-jfr-dev at openjdk.java.net
>>>>> CC: yasuenag at gmail.com <yasuenag at gmail.com>
>>>>>
>>>>> Hi all,
>>>>>
>>>>> Please review this change:
>>>>>
>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8233375
>>>>>     webrev: 
>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8233375/webrev.00/
>>>>>
>>>>> If JFR is running when JVM crashes, JFR will dump data to 
>>>>> hs_err_pid<PID>.jfr .
>>>>> It would perform in prepare_for_emergency_dump().
>>>>> However this function transits thread state to "_thread_in_vm".
>>>>>
>>>>> This change has been tested on submit repo as 
>>>>> mach5-one-ysuenaga-JDK-8233375-20191101-0651-6334762.
>>>>> It failed at compiler/types/correctness/CorrectnessTest.java
>>>>> However this test is for JIT compiler, and related issue has been 
>>>>> reported as JDK-8225620.
>>>>> So I think this patch can go through.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa