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

[Robbin Ehn]

Hi David,

Looks good.

Thanks, Robbin

On 5/3/19 4:30 AM, David Holmes wrote:
> 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
>>>