RFR 8211821: PrintStringTableStatistics crashes JVM

[David Holmes]

Hi Harold,

On 11/10/2018 7:42 AM, David Holmes wrote:
> Hi Harold,
> 
> On 11/10/2018 6:44 AM, Harold David Seigel wrote:
>> Hi,
>>
>> Please review this fix for JDK-8211821.  The change fixes the problem 
>> by moving the call to exit_globals() in Threads::destroy_vm() to 
>> before the thread gets deleted.
> 
> I need to give that some very careful thought - this stuff is very 
> fragile and a simple change can have unexpected consequences.
> 
> Thanks,
> David
> 
>    Although exit_globals() deletes tty, this is not a
>> problem because neither deleting thread nor freeing the 
>> jvmci_old_thread_counters use tty.

A lot can happen during thread destruction however and it was far from 
obvious that no logging or print/trace-flag flag might not result in use 
of tty during that process. I couldn't see anything obvious in relation 
to print/trace flags and I tested quite a few, and logging seems okay. 
But there could still be an issue lurking which we won't spot until 
someone uses the flag. Obviously we don't have tests that cover use of 
every flag (else we wouldn't have introduced this bug!).

I was also worried about what else exit_globals might do as in 
./share/runtime/java.cpp we have

void vm_perform_shutdown_actions() {
   // Warning: do not call 'exit_globals()' here. All threads are still 
running.
   // Calling 'exit_globals()' will disable thread-local-storage and 
cause all
   // kinds of assertions to trigger in debug mode.

But exit_globals() no longer touches thread-local-storage. In fact 
digging deeper it stopped doing that in January 2002 - bug "4526887 do 
not disable thread-local-storage during vm shutdown". :D

So I think we don't have to worry about the effects of exit_globals 
other than the ostream_exit logic. And that seems to be okay. So until 
proven otherwise I'm willing to assume this change is okay.

But could you please also delete that archaic comment in java.cpp - 
thanks :)

Thanks,
David
-----

>>
>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8211821/webrev/
>>
>> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8211821
>>
>> The fix was tested by running the new test and Mach5 tiers 1 and 2 
>> tests and builds on Linux-x64, Windows, and Mac OS X, running tiers 
>> 3-5 tests on Linux-x64, and by running JCK-12 API, Lang and VM tests 
>> on Linux-x64.
>>
>> Thanks, Harold
>>