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

[David Holmes]

Thanks Robbin!

David

On 3/05/2019 5:04 pm, Robbin Ehn wrote:
> 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
>>>>