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

Daniel D. Daugherty daniel.daugherty at oracle.com
Wed Apr 18 13:11:31 UTC 2018

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 jioprint and jioprintf from jvm.cpp and doing all in ostream.cpp. At the one place where we would call jioprint 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 jioprintf is employed, we can just call jiovfprintf(defaultStream::outputstream()). I don't see any other users of jioprint and jioprintf 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 calljioprint 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 jioprint() at all. I think it is just wrong: if a vfprintf hook is set, it prints to the defaultStream::outputstream(), otherwise to defaultStream::outputfd()? Isnt that the same? Compare this to jiovfprintf(), 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: jioprint -> jiofprintf -> jiovfprintf -> Arguments::vfprintfhook() else it does a raw write. Oh, I missed that. So, jioprint() checks if a vfprintf hook is installed. If it is, it calls jiofprintf() with defaultStream::outputstream() as output file handle. It expects jiofprintf() to call jiovfprintf(), 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 jioprint completely and make jiovfprintf() be the only place where the vfprintf hook is handled. For example, this would be equivalent: void jioprint(const char* s) { jioprintf("%s", s); } or this: void jioprint(const char* s) { jiofprintf(defaultStream::outputstream(), "%s", s); } They aren't equivalent because the always do a formatted write, never the raw write the currently exists. Does that matter? 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 jioprintf(), 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 jioprint() altogether and replace the few callers of it (all in ostream.cpp) with this: jioprintf("%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, sizet 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-dev mailing list