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

Christian Hagedorn christian.hagedorn at oracle.com
Mon May 18 08:56:22 UTC 2020 

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