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

[Langer, Christoph]

cc-ing hotspot-runtime-dev as Dan mentioned in the bug that it should rather be there.

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

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