RFR(S): 8201649: Remove dubious call_jio_print in ostream.cpp

[Thomas Stüfe]

Hi David,

On Tue, Apr 17, 2018 at 11:17 PM, David Holmes <david.holmes at oracle.com> wrote:
> On 18/04/2018 1:48 AM, Thomas Stüfe wrote:
>>
>> Hi Christoph,
>>
>> I do not understand jio_print() at all. I think it is just wrong: if a
>> vfprintf hook is set, it prints to the defaultStream::output_stream(),
>> otherwise to defaultStream::output_fd()? Isnt that the same? Compare
>> this to jio_vfprintf(), which does the same logic, but correctly
>> prints to the vfprintf hook if it is set.
>
>
> If there is a hook it does a formatted print: jio_print -> jio_fprintf ->
> jio_vfprintf -> Arguments::vfprintf_hook()
> else it does a raw write.

Oh, I missed that.

So, jio_print() checks if a vfprintf hook is installed.

If it is, it calls jio_fprintf() with defaultStream::output_stream()
as output file handle. It expects jio_fprintf() to call
jio_vfprintf(), which should do the same check again and come to the
same conclusion, then should disregard the output file handle handed
down, instead call the vfprintf hook.

(The output file handle is handed down as argument to the vfprintf
hook; we had a discussion last year about it:
http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-May/026794.html
and we were not sure that made any sense.)

Not very clear coding. I think that can be improved by removing the
vfprintf hook check from jio_print completely and make jio_vfprintf()
be the only place where the vfprintf hook is handled.

For example, this would be equivalent:

void jio_print(const char* s) {
  jio_printf("%s", s);
}

or this:

void jio_print(const char* s) {
  jio_fprintf(defaultStream::output_stream(), "%s", s);
}

in which case we could also maybe also remove jio_printf(), because it
is nowhere used inside the hotspot (though it is exported, but I find
no reference to it being used anywhere in the OpenJDK).

>
> Now why it does this is another matter. I have no idea. But I wouldn't
> suggest changing it just because I don't know why it's done the way it is.

I was reasonably sure I had understood the function, and that it was
broken. In that case the proposal to change or remove it made sense.

Thomas

>
> David
>
>
>
>> I would propose to get rid of jio_print() altogether and replace the
>> few callers of it (all in ostream.cpp) with this:
>>
>> jio_printf("%s", string);
>>
>> which does the same, but correctly.
>>
>> Best Regards, Thomas
>>
>> On Tue, Apr 17, 2018 at 4:48 PM, Langer, Christoph
>> <christoph.langer at sap.com> wrote:
>>>
>>> Hi,
>>>
>>> can you please review a fix proposal for defaultStream::write(const char*
>>> s, size_t len).
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8201649
>>> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8201649.0/
>>>
>>> I have seen occurrences of truncated buffers which don't need to happen.
>>>
>>> Thanks and best regards
>>> Christoph
>>>
>