RFR(xs): 8224487: outputStream should not be copyable

[Thomas Stüfe]

Hi Kim,

thanks for the review. Answers inline.

On Tue, May 21, 2019 at 5:56 PM Kim Barrett <kim.barrett at oracle.com> wrote:

> > On May 21, 2019, at 9:47 AM, Thomas Stüfe <thomas.stuefe at gmail.com>
> wrote:
> >
> > Hi all,
> > may I please have a review for this small fix. With JDK-8224193 (not yet
> > pushed) I saw a number of crashes which were caused by code handing
> around
> > copies of outputStream objects but copy semantics are not implemented.
> >
> > This fix forbids copying for outputStream and its descendants and fixes
> the
> > few places where that happened.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8224487
> > cr:
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8224487-make-streams-not-copyable/webrev.00/webrev/
> >
> > Note: In event log coding, I removed the convenience function
> stringStream
> > FormatStringLogMessage::stream(). This makes the (very few) call sites
> > slightly more verbose but I have open for RFR a cleanup of the event log
> > system (see JDK-8220762,
> >
> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2019-May/034349.html
> )
> > which will simplify and cleanup this coding anyway.
> >
> > Thank you,
> >
> > Thomas
>
> Generally nice.  Just a couple of comments.
>
>
> ------------------------------------------------------------------------------
> src/hotspot/share/utilities/ostream.hpp
>   46    outputStream(const outputStream&);
>
> Copy-assign should also be poisoned.  Hopefully that won't find too
> many more problems.
>
>
Good point! Will do that. I do not expect many surprises.


>
> ------------------------------------------------------------------------------
> src/hotspot/share/utilities/exceptions.hpp
>  189   static void log_exception(Handle exception, stringStream tempst);
> =>
>  189   static void log_exception(Handle exception, const char* message);
>
> Why not change the stringStream parameter to be passed by const-ref
> rather than by value?  That would avoid changing all the callers.
>
>
Because handing the whole stream object down to log_exception was overkill
anyway. All it wants is to print a text message, so I just give it that and
live with a slightly larger patch diff.

Also, I am not a big fan of references but that is just me. I usually
prefer to see at the call site if I hand in value or reference without
looking up the prototype.


>
> ------------------------------------------------------------------------------
> src/hotspot/share/utilities/events.hpp
> Removed from class FormatStringLogMessage
>  140  public:
>  141   // Wrap this buffer in a stringStream.
>  142   stringStream stream() {
>  143     return stringStream(this->_buf, this->size());
>  144   }
>
> FormatStringLogMessage seems kind of pointless now.  Maybe it should
> be eliminated?  That could be a separate cleanup.
>
>
Yes. As I wrote, I have a larger patch under review which changes all that
code anyway.

If I just remove FormatStringMessage I must publish the ctor of the parent
class, FormatMessage, and that was just that one step too far for this
patch.


>
> ------------------------------------------------------------------------------
>
>
Thank you! Will post an updated webrev.

Cheers, Thomas