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

[David Holmes]

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
>>>
>