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

[Christian Hagedorn]

Hi Coleen

On 15.05.20 12:38, coleen.phillimore at oracle.com wrote:
> 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.

I could already reproduce it locally on my Linux machine with 
-Xlog:methodhandles -XX:+Verbose. From what I understood, I thought that 
David is just talking about having a more general log test that covers 
the entire logging together with -XX:+Verbose for a HelloWorld program. 
But as you said, that would have some bad effects on test execution. 
However, such a test is not required for this bug to be triggered.

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

Great, I filed an RFE for it [1].

Best regards,
Christian


[1] https://bugs.openjdk.java.net/browse/JDK-8245107


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