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

[Christian Hagedorn]

On 15.05.20 14:56, coleen.phillimore at oracle.com wrote:
> On 5/15/20 8:06 AM, Christian Hagedorn wrote:
>> 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.
> 
> Good! This looks good to me then.

Thank you Coleen for your review!

> Thank you for filing an RFE to do more logging with Unified Logging.

No problem, I will take a look at it next.

Best regards,
Christian

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