RFR(S): 8149557: Resource mark breaks printing to string stream

[Lindenmaier, Goetz]

> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Tuesday, March 01, 2016 5:29 AM
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'Thomas Stüfe'
> <thomas.stuefe at gmail.com>
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(S): 8149557: Resource mark breaks printing to string stream
> 
> Hi Goetz,
> 
> On 29/02/2016 11:09 PM, Lindenmaier, Goetz wrote:
> > Hi David, Thomas,
> >
> > you are right, I need to check for NULL. I had the linux
> > case in mind.
> > New webrev:
> > http://cr.openjdk.java.net/~goetz/wr16/8149557-ResMrk/webrev.02/
> 
> I can't help but feel there is a bug in the Resource-area memory
> management code here. The problem as I understand it is that we have:
> 
>   void Symbol::print_symbol_on(outputStream* st) const {
>      ResourceMark rm;
>      st = st ? st : tty;
>      st->print("%s", as_quoted_ascii());
>    }
> 
> where we need the ResourceMark for the string returned by
> as_quoted_ascii. But if st is itself a resource-area object and needs to
> re-size in st->print then it does that in the context of the new
> ResourceMark, which fails the assert: "stringStream is re-allocated with
> a different ResourceMark." That seems to be the bug to me - a
> resource-area object that needs resizing should be able to do it in the
> context of an outer ResourceMark. :(
> 
> So the workaround is to use a separate ResourceMark to call
> as_quoted_ascii and then copy the result to a malloc'd string, which is
> used for the st->print (which may realloc in the context of the existing
> outer ResourceMark) and is then free()'d. Yuck. :(
Yes, this exactly describes the situation.  I wouldn't call it a bug in the Resource-area memory
management, but an unpleasant implementation detail of stringStream, that
makes it behave differently from its related classes.

> 
> Reluctantly Reviewed.
Thanks!

Would you mind sponsoring?  

Best regards,
  Goetz.


> 
> Thanks,
> David
> 
> 
> > Best regards,
> >    Goetz.
> >
> >
> >> -----Original Message-----
> >> From: David Holmes [mailto:david.holmes at oracle.com]
> >> Sent: Montag, 22. Februar 2016 01:55
> >> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'Thomas Stüfe'
> >> <thomas.stuefe at gmail.com>
> >> Cc: hotspot-runtime-dev at openjdk.java.net
> >> Subject: Re: RFR(S): 8149557: Resource mark breaks printing to string
> stream
> >>
> >> On 19/02/2016 11:05 PM, Lindenmaier, Goetz wrote:
> >>> Hi Thomas,
> >>>
> >>> thanks for looking at this.
> >>>
> >>>> you should check the return value of os::strdup().
> >>> strdup returns null on oom, %s should be ‘null’, and
> >>
> >> What are you assuming takes care of %s to 'null' conversion? gcc printf
> >> may do it but AFAIK it is undefined at the C library level.
> >>
> >> David
> >> -----
> >>
> >>> free handles null properly, too.  So I think this is fine.
> >>>
> >>> If I remove the RM, I might break callers. E.g.
> >> TestLogTouchedMethods.java.
> >>> Also, in that test, all logged method names will accumulate in memory.
> >>>
> >>> Best regards,
> >>>     Goetz.
> >>>
> >>>
> >>> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
> >>> Sent: Thursday, February 11, 2016 9:41 AM
> >>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> >>> Cc: hotspot-runtime-dev at openjdk.java.net
> >>> Subject: Re: RFR(S): 8149557: Resource mark breaks printing to string
> >> stream
> >>>
> >>> Hi Goetz,
> >>>
> >>> Thank you for fixing this. I see you are restless :-)
> >>>
> >>> you should check the return value of os::strdup(). The change looks fine
> >> otherwise, though I wonder whether we could just omit the
> ResourceMark
> >> and using the one of the caller. At least it is inconsistent, because
> >> Symbol::print_utf8_on() does not have a ResourceMark either.
> >>>
> >>> Kind regards, Thomas
> >>>
> >>>
> >>>
> >>> On Thu, Feb 11, 2016 at 9:24 AM, Lindenmaier, Goetz
> >> <goetz.lindenmaier at sap.com<mailto:goetz.lindenmaier at sap.com>>
> wrote:
> >>> Hi
> >>>
> >>> Please review this change. I please need a sponsor.
> >>> http://cr.openjdk.java.net/~goetz/wr16/8149557-ResMrk/webrev.01/
> >>>
> >>> If a stringStream is passed to Symbol::print_symbol_on(),
> >>> assertion "stringStream is re-allocated with a different ResourceMark."
> can
> >> fire in stringStream::write(const char* s, size_t len).
> >>> This can be verified by running TestLogTouchedMethods.java after
> adding
> >> the patch below to the VM.
> >>>
> >>> My fix copies the string to locally managed memory before popping the
> >> ResoreceMark.
> >>> A better solution would be to guarantee enough space on the stream
> >>> before doing the resource mark, but outputStream has no such
> >>> method as 'ensureCapacity' or the like.
> >>> Alternatively the ResourceMark could be skipped altogether.
> >>> But then the aux memory used remains on the ResourceArea too long.
> >>> Also, various ResourceMarks have to be added in other places.
> >>>
> >>> Best regards,
> >>>     Goetz.
> >>>
> >>>
> >>> --- a/src/share/vm/runtime/java.cpp     Wed Jan 13 11:33:21 2016 +0100
> >>> +++ b/src/share/vm/runtime/java.cpp     Wed Feb 10 08:56:14 2016
> +0100
> >>> @@ -340,7 +340,10 @@
> >>>      }
> >>>
> >>>      if (LogTouchedMethods && PrintTouchedMethodsAtExit) {
> >>> -    Method::print_touched_methods(tty);
> >>> +    ResourceMark rm;
> >>> +    stringStream st(16);
> >>> +    Method::print_touched_methods(&st);
> >>> +    tty->print("%s", st.as_string());
> >>>      }
> >>>
> >>>      if (PrintBiasedLockingStatistics) {
> >>>