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

[David Holmes]

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. :(

Reluctantly Reviewed.

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