RFR 8211821: PrintStringTableStatistics crashes JVM

[Harold David Seigel]

Hi David,

Thanks for taking a deep look into this change.  I thought about moving 
exit_globals()'s call to ostream_exit() into its own function that would 
get called independently of exit_globals(), but that got a bit messy 
because of the destructorsCalled flag and because exit_globals() gets 
called in multiple places.

I'll delete the java.cpp comment before pushing the change.

Thanks again for the review!

Harold


On 10/11/2018 5:03 AM, David Holmes wrote:
> 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
>>>