[15] RFR(XS): 8244946: fatal error: memory leak: allocating without ResourceMark with -XX:+Verbose -Xlog:methodhandles

[Christian Hagedorn]

On 15.05.20 00:38, David Holmes wrote:
> On 15/05/2020 12:02 am, coleen.phillimore at oracle.com wrote:
>>
>> I thought that this code under Verbose should have been changed to use 
>> logging with log_level = verbose.  We were trying to minimize/remove 
>> the Verbose flag.
>>
>> On 5/14/20 9:50 AM, David Holmes wrote:
>>> +1
>>>
>>> But I'd be inclined to generalize the test to run:
>>>
>>> -Xlog:all=trace -XX:+Verbose
>>
>> Don't do this, it would be tons of output and make the test unusable 
>> if it fails.
> 
> The only way the test can fail is if it crashes.
> 
> But we would want a level of indirection so that the test log doesn't 
> contain the full output ~11000 lines of text.

Could we still generalize the test but redirect the output to a file with:

-Xlog:all=trace:file=output.txt -XX:+Verbose ?

When I just immediately return from trace_method_handle_stub() without 
printing anything, the above command will not print anything to the 
console. So, it would print the same amount of lines to the console as 
the current option in webrev.01 with -XX:+Verbose -Xlog:methodhandles.

In a next step we could file an RFE to replace Verbose with unified 
logging in trace_method_handle_stub() which currently is only using tty. 
Even when using -Xlog:methodhandles alone without -XX:+Verbose, there is 
still some printing done in trace_method_handle_stub() to tty by

  500   tty->print_cr("MH %s %s=" PTR_FORMAT " sp=" PTR_FORMAT,
  501                 adaptername, mh_reg_name,
  502                 p2i(mh), p2i(entry_sp));

which is probably also not desired, as it is guarded by 
log_is_enabled(Info, methodhandles) earlier [1]. This could also be 
fixed in this follow-up RFE.

What do you think?

Best regards,
Christian


[1] 
http://hg.openjdk.java.net/jdk/jdk/file/748fedeb7cc1/src/hotspot/cpu/x86/methodHandles_x86.cpp#l598

> 
> David
> -----
> 
>> Coleen
>>
>>>
>>> Curiously I couldn't reproduce the crash that this bug is about. But 
>>> the hs_err file speaks for itself.
>>>
>>> Cheers,
>>> David
>>>
>>> On 14/05/2020 11:28 pm, Daniel D. Daugherty wrote:
>>>> On 5/14/20 4:00 AM, Christian Hagedorn wrote:
>>>>> Hi David, hi Daniel
>>>>>
>>>>> Thank you for your reviews!
>>>>>
>>>>> On 13.05.20 17:54, Daniel D. Daugherty wrote:
>>>>>>      Not your bug, but the comment has two typos:
>>>>>>
>>>>>>          s/by safer/but safer/
>>>>>>          s/unexpensive/inexpensive/
>>>>>
>>>>> Thanks, fixed it.
>>>>>
>>>>> On 14.05.20 01:51, David Holmes wrote:
>>>>>> src/hotspot/cpu/x86/methodHandles_x86.cpp
>>>>>>
>>>>>> Given all of the code is inside:
>>>>>>
>>>>>>   504   if (Verbose) {
>>>>>>
>>>>>> I would just move the ResourceMark to the top of that block. We 
>>>>>> are going to use it unconditionally, and we are not releasing it 
>>>>>> until the end of the method regardless. That avoids the potential 
>>>>>> concern Dan raised about extending the scope of the 
>>>>>> PRESERVE_EXCEPTION_MARK.
>>>>>
>>>>> This sounds reasonable. I moved it to the top of that block.
>>>>>
>>>>>>> There are other places like Klass::print_on(outputStream*) where 
>>>>>>> this is still the case. Should we file an RFE to clean these up?
>>>>>>
>>>>>> Obviously we don't have any tests running in a mode that causes 
>>>>>> this code to be executed, so yes it is probably a good idea to 
>>>>>> take a closer look at other call-sites and file a RFE if needed.
>>>>>
>>>>> Yes, I filed one to take a closer look at these [1].
>>>>>
>>>>> I included the changes in a new webrev:
>>>>> http://cr.openjdk.java.net/~chagedorn/8244946/webrev.01/
>>>>
>>>> Thumbs up.
>>>>
>>>> Dan
>>>>
>>>>>
>>>>> Best regards,
>>>>> Christian
>>>>>
>>>>>
>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8244999
>>>>>
>>>>
>>