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

David Holmes david.holmes at oracle.com
Tue May 19 05:50:47 UTC 2020 

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