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

[Marcus Larsson]

Hi Kim,

On 01/19/2016 08:25 PM, Kim Barrett wrote:
> On Jan 19, 2016, at 8:58 AM, Marcus Larsson <marcus.larsson at oracle.com> wrote:
>> Hi,
>>
>> Please review the following patch to fix an issue in UL causing the VM to crash during shutdown.
>>
>> The problem is that the static LogStdoutOutput is deinitialized before the last use of it (G1 concurrent thread tries to log very late during VM shutdown). The solution is to make sure neither LogStdoutOutput nor LogStderrOutput are deinitialized during the full lifetime of the VM. To accomplish this I've changed the storage from static objects to static pointers to heap instances that are allocated & initialized on first use [0]. These instances are never deleted and can always be used. Also updated LogConfiguration::finalize to disable all file outputs before deleting & freeing them.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~mlarsson/8146009/webrev.00/
>>
>> Issue:
>> https://bugs.openjdk.java.net/browse/JDK-8146009
>>
>> Testing:
>> - local runs of the reproducer (500+ iterations without crashing)
>> - JPRT
>>
>> Thanks,
>> Marcus
>>
>> [0]: https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use
> ------------------------------------------------------------------------------
> src/share/vm/logging/logOutput.cpp
>    32 LogOutput* LogOutput::stdout_output() {
>    33   static LogOutput* instance = new LogStdoutOutput();
>    34   return instance;
>    35 }
>    36
>    37 LogOutput* LogOutput::stderr_output() {
>    38   static LogOutput* instance = new LogStderrOutput();
>    39   return instance;
>    40 }
>
> Function scoped statics may not be thread-safe.
>
> They are thread-safe by default for gcc4.x (there's a compiler option
> to disable that).  I'm pretty sure they are not for older versions of
> Visual Studio, though C++11 requires thread-safe initialization so
> some relatively recent version of VS probably makes them so.  I don't
> know about Solaris or other platforms.

I don't think this will be a problem in this specific case. Both 
functions will always be called during early init when there's only a 
single thread around (LogConfiguration::initialize and LogTagSet 
constructor). I was actually able to limit the usage of these functions 
to only these two cases. I'll post a new webrev soon.

>
> ------------------------------------------------------------------------------
>
> The failure situation this change is attempting to address may still
> technically invoke undefined behavior with the change.  I think that
> referencing the function scoped static during exit processing may be
> theoretically problematic because the variable may have been
> destroyed.  It likely works despite that, since the destructor for a
> pointer-typed value probably trivially does nothing.
>
> This suggests that even thread-safe statics may not make the
> stdxxx_output functions strictly correct.

The functions return copies of the pointers, so no one will really have 
a reference to those specific pointers. The memory the pointers point to 
is also never freed. With the changes I mentioned above, no one will 
call these functions or look at that static data once initialization is 
complete. The functions will just be a mean to get predictable static 
initialization for stdout and stderr outputs.

>
> ------------------------------------------------------------------------------
>
> Why do we still have gc (or any other) threads running during a
> shutdown that does exit processing[1]?  That seems like the real bug
> here.  Shouldn't we either have stopped all threads before exit
> processing, or not be doing exit processing at all and instead be
> aborting?
>
> [1] By exit processing I mean the distinction between exit(3) et al
> and abort(3).

That's a fair point, and it could indeed be a bug itself. The reason I 
chose to fix the logging side of this is that I think it would be good 
if the log framework is available at all times, even right before the VM 
dies.

Thanks,
Marcus

> ------------------------------------------------------------------------------
>