RFR(M) 8155980: ARM InterpreterMacroAssembler::get_method_counters() should not be saving callee saved registers

[Chris Plummer]

Sigh, there's a bug in the following:

#ifdef AARCH64
       __ stp(Rdisp, R3_bytecode, Address(Rstack_top, -2*wordSize, 
pre_indexed));
#else
       __ push(RegisterSet(Rdisp) | RegisterSet(R3_bytecode));
#endif
       __ get_method_counters(Rmethod, Rcounters, dispatch);
#ifdef AARCH64
       __ ldp(Rdisp, R3_bytecode, Address(Rstack_top, 2*wordSize, 
post_indexed));
#else
       __ pop(RegisterSet(Rdisp) | RegisterSet(R3_bytecode));
#endif

Note that the "dispatch" label is passed to get_method_counters(), and 
it will generate code that branches to it, thus by passing the pop. Need 
to rethink this. I might need  to introduce a new label just before the 
dispatch label that will do the pop. The other choice is possibly 
get_method_counters() should be told which registers to save.

BTW, while looking over this code more carefully, it looks like for 
AARCH64, r5 also needs to be saved and restored. It's volatile on 
AARCH64, and is being used to store Rbumped_taken_count.

Chris

On 1/9/17 9:08 AM, Chris Plummer wrote:
> Thanks Dean. Unfortunately I got a crash over the weekend when 
> repeatedly running one of our test suites. Happens about 1 out of 10 
> times on both 32-bit and 64-bit. Looking into it now.
>
> Chris
>
> On 1/6/17 3:57 PM, dean.long at oracle.com wrote:
>> It looks good but you might want to add a comment in 
>> TemplateTable::branch() saying something like:
>>
>> // save and restore caller-saved registers that will be trashed by 
>> call_VM
>>
>> dl
>>
>>
>> On 1/6/17 3:14 PM, Chris Plummer wrote:
>>> Hello,
>>>
>>> Please review the following arm changes. They will be pushed to JDK 
>>> 10 only (when it opens).
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8155980
>>> http://cr.openjdk.java.net/~cjplummer/8155980/webrev.02/webrev.hotspot
>>>
>>> JDK-8012544 added a bunch of callee register saving to 
>>> InterpreterMacroAssembler::get_method_counters() around the 
>>> call_VM() call. The reason is because there are a couple of callers 
>>> of get_method_counters() that need to have r0 and r3 preserved. 
>>> get_method_counters() should not be responsible for having to save 
>>> callee saved registers. It does not know which ones are in use when 
>>> it is called, so it can't do this efficiently. In fact 2 of the 4 
>>> callers of get_method_counters() do not need any registers saved. 
>>> For this reason the callee of get_method_counters() should be 
>>> responsible for saving callee registers.
>>>
>>> This solution would have been been pretty straight forward, except 
>>> that AARCH64 does not allow SP to go above extended_sp, so when 
>>> setting up extended_sp I needed to make sure there will be room for 
>>> the 2 registers that need to be pushed. extended_sp is mainly based 
>>> on max_stack() for the method, plus an extra slot for jsr292 and 
>>> exception handling (but not both at the same time). So the fix here 
>>> is mostly about making sure there are always at least 2 extra slots 
>>> for pushing the 2 registers.
>>>
>>> Here are the changes:
>>>
>>> interp_masm_arm.cpp
>>>   -No longer save/restore registers in get_method_counters():
>>>
>>> templateTable_arm.cpp:
>>>   -Save/restore Rdisp and R3_bytecode to stack around calls to 
>>> get_method_counters.
>>>
>>> abstractinterpreter__arm.cpp::
>>>   -Properly account for extra 2 slots needed on AARCH64 when 
>>> creating a frame
>>>    in layout_activation()
>>>   -Note I switched to using method->constMethod()->max_stack() because
>>>    method->max_stack() includes Method::extra_stack_entries(), and I 
>>> want to
>>>    account for Method::extra_stack_entries() separately.
>>>
>>> templateInterpreterGenerator_arm.cpp:
>>>   -Properly account for extra 2 slots needed on AARCH64 when 
>>> creating a frame
>>>    in generate_normal_entry().
>>>
>>> I've tested the following with -Xcomp and with -Xmixed on arm32 and 
>>> arm64:
>>>
>>>   Kitchensink
>>>   vm.mlvm
>>>   :jdk_svc
>>>   :hotspot_runtime
>>>   :hotspot_serviceability
>>>   :hotspot_compiler
>>>   :jdk_lang
>>>   :jdk_util
>>>
>>> I also tested the test case from JDK-8012544.
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>>
>>
>