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

[David Holmes]

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
>