RFR: 8146009: "pure virtual method called" with using new GC logging mechanism

[Marcus Larsson]

Hi David,

Thanks for looking at this.

On 10/10/2016 01:31 AM, David Holmes wrote:
> Hi Marcus,
>
> On 8/10/2016 12:26 AM, Marcus Larsson wrote:
>> Hi,
>>
>> Making another attempt to fix this issue.
>>
>> Summary:
>> The following patch resolves a problem where the VM would crash during
>> shutdown due to static log related memory being de-initialized before
>> the last use of the logging framework. The solution involves parts of
>> the Nifty Counter idiom [0] to control static initialization and
>> de-initialization of stdout's and stderr's LogOutputs. Both objects are
>> now allocated using placement new, and avoids destructor calls during
>> de-initialization. The LogStreamInitializer makes sure both objects are
>> initialized before first use.
>
> Exactly when will this initialization take place?

It will happen during static initialization. The initializer object will 
ensure it happens before any use of the outputs.

> You have:
>
> ! LogFileStreamInitializer::LogFileStreamInitializer() {
> !   if (Atomic::cmpxchg(1, &initialized, 0) == 0) {
>
> but on many platforms cmpxchg is handled via a stub and so the stub 
> generator needs to have been initialized. If you call before that you 
> will get a non-atomic implementation ... in which case no point having 
> this pretend to be thread-safe.

Hmm, good point. I think I might've been too defensive here actually. 
Static initialization should be single threaded so the atomics are 
unnecessary, I think.

>
> I think I've queried this before, but does this have to rely on static 
> initialization? It seems fragile and error prone.

We want logging available during static initialization, so we're kind of 
stuck with it. Possibly something we can improve in the future though.

Thanks,
Marcus

>
> Thanks,
> David
>
>> Because the LogOutput::Stdout/err pointers could no longer be kept in
>> LogOutput, I've replaced all usages of them with the new references
>> instead.
>>
>> The patch includes a regression test for this issue, contributed by
>> Michail Chernov.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~mlarsson/8146009/webrev.00
>>
>> Issue:
>> https://bugs.openjdk.java.net/browse/JDK-8146009
>>
>> Testing:
>> JPRT testset hotspot, included test on supported platforms.
>>
>> Thanks,
>> Marcus
>>
>> [0] https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Nifty_Counter