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

David Holmes david.holmes at oracle.com
Sat May 16 04:01:13 UTC 2020 

Hi Christian,

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.

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.

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

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