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

Fri May 15 12:56:59 UTC 2020

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 for filing an RFE to do more logging with Unified Logging.
Coleen

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