RFR (S): 8222534: VerifyBeforeExit is not honored when System.exit is called

[David Holmes]

On 7/05/2019 6:06 am, coleen.phillimore at oracle.com wrote:
> I looked at v3.  

Oops - sorry about the typo on the URL.

> I suppose there's more to learn about the exit paths, 
> but this solves the problem and looks good.

Thanks. I fixed a typo in the new comment. But will push this now.

David
-----

> Thanks,
> Coleen
> 
> On 5/4/19 2:09 AM, David Holmes wrote:
>> Updated webrev: http://cr.openjdk.java.net/~dholmes/8222534/webrev.v2/
>>
>> The historic setting of the thread state to _thread_in_vm is moved 
>> ahead of the taking of the heap_lock. That logic has been there since 
>> "exit at a safepoint" was introduced by JDK-4526887, and there's 
>> nothing to tell you how we might get there in a different state. To be 
>> safe I've left it and added a comment.
>>
>> Thanks,
>> David
>>
>> On 4/05/2019 9:45 am, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> On 4/05/2019 12:56 am, coleen.phillimore at oracle.com wrote:
>>>> On 4/30/19 11:42 PM, David Holmes wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> On 1/05/2019 7:59 am, coleen.phillimore at oracle.com wrote:
>>>>>>
>>>>>> David, Thank you for the writeup of the exit paths.
>>>>>>
>>>>>> The reason I added this test with JDK-9074355 is because taking 
>>>>>> the Heap_lock in destroy_vm prevented a crash in VerifyBeforeExit.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~coleenp/2019/8074355.06/webrev/test/hotspot/jtreg/runtime/Shutdown/ShutdownTest.java.html 
>>>>>>
>>>>>>
>>>>>> I wonder if you need to take the Heap_lock in the VM_Exit path as 
>>>>>> well.
>>>>>
>>>>> Yes you are right. Sorry I was focusing on the wrong aspect of that 
>>>>> discussion. I've updated the v2 webrev in place as java.cpp is now 
>>>>> back in it. I will also update the exit path document to show the 
>>>>> taking of the Heap_lock.
>>>>>
>>>>> http://cr.openjdk.java.net/~dholmes/8222534/webrev.v2/
>>>>>
>>>>> Re-testing in mach5 underway - though it probably won't be testing 
>>>>> VerifyBeforeExit.
>>>>
>>>> I thought this change would enable VerifyBeforeExit with all our 
>>>> testing because it's trueInDebug and now on the exit path?
>>>
>>> You are right - I forgot about trueInDebug.
>>>
>>>>>
>>>>> I'm assuming, based on the fact the destroy_JavaVM path doesn't 
>>>>> hang, that taking the Heap_lock can't succeed until there are no GC 
>>>>> safepoint ops queued, and conversely once taken no GC safepoint ops 
>>>>> will queue (or if they do they won't block trying to take the heap 
>>>>> lock!)?
>>>>>
>>>> I believe this is the effect.
>>>>
>>>> I'm having trouble with control flow.   In destroy_vm() the 
>>>> Heap_lock is taken after that JavaThread is removed from the thread 
>>>> list, so we have a _no_safepoint_check_flag.   In this place, from 
>>>> debugging the thread is on the threads list, so safepoint checking 
>>>> is ok.
>>>>
>>>> http://cr.openjdk.java.net/~dholmes/8222534/webrev.v2/src/hotspot/share/runtime/java.cpp.frames.html 
>>>>
>>>
>>> Not only okay but essential - you will get deadlocks if you try to 
>>> take without safepoint check (been there done that :) ). And yes this 
>>> is just a normal live JavaThread executing normally so it has to 
>>> check safepoints.
>>>
>>>>   536     VM_Exit op(code);
>>>>
>>>> Can this line be after the Heap_lock acquisition or does the 
>>>> Heap_lock make this operation wait until GC is done to queue? ie 
>>>> it's this line, and not this one that we're blocking GC for:
>>>>
>>>>   548     VMThread::execute(&op);
>>>
>>> We need the heap_lock before line #548 - which never returns. The 
>>> declaration of the VM_Exit op and the taking of the heap lock can 
>>> occur in either order.
>>>
>>>>
>>>> Also, aside.
>>>>
>>>>   546     if (thread->is_Java_thread())
>>>>   547 ((JavaThread*)thread)->set_thread_state(_thread_in_vm);
>>>>
>>>> Not your change but this is surprising.  I wonder why this is?
>>>
>>> VMThread::execute (and subsequent code) will expect it ... oh! Shoot! 
>>> So will trying to take the heap_lock :( I need to move that higher 
>>> up. I'm assuming that some of the paths to vm_exit may not have the 
>>> thread "in_vm" - or at least at some point in time there was such a 
>>> path. I'll do some archaeology on that.
>>>
>>> Thanks Coleen!
>>>
>>> David
>>>
>>>>
>>>>> This bug has turned out to be far more complex than originally 
>>>>> anticipated :) Seems there are numerous reasons why various actions 
>>>>> are not on the vm_exit() path! I only wish someone had documented 
>>>>> why they were missing in the first place. So I added a comment 
>>>>> about the xtty logging problem in java.cpp (now that it's back in 
>>>>> the change).
>>>>>
>>>>
>>>> Thank you for all the comments.  This code is full of land mines 
>>>> without them.
>>>>
>>>> Looks good aside from questions.
>>>>
>>>> Coleen
>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>>
>>>>>> Coleen
>>>>>>
>>>>>> On 4/29/19 11:54 PM, David Holmes wrote:
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8222534
>>>>>>> Webrev: http://cr.openjdk.java.net/~dholmes/8222534/webrev/
>>>>>>>
>>>>>>> Stefan noticed that VerifyBeforeExit was not honoured when a Java 
>>>>>>> application terminates via System.exit.
>>>>>>>
>>>>>>> Examination of the exit code sequences revealed four differences 
>>>>>>> between an exit due to the last non-daemon thread terminating 
>>>>>>> (handled via jni_DestroyJavaVM) and a call to System.exit() 
>>>>>>> (handled via JVM_Halt()). There are four missing actions on the 
>>>>>>> System.exit() path:
>>>>>>>
>>>>>>> - No processing of VerifyBeforeExit
>>>>>>> - No XML logging before exit
>>>>>>> - No LogConfiguration::finalize()
>>>>>>> - No IdealGraphPrinter::clean_up()
>>>>>>>
>>>>>>> The first three have now been added at the appropriate point. 
>>>>>>> VerifyBefore exit was the main omission. The compiler team (i.e. 
>>>>>>> Vladimir K.) indicated they'd also like the XML logging. And the 
>>>>>>> LogConfiguration::finalize while possibly not essential avoids 
>>>>>>> any doubt as to whether buffered log output may get lost.
>>>>>>>
>>>>>>> The IdealGraphPrinter::cleanup was deemed unnecessary due to the 
>>>>>>> fact the process is being blown away anyway.
>>>>>>>
>>>>>>> The bug report contains a lot of details on the exit sequences 
>>>>>>> including a side-by-side comparison in the attached pdf, showing 
>>>>>>> the relative positioning of each action and that the correct 
>>>>>>> order has been maintained.
>>>>>>>
>>>>>>> The vm_exit() code affects a number of "abrupt" exit paths in the 
>>>>>>> VM, not just JVM_Halt, and this is discussed in the bug report as 
>>>>>>> well. In short the addition of the missing actions should not 
>>>>>>> cause any issues.
>>>>>>>
>>>>>>> Testing:
>>>>>>>  - some manual checking of exit paths and whether new code was 
>>>>>>> executed
>>>>>>>  - all hotspot/jtreg/gc tests with -XX:+VerifyBeforeExit added
>>>>>>>  - mach5 tiers 1-3
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>
>>>>
>