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

[David Holmes]

On 15/05/2020 8:38 pm, 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.

This issue was filed because combining -Xlog:methodhandles
and -XX:+Verbose caused a crash, because we do not adequately test these 
things and so we can introduce bugs like the current one. Hence my 
suggestion to run a sanity test that enables all logging with Verbose to 
catch such regressions. The output file is a bit over 1MB - hardly huge.

But I don't want to hold Christian up here, what he has is better than 
nothing.

David
-----

>>
>> 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.
> 
> Thanks,
> Coleen
>>
>> What do you think?
>>
>> Best regards,
>> Christian
>>
>>
>> [1] 
>> http://hg.openjdk.java.net/jdk/jdk/file/748fedeb7cc1/src/hotspot/cpu/x86/methodHandles_x86.cpp#l598 
>>
>>
>>>
>>> David
>>> -----
>>>
>>>> Coleen
>>>>
>>>>>
>>>>> 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
>>>>>>>
>>>>>>
>>>>
>