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

[David Holmes]

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.

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