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

[David Holmes]

On 18/04/2018 2:47 PM, Thomas Stüfe wrote:
> 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.)

I had forgotten about that. :)

> 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);
> }

They aren't equivalent because the always do a formatted write, never 
the raw write the currently exists. Does that matter? <shrug> no idea. I 
don't have time to do any archaeology on this piece of code - sorry.

Cheers,
David
-----

> 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
>>>>
>>