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

Fri May 15 14:06:36 UTC 2020

While working on the replacement of Verbose by Unified Logging, I've 
seen that we should also fix this bug for ARM and S390 (PPC and SPARC 
already have the ResourceMark set correctly and Aarch64 is not 
implementing trace_method_handle_stub()). I included the necessary 
changes in a new webrev:
http://cr.openjdk.java.net/~chagedorn/8244946/webrev.02/

Would be good if anybody could also have a quick look at these minor 
changes for ARM and S390 and verify them (I have only built and tested 
it on x86):

Thank you!

Best regards,
Christian

On 15.05.20 15:08, Christian Hagedorn wrote:
> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>
>>