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

Christian Hagedorn christian.hagedorn at oracle.com
Tue May 19 07:35:50 UTC 2020 

Thanks a lot for your review David! :)

Best regards,
Christian

On 19.05.20 07:50, David Holmes wrote:
> Ship it! :)
> 
> Thanks for your patience on this.
> 
> David
> 
> On 18/05/2020 6:56 pm, Christian Hagedorn wrote:
>> Hi David
>>
>> On 16.05.20 06:01, David Holmes wrote:
>>> On 16/05/2020 12:06 am, Christian Hagedorn wrote:
>>>> 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/
>>>
>>> src/hotspot/cpu/ppc/methodHandles_ppc.cpp
>>> src/hotspot/cpu/x86/methodHandles_x86.cpp
>>>
>>> You should keep the existing block so that the scope of the 
>>> PRESERVE_EXCEPTION_MARK is not changed. All this is needed is to move 
>>> the ResourceMark.
>>
>> I see, I fixed that.
>>
>>> runtime/logging/TestMethodHandlesVerbose.java
>>>
>>> Note that there is no need to declare the test in the runtime.logging 
>>> package. We generally do not use packages for regression tests.
>>
>> I adjusted the test to not use a package.
>>
>>>> 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):
>>>
>>> You say ARM and S390 but the webrev only has changes for ARM and PPC.
>>
>> Oh, sorry. Was meant to say ARM and PPC!
>>
>> I included the changes in a new webrev. I also moved the ResourceMark 
>> on ARM to the start of the block while keeping the scope for 
>> PRESERVE_EXCEPTION_MARK as done for x86 and PPC now:
>> http://cr.openjdk.java.net/~chagedorn/8244946/webrev.03/
>>
>> Best regards,
>> Christian
>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>>
>>>> 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
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>
>>>>>>

More information about the hotspot-dev mailing list