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

Thomas Stüfe thomas.stuefe at gmail.com
Wed Apr 18 08:32:48 UTC 2018


Sounds good to me.

Gruß Thomas

On Wed, Apr 18, 2018, 09:58 Langer, Christoph <christoph.langer at sap.com>
wrote:

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


More information about the hotspot-runtime-dev mailing list