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

[coleen.phillimore at oracle.com]

On 5/15/20 4:03 AM, Christian Hagedorn wrote:
> 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.

I'm confused.  Does the test crash with -Xlog:methodhandles 
-XX:+Verbose?  If so, that's all that should be needed.  If it needs 
-Xlog:all=trace, then don't add this test.  Even if it writes to an 
output file, it would be huge and affect test execution, particularly if 
more logging is added.
>
> 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.

Please, yes, this can be fixed with a follow-up RFE.

Thanks,
Coleen
>
> 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
>>>>>>
>>>>>
>>>