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

[Daniel D. Daugherty]

On 4/18/18 3:58 AM, Langer, Christoph wrote:
> cc-ing hotspot-runtime-dev as Dan mentioned in the bug that it should rather be there.

The review conversation is fine in hotspot-dev at ...

I moved the bug to hotspot/runtime since that it had no subcat set
which means that the Runtime triage team will pick it up. I believe
the policy for the 'hotspot' category is that a subcat should be set.


> Hi Thomas, David,
>
> I think it could be cleaned up nicely by removing jio_print and jio_printf from jvm.cpp and doing all in ostream.cpp. At the one place where we would call jio_print after my patch, we can do the vfprintf hook check and then either write to the hook or do the raw write. And the other 2 places where jio_printf is employed, we can just call jio_vfprintf(defaultStream::output_stream()). I don't see any other users of jio_print and jio_printf in the jdk repo.
>
> Any objections to that (e.g. some Oracle closed coding that'd break)? Otherwise I'll prepare something...

$ grep -r jio_print closed

Showed nothing found.

Dan


>
> Best regards
> Christoph
>
>
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Mittwoch, 18. April 2018 07:59
>> To: Thomas Stüfe <thomas.stuefe at gmail.com>
>> Cc: Langer, Christoph <christoph.langer at sap.com>; hotspot-
>> dev at openjdk.java.net
>> Subject: Re: RFR(S): 8201649: Remove dubious call_jio_print in ostream.cpp
>>
>> 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
>>>>>>