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

[coleen.phillimore at oracle.com]

I looked at v3.  I suppose there's more to learn about the exit paths, 
but this solves the problem and looks good.
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
>>>>>
>>>