Request for review (XXS): -XX:+TraceBytecodes does not work with -XX:+UseCompressedOops

[Coleen Phillimore]

I haven't seen any other feedback, so I assume this is 
non-controversial.  I filed bug 7122939 and start the checkin.
Thanks,
Coleen

On 12/16/2011 6:57 PM, Volker Simonis wrote:
> Thank you Coleen.
>
> Regards,
> Volker
>
> On Fri, Dec 16, 2011 at 7:00 PM, Coleen Phillimore
> <coleen.phillimore at oracle.com>  wrote:
>> Volker, Thank you for fixing this.  It looks good to me.   I can check it in
>> this contribution, pending additional comments.
>>
>> Coleen
>>
>>
>> On 12/16/2011 12:20 PM, Volker Simonis wrote:
>>> Hi,
>>>
>>> I've just realized that -XX:+TraceBytecodes does not work anymore if
>>> -XX:+UseCompressedOops is set.
>>>
>>> This is because TemplateInterpreterGenerator::trace_bytecode() uses
>>> 'r12' which holds the heap base for temporary saving the 'sp' before
>>> calling the Interpreter::trace_code() template:
>>>
>>>    __ mov(r12, rsp); // remember sp (can only use r12 if not using call_VM)
>>>    __ andptr(rsp, -16); // align stack as required by ABI
>>>    __ call(RuntimeAddress(Interpreter::trace_code(t->tos_in())));
>>>    __ mov(rsp, r12); // restore sp
>>>    __ reinit_heapbase();
>>>
>>> Notice that the comment explicitly mentions that 'r12' can only be
>>> used if 'call_VM' will not be used subsequently.
>>>
>>> But this is exactly what Interpreter::trace_code() does:
>>>
>>>
>>> address TemplateInterpreterGenerator::generate_trace_code(TosState state)
>>> {
>>> ...
>>>    __ call_VM(noreg,...
>>>
>>> void MacroAssembler::call_VM(Register oop_result,
>>> ...
>>>    call_VM_helper(oop_result, entry_point, 3, check_exceptions);
>>>
>>> void MacroAssembler::call_VM_helper(Register oop_result, address
>>> entry_point, int number_of_arguments, bool check_exceptions) {
>>> ...
>>>    call_VM_base(oop_result, noreg, rax, entry_point,
>>> number_of_arguments, check_exceptions);
>>>
>>> void MacroAssembler::call_VM_base(Register oop_result,
>>> ...
>>>    LP64_ONLY(if (UseCompressedOops) verify_heapbase("call_VM_base");)
>>>
>>>
>>> This finally leads to a call to 'verify_heapbase()' which will fail
>>> because 'r12' has been overwritten.
>>>
>>> The proposed fix is trivial: don't do the check if we are running with
>>> -XX:+TraceBytecodes:
>>>
>>> diff -r 86cbe939f0c7 src/cpu/x86/vm/assembler_x86.cpp
>>> --- a/src/cpu/x86/vm/assembler_x86.cpp  Mon Sep 19 12:18:46 2011 -0700
>>> +++ b/src/cpu/x86/vm/assembler_x86.cpp  Fri Dec 16 18:01:31 2011 +0100
>>> @@ -5967,7 +5967,7 @@
>>>     assert(number_of_arguments>= 0   , "cannot have negative number of
>>> arguments");
>>>     LP64_ONLY(assert(java_thread == r15_thread, "unexpected register"));
>>>   #ifdef ASSERT
>>> -  LP64_ONLY(if (UseCompressedOops) verify_heapbase("call_VM_base");)
>>> +  LP64_ONLY(if (UseCompressedOops&&    !TraceBytecodes)
>>>
>>> verify_heapbase("call_VM_base");)
>>>   #endif // ASSERT
>>>
>>>     assert(java_thread != oop_result  , "cannot use the same register
>>> for java_thread&    oop_result");
>>>
>>>
>>> For your convenience I've also attached the patch as a file to this
>>> message.
>>>
>>> Regards,
>>> Volker