[9] RFR(S): 8150441: CompileTask::print_impl() is broken after JDK-8146905

[Tobias Hartmann]

Hi David,

thanks for having a look!

On 23.02.2016 13:37, David Holmes wrote:
> Hi Tobias,
> 
> On 23/02/2016 8:19 PM, Tobias Hartmann wrote:
>> Hi,
>>
>> please review the following patch.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8150441
>> http://cr.openjdk.java.net/~thartmann/8150441/webrev.00/
>>
>> The fix for JDK-8146905 [1] removed staticBufferStream and instead passes VMError::out/log to CompileTask::print_line_on_error() to print the current compile task if an error occurs. The problem is that fdStream VMError::out/log does not initialize the TimeStamp outputStream::_stamp and we hit the "must not be clear" assert in TimeStamp::milliseconds() which is called from CompileTask::print_impl(). Before, the TimeStamp was initialized in the staticBufferStream constructor [2].
> 
> I find the original code difficult to follow.

In the original code, the staticBufferStream constructor initializes "_stamp" which is inherited from outputStream [2].

 staticBufferStream sbs(buffer, sizeof(buffer), &out);
 report(&sbs, false);

The staticBufferStream is passed on to
-> VMError::report(outputStream* st, ..)
-> CompileTask::print_line_on_error(..)
-> CompileTask::print(..)
-> CompileTask::print_impl(..)
which then invokes st->time_stamp().milliseconds() with st->time_stamp()._counter == 0.

>  The staticBufferStream did initialize its own stamp, but AFAICS that stamp is never used in relation to the wrapped outer-stream, so somehow that stamp must have been exposed through one of the methods that didn't delegate to the outer stream ??

Yes, the stamp initialized by staticBufferStream is exposed through st->time_stamp() which does not delegate to the outer stream.

>> The time stamps should be explicitly initialized like we do in ostream_init(). I verified that my patch solves the problem.
> 
> Why is the timestamp of a stream not initialized when the stream is constructed? Why are we deferring it until report_and_die() ?

I'm not sure about this but I think we could eagerly initialize the timestamp in fdStream::fdStream() or outputStream::outputStream().

Thanks,
Tobias

> 
> Thanks,
> David
> 
>> Thanks,
>> Tobias
>>
>> [1] http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/64ba9950558b
>> [2] http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/64ba9950558b#l1.102
>>