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

[David Holmes]

Coleen, Robbin (anyone else)

Could you please review the final version of this. In summary the 
changes are:

- Add the VerifyBeforeExit logic, together with acquisition of the 
Heap_lock by the JavaThread before initiating the terminal safepoint.
- Add the missing LogConfiguration::finalize();
- Document why we can't perform XML termination processing for the xtty 
or IdealGraphPrinter (the comments are placed where the code would be if 
we could do it)

Thanks,
David

On 1/05/2019 1: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'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!)?
> 
> 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).
> 
> 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
>>