RFR(xxs): 8145410: OutputStream::fill_to() does not work for error log

[Thomas Stüfe]

Hi David,

On Wed, Dec 16, 2015 at 8:32 AM, David Holmes <david.holmes at oracle.com>
wrote:

> On 16/12/2015 5:26 PM, Thomas Stüfe wrote:
>
>> Hi David,
>>
>>
>> On Wed, Dec 16, 2015 at 7:46 AM, David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>>     Hi Thomas,
>>
>>     I'll need a second opinion on this one :)
>>
>>     On 16/12/2015 12:59 AM, Thomas Stüfe wrote:
>>
>>         Hi all,
>>
>>         please review and sponsor this tiny change.
>>
>>         outputStream::fill_to() does not work as expected for the
>>         staticBufferStream because its position never gets updated. That
>>         means that
>>         fill_to() does not work in error reporting.
>>
>>
>>     Okay I think I see what you are doing with the call to
>>     update_position, but shouldn't all staticBufferStream methods be
>>     delegating to the _outer_stream? I confess I'm unclear the roles of
>>     the buffer and the outer_stream in this abstraction.
>>
>>
>> I do not really like this delegation coding, but it is as it is for now.
>> If we wanted to relegate all functionality to the inner stream, we would
>> have to overwrite more functions in the staticBufferStream - any
>> functions really which use one of the _position, _newlines, _precount
>> members - to not use the ones from the outer class but the one from the
>> inner class.
>>
>> But I also think that delegation is a bad fit here. The only reason
>> staticBufferStream exists is because it uses a user-provided buffer as
>> scratch-buffer for do_vsnprintf() to prevent large stack allocations.
>> The comment is also misleading, it was difficult at first to tell apart
>> the difference between staticBufferStream, stringStream and
>> bufferedStream.
>>
>> For bufferedStream and stringStream "buffer" means the real output
>> buffer, not the scratch buffer for vsnprintf. Otherwise I am hazy on the
>> difference between those two - the only difference I could spot is that
>> bufferedStream seems to use C-Heap, stringStream uses resource area
>> buffer. Both cases support growing on demand if needed.
>>
>> I think the scratch buffer management would be way better incorporated
>> into outputStream itself. For instance, provide methods to provide the
>> class with a user provided scratch buffer - and if none is given, use
>> stack space. That would get rid completely of staticBufferStream.
>>
>> There is a lot of redundant coding in ostream.cpp, and this would
>> benefit from cleaning up.
>>
>> But I think my change, for now, is the least invasive fix without larger
>> redesign.
>>
>
> Okay. The only existing use of staticBufferStream is in the vmError
> routines. Does this change impact though usages at all, or does it only
> affect uses of fill_to ?
>
>
It impacts a number of functions besides fill_to(), e.g. sp(), move_to(),
indent(). I did not dive down into all printers in vmError.cpp to check
which of them uses these functions.

Generally, the effect will be that all of the above functions now work as
intended. That means that existing output may change sightly, especially
how whitespaces are printed. I do not see how existing output could be
garbled (e.g. trunctated) by this change, but I do not oversee all effects.
So, for instance, if there are tests whcih parse an hs-err file, those may
be triggered.

However, note that this particular change is life in our VM since 2006, so
in general it should work nicely.

If there are any jtreg tests I could run to test hs-err file output
regressions, pls let me know.



>
>>     Aside: Looking at these other uses of update_position:
>>
>>     void fileStream::write(const char* s, size_t len) {
>>        if (_file != NULL)  {
>>          // Make an unused local variable to avoid warning from gcc 4.x
>>     compiler.
>>          size_t count = fwrite(s, 1, len, _file);
>>        }
>>        update_position(s, len);
>>     }
>>
>>     void fdStream::write(const char* s, size_t len) {
>>        if (_fd != -1) {
>>          // Make an unused local variable to avoid warning from gcc 4.x
>>     compiler.
>>          size_t count = ::write(_fd, s, (int)len);
>>        }
>>        update_position(s, len);
>>     }
>>
>>     shouldn't they be inside the if statement ??
>>
>>
>> Not sure but I think in both cases it does not matter, apart from
>> uselessly burning cycles. Because there is no way to reopen a file if it
>> fails to open in the constructors of fdStream or fileStream. And if it
>> cannot be ever reopened, nothing will ever be written for this stream
>> again, so bookkeeping newlines and position has no purpose.
>>
>
> Looks weird to me :)
>
>
I agree.

..Thomas


> Thanks,
> David
>
>
> Kind Regards, Thomas
>>
>>     Cheers,
>>     David
>>     -----
>>
>>
>>
>>         (I need this function because I am implementing an AIX-specific
>>         callstack
>>         dumper and with the broken fill_to the output looks quite ugly)
>>
>>         Bug: https://bugs.openjdk.java.net/browse/JDK-8145410
>>         webrev:
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8145410-fill_to-errorlog/webrev.00/webrev/
>>
>>         Thanks!
>>
>>         Thomas
>>
>>
>>