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

[Langer, Christoph]

Hi David, Thomas,

@David: No, I don't think it needs to be dialed back at this point, but I agree, the bug should be updated somewhat to the current scope by mentioning that the goal is to get rid of jio_print and jio_printf completely (along with call_jio_print in ostream.cpp). Those 2 functions were just 2 hooks used in ostream.cpp and got routed through jio_vfprintf anyway, except for the raw write case when no vfprint_hook was installed - and we'll preserve that.

As for your concern, Thomas: I don't think this should hold us back here. The jio_vfprintf function is used ubiquitously throughout the jdk and the same concern would apply. Though obviously the vfprintf hook functionality is not documented very well, I guess in fact we rely on Posix compliant vfprintf implementations already for quite some time. Plus we are not talking about a downport into a delivered JDK here but it's about the upcoming JDK11.

Best regards
Christoph

> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Donnerstag, 19. April 2018 00:11
> To: Thomas Stüfe <thomas.stuefe at gmail.com>; Langer, Christoph
> <christoph.langer at sap.com>
> Cc: daniel.daugherty at oracle.com; hotspot-dev at openjdk.java.net; hotspot-
> runtime-dev at openjdk.java.net
> Subject: Re: RFR(S): 8201649: Remove dubious call_jio_print in ostream.cpp
> 
> I'm losing track of the original goal here and the scope of this seems
> to be blowing out into unknown territory. We don't know why we need the
> raw write; and we don't know what we can assume about the vprintf-hook.
> 
> I suggest to dial this back to whatever real issue was initially being
> addressed.
> 
> Cheers,
> David
> 
> On 19/04/2018 1:55 AM, Thomas Stüfe wrote:
> > Hi Chrisoph,
> >
> > thanks for the new webrev, this looks all very reasonable.
> >
> > Unfortunately a small issue occurred to me while thinking this over...
> > Sigh, this turns out to be more complicated than I thought.
> >
> > The original intent of this change was to get rid of that extra copy
> > step we do in "call_jio_print" which aims to ensure that the string we
> > hand down to jio_print is zero terminated.
> >
> > But ultimately, the problem was never jio_print, but the vfprintf
> > hook: The vfprintf hook has the form "foo(FILE*, char* fmt, ....) and
> > this is a contract with an embedding program - it has to provide a
> > "fprinf-like function" but ultimately can do whatever it wants with
> > the arguments we give it.
> >
> > (IMHO this whole vfprintf hook thing was very badly designed. We had a
> > number of issues with this already. For a discussion about some
> > details, see e.g.
> > http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-
> May/026794.html
> > .)
> >
> > The problem I see is that now we may call the vfprintf hook with
> > something like ("%.*s", precision, s) with the explicit intention of
> > passing in not-zero-terminated strings for s and a precision which
> > prevents us reading beyond the end of s. This was the whole point - we
> > avoid the copy step.
> >
> > As we discussed offlist, this would be a perfectly valid fix were we
> > to feed those arguments to a standard fprintf() function which is
> > POSIX compatible, because POSIX states for the format specifier 's':
> >
> > <quote>
> > The argument shall be a pointer to an array of char. Bytes from the
> > array shall be written up to (but not including) any terminating null
> > byte. If the precision is specified, no more than that many bytes
> > shall be written. If the precision is not specified or is greater than
> > the size of the array, the application shall ensure that the array
> > contains a null byte.
> > </quote>
> >
> > This explicitly allows us to pass a pointer to a non-zero-terminated
> > string as long as the precision is smaller than its length.
> >
> > However, the argument vfprintf hook can be anything. It is a
> > user-provided function. Usually they will probably just call vxxprintf
> > functions from the libc, but for all we know they may roll their own
> > printf routine. So, we may uncover bugs in their implementation.
> >
> > Seeing that the vfprintf hook is very badly documented, we move in
> > gray area here. We may break user code which has a bad, non-Posix
> > implementation of the vfprintf hook.
> >
> > I would like to know if others think this concern is valid.
> >
> > Otherwise, the patch looks good.
> >
> >
> > ..Thomas
> >
> >
> >
> > On Wed, Apr 18, 2018 at 5:01 PM, Langer, Christoph
> > <christoph.langer at sap.com> wrote:
> >> Hi,
> >>
> >>>> 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.
> >>
> >> I've prepared a new webrev:
> http://cr.openjdk.java.net/~clanger/webrevs/8201649.1/
> >>
> >> I remove jio_printf and jio_print completely.
> >>
> >> If we agree that in defaultStream::write(), the raw write is not needed
> and we can use fprintf, we could further simplify the code to just  call
> jio_fprintf.
> >>
> >> Furhtermore, I have updated the jio_*print* function declarations in
> jvm.cpp to match those of jvm.h. Is that desired (I thought yes, in times
> where we try to get rid of the mapfiles anyway). I wonder if those symbols
> can be taken out of make/hotspot/symbols/symbols-shared then...
> >>
> >> Best regards
> >> Christoph